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

Allow full path in lxc.devices.allow #1708

Merged
merged 1 commit into from Jul 29, 2017
Merged

Allow full path in lxc.devices.allow #1708

merged 1 commit into from Jul 29, 2017

Conversation

aeris
Copy link
Contributor

@aeris aeris commented Jul 19, 2017

Some devices like LVM or cryptsetup entries have no stable major/minor, changing between host reboots.
In this case, hardcoded numbers are not usable in config file and there is currently no way to use hook with lxc-device to do the link at guest startup :

* `pre-start`/`autodev` hook runs in host context but has the guest in stopped state and so lxc-device not usable
* `start` hook is in running state but runs in guest context and so lxc-device not available

This patch converts fullpath in lxc.devices.allow to current major/minor numbers to address those changing numbers.

Signed-off-by: aeris aeris@imirhil.fr

@lxc-jenkins
Copy link

This pull request didn't trigger Jenkins as its author isn't in the whitelist.

An organization member must perform one of the following:

  • To have this branch tested by Jenkins, use the "ok to test" command.
  • To have a one time test done, use the "test this please" command.

Those commands are simple Github comments of the format: "jenkins: COMMAND"

Copy link
Member

@hallyn hallyn left a comment

Choose a reason for hiding this comment

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

Not clear what your goal is here. Please show exactly what an example input would look like, and explain what you're doing with it.

free(parts);
}
} else {
strcpy(converted_value, value);
Copy link
Member

Choose a reason for hiding this comment

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

You never use converted_value anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed a changed chunk during commit. Fixed.

}

if (!snprintf(converted_value, 50, "%c %lu:%lu %s", type,
MAJOR(dev), MINOR(dev), mode)) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to do something if this snprintf fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just a trailing test. Removed.

@aeris
Copy link
Contributor Author

aeris commented Jul 20, 2017

The goal of this patch it to transform lxc.devices.allow /dev/sda rwm to lxc.devices.allow b 8:0 rwm on the fly, to allow full path in lxc.devices.allow. With lxc.mount.entry, it allows to mount devices with dynamic major/minor, like LVM or cryptsetup mapper, not stable between reboot.

For example, currently this is not possible:

lxc.devices.allow = b 254 3 rwm
lxc.mount.entry = /dev/system/data dev/system/data none bind,create=file

Because /dev/system/data is a LV and its minor sometime changes between reboot.
With this patch, I can do

lxc.devices.allow = /dev/system/data rwm
lxc.mount.entry = /dev/system/data dev/system/data none bind,create=file

@hallyn
Copy link
Member

hallyn commented Jul 24, 2017

This patch cannot be doing what you want. You write the result into a new variable 'converted_value', which you do not later use.

@aeris
Copy link
Contributor Author

aeris commented Jul 24, 2017

See the missing commit chunk added after the initial commit 8d4f36e

@hallyn
Copy link
Member

hallyn commented Jul 24, 2017 via email

@aeris
Copy link
Contributor Author

aeris commented Jul 25, 2017

I add more tests on e7a51c5

@aeris
Copy link
Contributor Author

aeris commented Jul 25, 2017

Here we go ! feed9c7

@hallyn
Copy link
Member

hallyn commented Jul 25, 2017

No, it should be (ret < 0 || ret >= 50)

@hallyn
Copy link
Member

hallyn commented Jul 25, 2017

And then please squash them all into one commit, and I'll apply. Thanks!

@aeris
Copy link
Contributor Author

aeris commented Jul 25, 2017

I missred the snprintf doc…
Fixed and squashed into a432e86

@aeris aeris force-pushed the master branch 2 times, most recently from ff5a3c4 to a432e86 Compare Jul 25, 2017
@hallyn
Copy link
Member

hallyn commented Jul 26, 2017

I'm sorry, I noticed two more things.

First, you are mallocing to_split twice.

Secondly, you are not always checking that malloc and strdup succeeded.

Please fix those, or feel free at this point to tell me you prefer I just fix those and I merge.

Some devices like LVM or cryptsetup entries have no stable major/minor, changing between host reboots.
In this case, hardcoded numbers are not usable in config file and there is currently no way to use hook with lxc-device to do the link at guest startup :

    * `pre-start`/`autodev` hook runs in host context but has the guest in stopped state and so lxc-device not usable
    * `start` hook is in running state but runs in guest context and so lxc-device not available

This patch converts fullpath in lxc.devices.allow to current major/minor numbers to address those changing numbers.

Signed-off-by: aeris <aeris@imirhil.fr>
@aeris
Copy link
Contributor Author

aeris commented Jul 26, 2017

to_split must be duplicated, strtok_r modifies its first argument, so reusing the first value leads to segfault.
For malloc/strdup, I add more tests.

@hallyn hallyn merged commit da7a897 into lxc:master Jul 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants