Re: [libvirt] [RFC 3/3] lxc: Mount NBD devices before clone

2018-05-09 Thread Michal Privoznik
On 04/26/2018 08:09 PM, John Ferlan wrote:
> 
> 
> On 04/15/2018 05:25 PM, Radostin Stoyanov wrote:
>> When user-namespace is enabled we are not allowed
>> to mount block/NBD devices.
>>
>> Instead, mount /dev/nbdX to /run/libvirt/lxc/.root
>> and set:
>>
>>  fs->src->path = /run/libvirt/lxc/.root
>>  fs->type = VIR_DOMAIN_FS_TYPE_MOUNT
>> ---
>>  src/lxc/lxc_container.c  | 53 
>> 
>>  src/lxc/lxc_controller.c | 49 +---
>>  2 files changed, 33 insertions(+), 69 deletions(-)

As John points out, couple of memleaks and also explanation why you're
removing PrepareRoot()?

Otherwise looking good.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [RFC 3/3] lxc: Mount NBD devices before clone

2018-04-26 Thread John Ferlan


On 04/15/2018 05:25 PM, Radostin Stoyanov wrote:
> When user-namespace is enabled we are not allowed
> to mount block/NBD devices.
> 
> Instead, mount /dev/nbdX to /run/libvirt/lxc/.root
> and set:
> 
>   fs->src->path = /run/libvirt/lxc/.root
>   fs->type = VIR_DOMAIN_FS_TYPE_MOUNT
> ---
>  src/lxc/lxc_container.c  | 53 
> 
>  src/lxc/lxc_controller.c | 49 +---
>  2 files changed, 33 insertions(+), 69 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 3b8cb966e..420bb20ab 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -658,55 +658,6 @@ static int lxcContainerResolveSymlinks(virDomainFSDefPtr 
> fs, bool gentle)
>  return 0;
>  }
>  

Why is this being removed?  Not clear from commit message...

> -static int lxcContainerPrepareRoot(virDomainDefPtr def,
> -   virDomainFSDefPtr root,
> -   const char *sec_mount_options)
> -{
> -char *dst;
> -char *tmp;
> -
> -VIR_DEBUG("Prepare root %d", root->type);
> -
> -if (root->type == VIR_DOMAIN_FS_TYPE_MOUNT)
> -return 0;
> -
> -if (root->type == VIR_DOMAIN_FS_TYPE_FILE) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Unexpected root filesystem without loop device"));
> -return -1;
> -}
> -
> -if (root->type != VIR_DOMAIN_FS_TYPE_BLOCK) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("Unsupported root filesystem type %s"),
> -   virDomainFSTypeToString(root->type));
> -return -1;
> -}
> -
> -if (lxcContainerResolveSymlinks(root, false) < 0)
> -return -1;
> -
> -if (virAsprintf(, "%s/%s.root",
> -LXC_STATE_DIR, def->name) < 0)
> -return -1;
> -
> -tmp = root->dst;
> -root->dst = dst;
> -
> -if (lxcContainerMountFSBlock(root, "", sec_mount_options) < 0) {
> -root->dst = tmp;
> -VIR_FREE(dst);
> -return -1;
> -}
> -
> -root->dst = tmp;
> -root->type = VIR_DOMAIN_FS_TYPE_MOUNT;
> -VIR_FREE(root->src->path);
> -root->src->path = dst;
> -
> -return 0;
> -}
> -
>  static int lxcContainerPivotRoot(virDomainFSDefPtr root)
>  {
>  int ret;
> @@ -1755,10 +1706,6 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr 
> vmDef,
>  if (virFileResolveAllLinks(LXC_STATE_DIR, ) < 0)
>  goto cleanup;
>  
> -/* Ensure the root filesystem is mounted */
> -if (lxcContainerPrepareRoot(vmDef, root, sec_mount_options) < 0)
> -goto cleanup;
> -
>  /* Gives us a private root, leaving all parent OS mounts on /.oldroot */
>  if (lxcContainerPivotRoot(root) < 0)
>  goto cleanup;
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 61d9ed07b..d1ae60b1d 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -530,9 +530,12 @@ static int 
> virLXCControllerAppendNBDPids(virLXCControllerPtr ctrl,
>  }
>  
>  
> -static int virLXCControllerSetupNBDDeviceFS(virDomainFSDefPtr fs)
> +static int virLXCControllerSetupNBDDeviceFS(virLXCControllerPtr ctrl,
> +virDomainFSDefPtr fs)
>  {
> -char *dev;
> +char *dev, *dst, *tmp, *sec_mount_options;

There are those that prefer one per line.

> +virDomainDefPtr def = ctrl->def;
> +virSecurityManagerPtr securityDriver = ctrl->securityManager;
>  
>  if (fs->format <= VIR_STORAGE_FILE_NONE) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -540,22 +543,42 @@ static int 
> virLXCControllerSetupNBDDeviceFS(virDomainFSDefPtr fs)
>  return -1;
>  }
>  
> +if (virAsprintf(, "%s/%s.root/",
> +LXC_STATE_DIR, def->name) < 0)
> +return -1;
> +
> +if (!(sec_mount_options = 
> virSecurityManagerGetMountOptions(securityDriver, def)))
> +return -1;

This would leak dst

> +
>  if (virFileNBDDeviceAssociate(fs->src->path,
>fs->format,
>fs->readonly,
>) < 0)
>  return -1;

This would leak dst, sec_mount_options

>  
> -VIR_DEBUG("Changing fs %s to use type=block for dev %s",
> -  fs->src->path, dev);
> -/*
> - * We now change it into a block device type, so that
> - * the rest of container setup 'just works'
> - */
> -fs->type = VIR_DOMAIN_FS_TYPE_BLOCK;
>  VIR_FREE(fs->src->path);
>  fs->src->path = dev;
>  
> +tmp = fs->dst;
> +fs->dst = dst;
> +
> +if (lxcContainerMountFSBlock(fs, "", sec_mount_options) < 0) {
> +fs->dst = tmp;
> +VIR_FREE(dst);

This would leak sec_mount_options

> +return -1;
> +}
> +
> +fs->dst = tmp;
> +fs->type = VIR_DOMAIN_FS_TYPE_MOUNT;