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@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 &&
+ 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
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@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@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);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
Thanks.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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:
>>>
>>> <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@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.
>
That's fine - I was 50/50 anyway...
John
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> Thanks.
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.