[libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points

[John Ferlan]

On 06/22/2017 12:18 PM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1462060
> 
> When building a qemu namespace we might be dealing with bare
> regular files. Files that live under /dev. For instance
> /dev/my_awesome_disk:
> 
>   <disk type='file' device='disk'>
>     <driver name='qemu' type='qcow2'/>
>     <source file='/dev/my_awesome_disk'/>
>     <target dev='vdc' bus='virtio'/>
>   </disk>
> 
>   # qemu-img create -f qcow2 /dev/my_awesome_disk 10M
> 
> So far we were mknod()-ing them which is
> obviously wrong. We need to touch the file and bind mount it to
> the original:
> 
> 1) touch /var/run/libvirt/qemu/fedora.dev/my_awesome_disk
> 2) mount --bind /dev/my_awesome_disk /var/run/libvirt/qemu/fedora.dev/my_awesome_disk
> 
> Later, when the new /dev is built and replaces original /dev the
> file is going to live at expected location.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 977b5c089..6d7c218a2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7708,6 +7708,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
>      int ret = -1;
>      bool isLink = false;
>      bool isDev = false;
> +    bool isReg = false;
>      bool create = false;
>  #ifdef WITH_SELINUX
>      char *tcon = NULL;
> @@ -7731,6 +7732,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
>  
>      isLink = S_ISLNK(sb.st_mode);
>      isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
> +    isReg = S_ISREG(sb.st_mode);
>  
>      /* Here, @device might be whatever path in the system. We
>       * should create the path in the namespace iff it's "/dev"
> @@ -7842,16 +7844,12 @@ qemuDomainCreateDeviceRecursive(const char *device,
>              }
>              goto cleanup;
>          }
> -
> -        /* Set the file permissions again: mknod() is affected by the
> -         * current umask, and as such might not have set them correctly */
> +    } else if (isReg) {
>          if (create &&
> -            chmod(devicePath, sb.st_mode) < 0) {
> -            virReportSystemError(errno,
> -                                 _("Failed to set permissions for device %s"),
> -                                 devicePath);
> +            virFileTouch(devicePath, sb.st_mode) < 0)
>              goto cleanup;
> -        }
> +        /* Just create the file here so that code below sets
> +         * proper owner and mode. Bind mount only after that. */
>      } else {
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                         _("unsupported device type %s %o"),
> @@ -7871,6 +7869,15 @@ qemuDomainCreateDeviceRecursive(const char *device,
>          goto cleanup;
>      }


> +    /* Symlinks don't have mode */
> +    if (!isLink &&


So the "one" concern I have would be to use (isDev || isReg) instead of
(!isLink) - if only to CYA that something new bool isn't invented that
would also not need the chmod.  IDC, I'm fine with it this way - your
call - just figured I'd point it out.

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

John

> +        chmod(devicePath, sb.st_mode) < 0) {
> +        virReportSystemError(errno,
> +                             _("Failed to set permissions for device %s"),

I'm also OK with printing the permissions "0%o" that failed ;-)

> +                             devicePath);
> +        goto cleanup;
> +    }
> +
>      /* Symlinks don't have ACLs. */
>      if (!isLink &&
>          virFileCopyACLs(device, devicePath) < 0 &&
> @@ -7903,6 +7910,11 @@ qemuDomainCreateDeviceRecursive(const char *device,
>      }
>  #endif
>  
> +    /* Finish mount process started earlier. */
> +    if (isReg &&
> +        virFileBindMountDevice(device, devicePath) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      VIR_FREE(target);
>