Update manager.inc to allow mount of labeled filesystems #543
Conversation
1) Reordered checks by specicifity is_devicefile will catch anything in /dev and its subdirectories, so I think it should be done last. (Sidenote: Maybe you wanted ^\/dev\/[^\/]+$ in functions.inc?) 2) Label will never match the full /dev/disk/by-label path, so I propose to match only the string after the last slash. (This might be beneficial for the uuid part, too, but I didn't make this change here.) 3) The final else statement only handled the "label" case (and didn't in case the label wasn't on its own ;-) making it unnecessary. The proposed change should handle isolated labels as well as labels with path, because labels can't contain slashes afaik. 4) I didn't use else-if constructs, because the label case has a sub condition with a regex match and I didn't know how to do that as an && in the first condition. To keep things consistent used !found in all conditions except obviously the first. I've never coded in PHP before, so please forgive me if it looks clumsy.
|
|
|
Can you add some example device names in the code to make it more clear what happens here. Please also add a description why this PR is necessary, currently i do not understand the intention. Please add the signed-off-by line ( |
|
Mount button threw exception with this device: |
|
Both is_fs_uuid and is_devicefile_by in functions.inc are more specific. |
|
How can I reproduce this? I never saw that behavior and the code is really old there. |
|
The only thing I did was create a volume with label, in this case |
|
But the fs really doesn't matter. |
|
|
|
This could be changed by using this instead: |
That it the intention of this function to check if it is a device path, and that also includes /dev/disk/by... paths. Changing this will break everything in OMV, so this would be a really bad idea. |
|
OK, but in this case, the label case can never be reached. |
|
Please post the output of blkid and the dir tree of the device, e.g. /dev/... I can not merge this PR before I fully can reproduce this, otherwise it might break other installations. |
|
I can see that. |
|
label is a special case of dev imo |
|
I need to check the code, but it works for me in the past and there were no bug reports related this till now. I assume it has something to do with BTRFS. I think a device by path link should be resolved to a canonical path when using realpath, so a previous code path should be executed in this case. But as said, I need to check that. |
|
functions.inc condition |
|
I really get that. |
|
A workaround would be to use one of the constituent drives instead of the label, but that didn't jive with me. |
|
|
|
|
|
On the topic of canonical file paths, why not make the (unchanging) device ids or wwns or the (editable) labels the main anchor instead of the almost random /dev/sd* device order? I realize you're on the same page as the kernel there, and you can't argue with the kernel, so these boots are made for walking. Devices keep changin' when they ought to be samin' and some of these days these boots are walking all over us. :-) Happy Holidays! |
Because of that OMV prefers predictable device names for storing in the database and for configuration files, but canonical device names are the best to work at runtime because they are simple. |
|
I did some quickie tests with unmodded an modified filesystem/manager.inc: Scenarios: All these scenarios where performed with the original and with the modified version of filesystem/manager.inc, with poweroff and new boot between changes of this file. For each scenario, the disk on which the test was performed was initialised with 8MB of zeros, then partitioned or not as required by the scenario and a new filesystem created on the cli. Where required, the filesystem was then labeled. More details in the next post. |
|
Full version of the tests attached. |
|
You still have not described the intention of the PR. Could you please do that? I'm now more irritated when i look into your attached file. I can see a Salt stack trace? Are you trying to fix that? If yes, why do you think this can be fixed in the PHP backend code that is not related to the Python code used by Salt? You have raised more question than you've answered. |
I'm sorry, i can really get no information out of this. Please explain the problem and how to reproduce it. What i know out of the current information is:
I need:
|
|
Can not reproduce this behavior.
|
|
I've checked the original code again and IMO you didn't have understood it correct. The
IMO the current implementation is correct and the problem comes from elsewhere. |
|
I will close this PR because i do not see a problem here. We should open a bug report instead, describing the problem, providing all available information and see how to fix the issue. |
|
Thanks, I'll do that. |
|
Screenshots from unmodded fresh install. |
|
Please post the output of Please, no screenshots, the console supports copy & paste (without loosing the format). |
|
root@openmediavault: |
Signed-off-by: Volker Theile <votdev@gmx.de>
Signed-off-by: Volker Theile <votdev@gmx.de>
Signed-off-by: Volker Theile <votdev@gmx.de>
|
The real issue here is way more complicated than you tried to fix with your PR. BTRFS has a strange behaviour with canonical device files and what the kernel reports in the mount table. |
|
Oh boy. |
|
BTW: Thanks for the fast reaction! |
Signed-off-by: Volker Theile <votdev@gmx.de>
Signed-off-by: Volker Theile <votdev@gmx.de>
Signed-off-by: Volker Theile <votdev@gmx.de> (cherry picked from commit f2a80d8)
is_devicefile will catch anything in /dev and its subdirectories, so I think it should be done last. (Sidenote: Maybe you wanted ^/dev/[^\/]+$ in functions.inc?)
I've never coded in PHP before, so please forgive me if it looks clumsy.