Changeset
src/lxc/lxc_container.c  |  58 +------------------
src/lxc/lxc_container.h  |   4 ++
src/lxc/lxc_controller.c | 145 +++++++++++++++++++++++++++--------------------
3 files changed, 87 insertions(+), 120 deletions(-)
Git apply log
Switched to a new branch '20180415212539.19974-1-rstoyanov1@gmail.com'
Applying: lxc: Make lxcContainerMountFSBlock non static
Applying: lxc: Move up virLXCControllerAppendNBDPids
Applying: lxc: Mount NBD devices before clone
To https://github.com/patchew-project/libvirt
 + 4ec91a3...81b7686 patchew/20180415212539.19974-1-rstoyanov1@gmail.com -> patchew/20180415212539.19974-1-rstoyanov1@gmail.com (forced update)
Test passed: syntax-check

loading

[libvirt] [RFC 0/3] LXC with block device and enabled userns
Posted by Radostin Stoyanov, 13 weeks ago
Problem background
------------------

The LXC driver has support for the filesystem types "file" and "block"
that allow a disk image to be mounted in the guest (container). [1]

However, when user-namespace is enabled (uid/gid mapping is used) the
mount of the root filesystem block device fails. [2]

According to "man 7 user_namespaces":

	Mounting block-based filesystems can be done only by a process that holds
	CAP_SYS_ADMIN in the initial user namespace.


Suggested approach
------------------
Mount the root file system block device before the clone() call, then set
filesystem type to VIR_DOMAIN_FS_TYPE_MOUNT and filesystem source to the folder
where it was mounted.


Issues encountered
--------------------

This patch series implements the basic idea of the mentioned approach.
In result, a container with configured idmap and NBD filesystem is able to start.

However, on guest shutdown this kernel error [3] occurs.

Similar messages [4] occur on shutdown when NBD filesystem is used with LXC
container without idmap.

Perhaps, one reason could be that on guest shutdown the LXC driver kills qemu-nbd
process without sending disconnect for the specified device.


References
---------- 

[1] https://libvirt.org/formatdomain.html#elementsFilesystems
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1328946
[3] https://pastebin.com/raw/jMBk5mtG
[4] https://pastebin.com/raw/wTKbuRP9

Radostin Stoyanov (3):
  lxc: Make lxcContainerMountFSBlock non static
  lxc: Move up virLXCControllerAppendNBDPids
  lxc: Mount NBD devices before clone

 src/lxc/lxc_container.c  |  58 +------------------
 src/lxc/lxc_container.h  |   4 ++
 src/lxc/lxc_controller.c | 145 +++++++++++++++++++++++++++--------------------
 3 files changed, 87 insertions(+), 120 deletions(-)

-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC 1/3] lxc: Make lxcContainerMountFSBlock non static
Posted by Radostin Stoyanov, 13 weeks ago
---
 src/lxc/lxc_container.c | 5 +----
 src/lxc/lxc_container.h | 4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 532fd0be0..3b8cb966e 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -113,9 +113,6 @@ struct __lxc_child_argv {
     int *nsInheritFDs;
 };
 
-static int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
-                                    const char *srcprefix,
-                                    const char *sec_mount_options);
 
 
 /*
@@ -1499,7 +1496,7 @@ static int lxcContainerMountFSBlockHelper(virDomainFSDefPtr fs,
 }
 
 
-static int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
+int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
                                     const char *srcprefix,
                                     const char *sec_mount_options)
 {
diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
index 641e2d460..3a3564b79 100644
--- a/src/lxc/lxc_container.h
+++ b/src/lxc/lxc_container.h
@@ -68,4 +68,8 @@ int lxcContainerChown(virDomainDefPtr def, const char *path);
 
 bool lxcIsBasicMountLocation(const char *path);
 
+int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
+                             const char *srcprefix,
+                             const char *sec_mount_options);
+
 #endif /* LXC_CONTAINER_H */
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 1/3] lxc: Make lxcContainerMountFSBlock non static
Posted by John Ferlan, 11 weeks ago

On 04/15/2018 05:25 PM, Radostin Stoyanov wrote:
> ---
>  src/lxc/lxc_container.c | 5 +----
>  src/lxc/lxc_container.h | 4 ++++
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 

Hopefully mprivozn can take a look at this series as he knows most about
namespaces...  Not sure if he's just been too busy recently and missed
something referencing namespaces from your cover...

> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 532fd0be0..3b8cb966e 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -113,9 +113,6 @@ struct __lxc_child_argv {
>      int *nsInheritFDs;
>  };
>  
> -static int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
> -                                    const char *srcprefix,
> -                                    const char *sec_mount_options);
>  
>  
>  /*
> @@ -1499,7 +1496,7 @@ static int lxcContainerMountFSBlockHelper(virDomainFSDefPtr fs,
>  }
>  
>  
> -static int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
> +int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
>                                      const char *srcprefix,
>                                      const char *sec_mount_options)

while you're at it since more recent code uses a different syntax:

int
lxcContainerMountFSBlock(virDomainFSDefPtr fs,
                         const char *srcprefix,
                         const char *sec_mount_options)

NB: Alignment of 2nd/3rd args too..

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

>  {
> diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
> index 641e2d460..3a3564b79 100644
> --- a/src/lxc/lxc_container.h
> +++ b/src/lxc/lxc_container.h
> @@ -68,4 +68,8 @@ int lxcContainerChown(virDomainDefPtr def, const char *path);
>  
>  bool lxcIsBasicMountLocation(const char *path);
>  
> +int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
> +                             const char *srcprefix,
> +                             const char *sec_mount_options);
> +
>  #endif /* LXC_CONTAINER_H */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC 2/3] lxc: Move up virLXCControllerAppendNBDPids
Posted by Radostin Stoyanov, 13 weeks ago
There is no functional change in this patch.

It only moves virLXCControllerAppendNBDPids above
virLXCControllerSetupNBDDeviceFS.
---
 src/lxc/lxc_controller.c | 96 ++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 507bffda0..61d9ed07b 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -481,6 +481,55 @@ static int virLXCControllerSetupLoopDeviceDisk(virDomainDiskDefPtr disk)
 }
 
 
+static int virLXCControllerAppendNBDPids(virLXCControllerPtr ctrl,
+                                         const char *dev)
+{
+    char *pidpath = NULL;
+    pid_t *pids = NULL;
+    size_t npids = 0;
+    size_t i;
+    int ret = -1;
+    size_t loops = 0;
+    pid_t pid;
+
+    if (!STRPREFIX(dev, "/dev/") ||
+        virAsprintf(&pidpath, "/sys/devices/virtual/block/%s/pid", dev + 5) < 0)
+        goto cleanup;
+
+    /* Wait for the pid file to appear */
+    while (!virFileExists(pidpath)) {
+        /* wait for 100ms before checking again, but don't do it for ever */
+        if (errno == ENOENT && loops < 10) {
+            usleep(100 * 1000);
+            loops++;
+        } else {
+            virReportSystemError(errno,
+                                 _("Cannot check NBD device %s pid"),
+                                 dev + 5);
+            goto cleanup;
+        }
+    }
+
+    if (virPidFileReadPath(pidpath, &pid) < 0)
+        goto cleanup;
+
+    if (virProcessGetPids(pid, &npids, &pids) < 0)
+        goto cleanup;
+
+    for (i = 0; i < npids; i++) {
+        if (VIR_APPEND_ELEMENT(ctrl->nbdpids, ctrl->nnbdpids, pids[i]) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(pids);
+    VIR_FREE(pidpath);
+    return ret;
+}
+
+
 static int virLXCControllerSetupNBDDeviceFS(virDomainFSDefPtr fs)
 {
     char *dev;
@@ -545,53 +594,6 @@ static int virLXCControllerSetupNBDDeviceDisk(virDomainDiskDefPtr disk)
     return 0;
 }
 
-static int virLXCControllerAppendNBDPids(virLXCControllerPtr ctrl,
-                                         const char *dev)
-{
-    char *pidpath = NULL;
-    pid_t *pids = NULL;
-    size_t npids = 0;
-    size_t i;
-    int ret = -1;
-    size_t loops = 0;
-    pid_t pid;
-
-    if (!STRPREFIX(dev, "/dev/") ||
-        virAsprintf(&pidpath, "/sys/devices/virtual/block/%s/pid", dev + 5) < 0)
-        goto cleanup;
-
-    /* Wait for the pid file to appear */
-    while (!virFileExists(pidpath)) {
-        /* wait for 100ms before checking again, but don't do it for ever */
-        if (errno == ENOENT && loops < 10) {
-            usleep(100 * 1000);
-            loops++;
-        } else {
-            virReportSystemError(errno,
-                                 _("Cannot check NBD device %s pid"),
-                                 dev + 5);
-            goto cleanup;
-        }
-    }
-
-    if (virPidFileReadPath(pidpath, &pid) < 0)
-        goto cleanup;
-
-    if (virProcessGetPids(pid, &npids, &pids) < 0)
-        goto cleanup;
-
-    for (i = 0; i < npids; i++) {
-        if (VIR_APPEND_ELEMENT(ctrl->nbdpids, ctrl->nnbdpids, pids[i]) < 0)
-            goto cleanup;
-    }
-
-    ret = 0;
-
- cleanup:
-    VIR_FREE(pids);
-    VIR_FREE(pidpath);
-    return ret;
-}
 
 static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl)
 {
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 2/3] lxc: Move up virLXCControllerAppendNBDPids
Posted by John Ferlan, 11 weeks ago

On 04/15/2018 05:25 PM, Radostin Stoyanov wrote:
> There is no functional change in this patch.
> 
> It only moves virLXCControllerAppendNBDPids above
> virLXCControllerSetupNBDDeviceFS.
> ---
>  src/lxc/lxc_controller.c | 96 ++++++++++++++++++++++++------------------------
>  1 file changed, 49 insertions(+), 47 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC 3/3] lxc: Mount NBD devices before clone
Posted by Radostin Stoyanov, 13 weeks ago
When user-namespace is enabled we are not allowed
to mount block/NBD devices.

Instead, mount /dev/nbdX to /run/libvirt/lxc/<domain>.root
and set:

	fs->src->path = /run/libvirt/lxc/<domain>.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;
 }
 
-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(&dst, "%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, &stateDir) < 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;
+    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(&dst, "%s/%s.root/",
+                    LXC_STATE_DIR, def->name) < 0)
+        return -1;
+
+    if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, def)))
+        return -1;
+
     if (virFileNBDDeviceAssociate(fs->src->path,
                                   fs->format,
                                   fs->readonly,
                                   &dev) < 0)
         return -1;
 
-    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);
+        return -1;
+    }
+
+    fs->dst = tmp;
+    fs->type = VIR_DOMAIN_FS_TYPE_MOUNT;
+
+    /* The NBD device will be cleaned up while the cgroup will end.
+     * For this we need to remember the qemu-nbd pid and add it to
+     * the cgroup*/
+    if (virLXCControllerAppendNBDPids(ctrl, fs->src->path) < 0)
+
+    VIR_FREE(fs->src->path);
+    fs->src->path = dst;
+
     return 0;
 }
 
@@ -637,13 +660,7 @@ static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl)
             }
             ctrl->loopDevFds[ctrl->nloopDevs - 1] = fd;
         } else if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_NBD) {
-            if (virLXCControllerSetupNBDDeviceFS(fs) < 0)
-                goto cleanup;
-
-            /* The NBD device will be cleaned up while the cgroup will end.
-             * For this we need to remember the qemu-nbd pid and add it to
-             * the cgroup*/
-            if (virLXCControllerAppendNBDPids(ctrl, fs->src->path) < 0)
+            if (virLXCControllerSetupNBDDeviceFS(ctrl, fs) < 0)
                 goto cleanup;
         } else {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-- 
2.14.3

--
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
Posted by John Ferlan, 11 weeks ago

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/<domain>.root
> and set:
> 
> 	fs->src->path = /run/libvirt/lxc/<domain>.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(&dst, "%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, &stateDir) < 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(&dst, "%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,
>                                    &dev) < 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;
> +
> +    /* The NBD device will be cleaned up while the cgroup will end.
> +     * For this we need to remember the qemu-nbd pid and add it to
> +     * the cgroup*/
> +    if (virLXCControllerAppendNBDPids(ctrl, fs->src->path) < 0)

Bad cut-n-paste...

What would we do if < 0?? VIR_FREE(fs->src->path);

otherwise, we'd leak it and just set @dst to fs->src->path

> +
> +    VIR_FREE(fs->src->path);
> +    fs->src->path = dst;
> +

still leaving sec_mount_options leaked.

Perhaps real cleanup type processing should be used instead and an
initialized "int ret = -1;" with the ret = 0 once everything is
successful.  You can use VIR_STEAL_PTR for dst and dev and have cleanup
have the VIR_FREE for dev, dst, and sec_mount_options assuming they are
all initialized to NULL.

>      return 0;
>  }
>  
> @@ -637,13 +660,7 @@ static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl)
>              }
>              ctrl->loopDevFds[ctrl->nloopDevs - 1] = fd;
>          } else if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_NBD) {
> -            if (virLXCControllerSetupNBDDeviceFS(fs) < 0)
> -                goto cleanup;
> -
> -            /* The NBD device will be cleaned up while the cgroup will end.
> -             * For this we need to remember the qemu-nbd pid and add it to
> -             * the cgroup*/
> -            if (virLXCControllerAppendNBDPids(ctrl, fs->src->path) < 0)
> +            if (virLXCControllerSetupNBDDeviceFS(ctrl, fs) < 0)
>                  goto cleanup;

Perhaps a patch between this and the last one to move the call to
virLXCControllerAppendNBDPids into virLXCControllerSetupNBDDeviceFS
would be appropriate and return -1 instead of cleanup just before return
0.

That may make this followup patch a bit easier to follow.

John

>          } else {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> 

--
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
Posted by Michal Privoznik, 9 weeks ago
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/<domain>.root
>> and set:
>>
>> 	fs->src->path = /run/libvirt/lxc/<domain>.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