Skip to content

CA-402814: rationalise canonicalisation of device path #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MarkSymsCtx wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CA-402814: rationalise canonicalisation of device path #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.idea
6 changes: 3 additions & 3 deletions diskutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("/", "!")
Copy link
Contributor

@rosslagerwall rosslagerwall Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MarkSymsCtx MarkSymsCtx Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well for starters injecting ! into a path is doomed to fail and using regexes instead of os primitives is rather reinventing the wheel.

Copy link
Contributor

@freddy77 freddy77 Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@MarkSymsCtx MarkSymsCtx Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And probably an exists check as well


realpath = os.path.realpath(dev)
dev = os.path.basename(realpath)
Comment on lines +293 to +294
Copy link
Contributor

@freddy77 freddy77 Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MarkSymsCtx MarkSymsCtx Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@freddy77 freddy77 Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some device name have path separators in their names

return int(__readOneLineFile__("/sys/block/%s/queue/logical_block_size"
% dev))

Expand Down