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

Let is_write_protected() report devices without device node as not write protected #3091

Merged
merged 4 commits into from Jan 22, 2024

Conversation

jsmeix
Copy link
Member

@jsmeix jsmeix commented Nov 23, 2023

I don't know how I could test it.
I cannot reproduce the matching issue
because I don't have a system with /sys/block/nvme0c0n1
or something similar - i.e. where a /sys/block/device
does not have a matching /dev/device.

  • Description of the changes in this pull request:

Let the is_write_protected function
report candidate devices without device node
as not write protected
because not all /sys/block devices have a "dev" file
e.g. /sys/block/nvme0c0n1 has no /dev/nvme0c0n1 device node, see
#3085

Because the is_write_protected function is meant
to identify write-protected disks and partitions
only candidate devices with a device node
are considered for write protection.

Let the is_write_protected function
report candidate devices without device node
as not write protected
because not all /sys/block devices have a "dev" file
e.g. /sys/block/nvme0c0n1 has no /dev/nvme0c0n1 device node
see #3085
Because the is_write_protected function is meant
to identify write-protected disks and partitions
only candidate devices with a device node
are considered for write protection
@jsmeix jsmeix added enhancement Adaptions and new features bug The code does not do what it is meant to do labels Nov 23, 2023
@jsmeix jsmeix added this to the ReaR v2.8 milestone Nov 23, 2023
@jsmeix jsmeix requested a review from a team November 23, 2023 10:00
@jsmeix jsmeix self-assigned this Nov 23, 2023
@jsmeix jsmeix requested a review from pcahyna November 23, 2023 10:14
@jsmeix
Copy link
Member Author

jsmeix commented Nov 23, 2023

@pcahyna
could you please have a look here (as time permits)
because I don't know how to test it as I don't have a system
with /sys/block/nvme0c0n1 or something similar
i.e. where a /sys/block/device does not have a matching /dev/device

Completely overhauled lib/write-protect-functions.sh
which now contains only the is_write_protected function
(the only one intened to be called by other scripts).
The former helper functions are not needed as functions
because all what actually happens is a single straightforward sequence of actions.
So what the helper functions did is now done
step by step directly in the is_write_protected function.
This avoids in particular RFC 1925 item (6a).
@jsmeix
Copy link
Member Author

jsmeix commented Nov 23, 2023

Oops - I did it again ;-)

Because I got confused by the former helper functions
where all used $device but with changed meanings
I needed to clean up things so that at least I can
better understand what goes on to be able to fix
issues that I can neither reproduce nor test
with a higher probability that my fix actually works.

@jsmeix jsmeix added the cleanup label Nov 23, 2023
@jsmeix
Copy link
Member Author

jsmeix commented Nov 23, 2023

I did some tests on my test system
and things worked so far at least for me
so it is now (hopefully) not totally broken.

Some comment text improvements
and some improved variable names
to better describe and tell what things actually are
Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

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

In general I approve, although I also can't really test this specific code for the same reason you can't test it. My motivation for approving is that ReaR should only concern itself with "real" disks, that also have a /dev device node.

If indeed this device is part of NVMe multipathing then we should mention it in the comment and maybe keep in mind that NVMe multipathing is not yet properly supported.

usr/share/rear/lib/write-protect-functions.sh Outdated Show resolved Hide resolved
Add reason
"because it is part of NVMe multipathing"

Co-authored-by: Schlomo Schapiro <schlomo+github@schapiro.org>
@jsmeix
Copy link
Member Author

jsmeix commented Jan 18, 2024

I wanted to test it more but I don't find time for that so
#3091 (comment)
must suffice and I like to merge it tomorrow afternoon
unless there are objections from other @rear/contributors

@jsmeix jsmeix merged commit a1d1b7d into master Jan 22, 2024
19 checks passed
@jsmeix jsmeix deleted the jsmeix-write-protect-only-devnodes branch January 22, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do cleanup enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants