[pve-devel] [PATCH v2 container 1/9] tools: add can_use_new_mount_api helper

[Wolfgang Bumiller]

On Wed, Nov 13, 2019 at 02:46:57PM +0100, Thomas Lamprecht wrote:
> On 11/13/19 1:30 PM, Oguz Bektas wrote:
> > hi,
> > 
> > On Wed, Nov 13, 2019 at 10:33:11AM +0100, Wolfgang Bumiller wrote:
> >> Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> >> ---
> >> New patch
> >>
> >>  src/PVE/LXC/Tools.pm | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> diff --git a/src/PVE/LXC/Tools.pm b/src/PVE/LXC/Tools.pm
> >> index bebd7d8..0256b6a 100644
> >> --- a/src/PVE/LXC/Tools.pm
> >> +++ b/src/PVE/LXC/Tools.pm
> >> @@ -2,6 +2,8 @@
> >>  
> >>  package PVE::LXC::Tools;
> >>  
> >> +use Errno qw(ENOSYS);
> >> +
> >>  use PVE::SafeSyslog;
> >>  
> >>  # LXC introduced an `lxc.hook.version` property which allows hooks to be executed in different
> >> @@ -130,4 +132,20 @@ sub cgroup_do_write($$) {
> >>      return 1;
> >>  }
> >>  
> >> +# Check whether the kernel supports the new mount api. This is used in the pre-start hook and in
> >> +# the hotplugging code.
> >> +my $cached_can_use_new_mount_api = undef;
> >> +sub can_use_new_mount_api() {
> > 
> > i don't like these names.. isn't can_use_new_mount_api() a bit too
> > vague? "new" is also relative, it won't be "new" in a few releases
> > 
> > maybe something like can_hotplug_mountpoint() would be better,
> > describing what it checks in a more verbose way?
> 
> can_hotplug is not really better, as it got the direction wrong.

That was my main point, what the function actually does has nothing to
do with hotplugging. In fact, strictly speaking it's not even a
requirement, as even with the old api we *could* potentially do mount
point hotplugging. It just involves a little more black magic and
ritualistic sacrifices.

> Just because we need fsopen for the way Wolfgang implemnts CT mountpoint

I actually ended up only using move_mount(), but it's part of the same
change set in the kernel. (But there's a good chance we may want to use
a few more in the future. Although then we probably won't be needing the
early check anymore.) Anyway, I suppose I can use move_mount(-1, NULL,
-1, NULL) instead, too ;-)

> hotplug, it does not means that fsopen can only used for that in the
> future. You tend to call checks for the thing you want to do if they
> succeed, but most of the time they should be called for what they really
> check, if that is a combined check to see if hotplug is possible that name
> could be OK, but that isn't such a method.
> 
> something in the form of "can_fsopen_syscall" would work better, IMO.
> 
> Also, we may want to have a "can do syscall X" check in common?

I've mostly decided against that because normally I'd rather just "do
it" and then deal with the error after the fact. In the mount case
however my main concern was that it's just a lot cheaper than going
through the entire mount process (potentially involving network access,
creating directories etc., switching namespaces...) just to fail at the
very end.