CA-402814: rationalise canonicalisation of device path #201
Conversation
Signed-off-by: Mark Syms <mark.syms@cloud.com>
MarkSymsCtx
requested review from
andyhhp,
rosslagerwall,
freddy77 and
alexbrett
| @@ -0,0 +1 @@ | |||
| .idea | |||
| @@ -289,9 +289,9 @@ def getDiskBlockSize(dev): | |||
| dev = '/dev/' + dev | |||
| if isDeviceMapperNode(dev): | |||
| return getDiskBlockSize(getDeviceSlaves(dev)[0]) | |||
| if dev.startswith("/dev/"): | |||
| dev = re.match("/dev/(.*)", dev).group(1) | |||
| dev = dev.replace("/", "!") | |||
There was a problem hiding this comment.
What specifically was wrong with the original code? The same pattern is used in several places in the installer (e.g. in getDiskDeviceSize() above) so if it is buggy then it should be fixed everywhere.
There was a problem hiding this comment.
Well for starters injecting ! into a path is doomed to fail and using regexes instead of os primitives is rather reinventing the wheel.
There was a problem hiding this comment.
Apparently there are/were some devices having path separators in their name. And apparently the ! was the /sys path encoding for these devices. os primitives and regexes are doing very different things, some are resolving symlinks converting to physical paths, the others are transforming the paths using just their names.
But @rosslagerwall point still remains, the same code is spread in different places so if one is broken why the others are fine?
I think adding realpath call before instead of replacing should work for both cases (your /dev/by-id/... and the /dev/XXX/YYY)
There was a problem hiding this comment.
And probably an exists check as well
| realpath = os.path.realpath(dev) | ||
| dev = os.path.basename(realpath) |
There was a problem hiding this comment.
Yes, the code removing /dev/ was redundant giving previous lines, however this not explain this change. The same code you are removing and change is present on the previous function. I think this code is dealing with devices having path separator in them. How your code is dealing with them? It looks like you are removing the support of such devices instead.
There was a problem hiding this comment.
Yes, by using realpath to reduce that down to the actual device rather than the symlink, which should not have path separators in its name.
There was a problem hiding this comment.
Some device name have path separators in their names
Rationalise how the dev path is canonicalized to the
/sys/path using realpath and basename.The if was redundant as the first line of the method prepends
/dev/if it missing.