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

rules: fix for possible whitespace in the "model" attribute #4077

Closed

Conversation

msekletar
Copy link
Contributor

Without this the rules would create multiple wrong symlinks, because
SYMLINK treats strings with spaces as a list of links to create.

Suggested-by: Artur Paszkiewicz artur.paszkiewicz@intel.com

Without this the rules would create multiple wrong symlinks, because
SYMLINK treats strings with spaces as a list of links to create.

Suggested-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
@msekletar msekletar added the udev label Sep 1, 2016
@msekletar
Copy link
Contributor Author

Note that this patch was not tested because, ATM I don't have access to a machine with NVMe drives.

@msekletar
Copy link
Contributor Author

@kaysievers any chance you could review and possibly merge this?

@keszybz
Copy link
Member

keszybz commented Sep 9, 2016

Hm, spaces in the SYMLINK option could only come from ID_SERIAL, and ID_SERLIAL should not contain spaces?

Copy link
Contributor

@kaysievers kaysievers left a comment

Choose a reason for hiding this comment

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

Hmm, the built-in tools do escaping, to keep the symlink names reversible. I guess we should not mangle the names; so looks like we should add a new mode for string_escape to do do what the other tools are doing to be used for stuff we read from sysfs.

@martinpitt martinpitt added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Oct 10, 2016
@msekletar
Copy link
Contributor Author

Hm, spaces in the SYMLINK option could only come from ID_SERIAL, and ID_SERLIAL should not contain spaces?

ID_SERIAL is composed from two parts, ID_SERIAL_SHORT (that shouldn't contain spaces) and from model sysfs attribute and that may contain spaces.

@msekletar
Copy link
Contributor Author

Hmm, the built-in tools do escaping, to keep the symlink names reversible. I guess we should not mangle the names; so looks like we should add a new mode for string_escape to do do what the other tools are doing to be used for stuff we read from sysfs.

I don't think anyone actually cares about this. I mean you can always look directly at sysfs attribute if you want. IMHO, only thing people care about is symlink being unique and stable.

@msekletar
Copy link
Contributor Author

@martinpitt you've added needs-rework label. I don't think this is accurate, instead we should make our mind about this issue and either merge this or close it. There is not much to be actually reworked.

Maybe close this in favor of #4837?

@keszybz
Copy link
Member

keszybz commented Jan 11, 2017

#4837 should handle this.

In fact, we can probably drop OPTIONS="string_escape=replace" from various other places, but that's subject for another PR.

@keszybz keszybz closed this Jan 11, 2017
@keszybz
Copy link
Member

keszybz commented Jan 11, 2017

Oh, we actually don't use string_escape anywhere, because md/dm rules have moved to mdadm ages ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks udev
Development

Successfully merging this pull request may close these issues.

None yet

4 participants