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

2017-07-10 Thread John Ferlan


On 07/10/2017 09:33 AM, Michal Privoznik wrote:
> 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:
>>>
>>>   
>>> 
>>> 
>>> 
>>>   
>>>
>>>   # 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 
>>> ---
>>>  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.
> 

That's fine - I was 50/50 anyway...

John

>>
>> Reviewed-by: John Ferlan 
> 
> Thanks.
> 
> Michal
> 

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


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

2017-07-10 Thread 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:
>>
>>   
>> 
>> 
>> 
>>   
>>
>>   # 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 
>> ---
>>  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 

Thanks.

Michal

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


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

2017-06-27 Thread 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:
> 
>   
> 
> 
> 
>   
> 
>   # 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 
> ---
>  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 

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);
> 

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


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

2017-06-22 Thread Michal Privoznik
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:

  



  

  # 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 
---
 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 &&
+chmod(devicePath, sb.st_mode) < 0) {
+virReportSystemError(errno,
+ _("Failed to set permissions for device %s"),
+ 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);
-- 
2.13.0

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