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

[Michal Privoznik]

On 06/28/2017 12:11 AM, John Ferlan wrote:
> 
> 
> 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.

Funny, I didn't want to use isDev || isReg for exactly this reason. When
new type is introduced nothing needs to be adjusted here. The new type
is more likely to support mode - frankly so far symlinks are the only
type that I've met that doesn't have mode. Therefore I'd like to keep as is.

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

Thanks.

Michal