Skip to content
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.

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

CA-402814: rationalise canonicalisation of device path #202

Open
wants to merge 1 commit into
base: release/xs8
Choose a base branch
from

Conversation

MarkSymsCtx
Copy link
Contributor

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.

(this is same as #201 but targeting the release/xs8 branch)

Signed-off-by: Mark Syms <mark.syms@cloud.com>
@MarkSymsCtx
Copy link
Contributor Author

MarkSymsCtx commented Feb 26, 2025

BST has been successfully completed in XenRT. It didn't address the original issue that popped this up as that turned out to be a configuration issue on a host in XenRT claiming to have disks that it did not have.

That was resulting in errors like this

No such file or directory: '/sys/block/disk!by-id!ata-Samsung_SSD_850_PRO_1TB_S252NX0H604307Y/queue/logical_block_size'

which is hardly surprising given the mangling in the path.

@freddy77
Copy link
Collaborator

I think this patch is fixing a problem, introducing another.

@freddy77
Copy link
Collaborator

If I remember, there are some devices (ccsis ??) that have a slash inside the path, something like /dev/ccsis/name. Maybe others. You are removing support for them. Another question is how that /dev/by-id came at that path? Should the device name resolved to "physical" before getting to that path instead ?

@MarkSymsCtx
Copy link
Contributor Author

If I remember, there are some devices (ccsis ??) that have a slash inside the path, something like /dev/ccsis/name. Maybe others. You are removing support for them. Another question is how that /dev/by-id came at that path? Should the device name resolved to "physical" before getting to that path instead ?

That name came from the answerfile supplied by XenRT, in the particular failure case the device, and symlink, didn't actually exist and so we got this garbage. Using realpath should be resolving this to "physical". We can add the replace back in after that is completed and before we take basename is we think there are devices with these characteristics.

@freddy77
Copy link
Collaborator

I found some reference in distutil.py:

# /dev/cciss : c[0-7]d[0-15]: Compaq Next Generation Drive Array
# /dev/ida   : c[0-7]d[0-15]: Compaq Intelligent Drive Array
# /dev/rd    : c[0-7]d[0-31]: Mylex DAC960 PCI RAID controller

We could possibly find some of these arrays in the lab to have a test.

What do you mean by

in the particular failure case the device, and symlink, didn't actually exist and so we got this garbage

I mean, your code with realpath worked, so the symlink should have existed.

@MarkSymsCtx
Copy link
Contributor Author

I found some reference in distutil.py:

# /dev/cciss : c[0-7]d[0-15]: Compaq Next Generation Drive Array
# /dev/ida   : c[0-7]d[0-15]: Compaq Intelligent Drive Array
# /dev/rd    : c[0-7]d[0-31]: Mylex DAC960 PCI RAID controller

We could possibly find some of these arrays in the lab to have a test.

What do you mean by

in the particular failure case the device, and symlink, didn't actually exist and so we got this garbage

I mean, your code with realpath worked, so the symlink should have existed.

I mean that on the host that was misconfigured, so that the answerfile said the guest SR device was

/dev/disk/by-id/ata-Samsung_SSD_850_PRO_1TB_S252NX0H604307Y

that disk wasn't installed so the symlink didn't exist. The code didn't check that it existed or use realpath to resolve it. It would probably be a good idea to add an

if not os.path.exists(dev):
    <raise error>

in this (and related parts).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants