On 06/22/2017 12:18 PM, Michal Privoznik wrote:

It comes very handy to have source path for chardevs. We already have such function: virDomainAuditChardevPath() but it's static and has name not suitable for exposing.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_audit.c | 44 ++++++-------------------------------------- src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 4 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 1e667af73..484420a21 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -68,38 +68,6 @@ virDomainAuditGetRdev(const char *path ATTRIBUTE_UNUSED) #endif

-static const char * -virDomainAuditChardevPath(virDomainChrSourceDefPtr chr) -{ - if (!chr) - return NULL; - - switch ((virDomainChrType) chr->type) { - case VIR_DOMAIN_CHR_TYPE_PTY: - case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_FILE: - case VIR_DOMAIN_CHR_TYPE_PIPE: - case VIR_DOMAIN_CHR_TYPE_NMDM: - return chr->data.file.path; - - case VIR_DOMAIN_CHR_TYPE_UNIX: - return chr->data.nix.path; - - case VIR_DOMAIN_CHR_TYPE_TCP: - case VIR_DOMAIN_CHR_TYPE_UDP: - case VIR_DOMAIN_CHR_TYPE_NULL: - case VIR_DOMAIN_CHR_TYPE_VC: - case VIR_DOMAIN_CHR_TYPE_STDIO: - case VIR_DOMAIN_CHR_TYPE_SPICEVMC: - case VIR_DOMAIN_CHR_TYPE_SPICEPORT: - case VIR_DOMAIN_CHR_TYPE_LAST: - return NULL; - } - - return NULL; -} - - static void virDomainAuditGenericDev(virDomainObjPtr vm, const char *type, @@ -178,8 +146,8 @@ virDomainAuditChardev(virDomainObjPtr vm, newsrc = newDef->source;

virDomainAuditGenericDev(vm, "chardev", - virDomainAuditChardevPath(oldsrc), - virDomainAuditChardevPath(newsrc), + virDomainChrSourceDefPath(oldsrc), + virDomainChrSourceDefPath(newsrc), reason, success); }

@@ -218,7 +186,7 @@ virDomainAuditSmartcard(virDomainObjPtr vm,

case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: virDomainAuditGenericDev(vm, "smartcard", NULL, - virDomainAuditChardevPath(def->data.passthru), + virDomainChrSourceDefPath(def->data.passthru), reason, success); break;

@@ -264,7 +232,7 @@ virDomainAuditRNG(virDomainObjPtr vm, break;

case VIR_DOMAIN_RNG_BACKEND_EGD: - newsrcpath = virDomainAuditChardevPath(newDef->source.chardev); + newsrcpath = virDomainChrSourceDefPath(newDef->source.chardev); break;

case VIR_DOMAIN_RNG_BACKEND_LAST: @@ -279,7 +247,7 @@ virDomainAuditRNG(virDomainObjPtr vm, break;

case VIR_DOMAIN_RNG_BACKEND_EGD: - oldsrcpath = virDomainAuditChardevPath(oldDef->source.chardev); + oldsrcpath = virDomainChrSourceDefPath(oldDef->source.chardev); break;

case VIR_DOMAIN_RNG_BACKEND_LAST: @@ -982,7 +950,7 @@ virDomainAuditShmem(virDomainObjPtr vm, { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname = virAuditEncode("vm", vm->def->name); - const char *srcpath = virDomainAuditChardevPath(&def->server.chr); + const char *srcpath = virDomainChrSourceDefPath(&def->server.chr); const char *virt = virDomainVirtTypeToString(vm->def->virtType); char *shmpath = NULL;

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0409c62ef..2cbe96b6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2034,6 +2034,39 @@ virDomainNetDefFree(virDomainNetDefPtr def) VIR_FREE(def); }

+ +const char * +virDomainChrSourceDefPath(virDomainChrSourceDefPtr chr)

This should be an action function... By action I mean one of: virDomainChrGetSourceDefPath virDomainChrSourceGetDefPath virDomainChrSourceDefGetPath virDomainChrSourceDefPathGet

+{ + if (!chr) + return NULL; + + switch ((virDomainChrType) chr->type) { + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_NMDM: + return chr->data.file.path; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + return chr->data.nix.path; + + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + case VIR_DOMAIN_CHR_TYPE_LAST: + return NULL; + } + + return NULL; +} + + void ATTRIBUTE_NONNULL(1) virDomainChrSourceDefClear(virDomainChrSourceDefPtr def) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6d9ee9787..51b830917 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3256,6 +3256,8 @@ int virDomainDefFindDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, bool reportError);

+const char *virDomainChrSourceDefPath(virDomainChrSourceDefPtr chr); + void virDomainChrSourceDefClear(virDomainChrSourceDefPtr def);

Sigh... Which is more correct @chr or @def? As long as the API name has the Get: Reviewed-by: John Ferlan <jferlan@redhat.com> John

char *virDomainObjGetMetadata(virDomainObjPtr vm, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1e9471c5..f671da9d5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -208,6 +208,7 @@ virDomainChrSerialTargetTypeToString; virDomainChrSourceDefClear; virDomainChrSourceDefCopy; virDomainChrSourceDefFree; +virDomainChrSourceDefPath; virDomainChrSpicevmcTypeFromString; virDomainChrSpicevmcTypeToString; virDomainChrTcpProtocolTypeFromString;

On 06/22/2017 12:18 PM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1459592

In 290a00e41d I've tried to fix the process of building a qemu namespace when dealing with file mount points. What I haven't realized then is that we might be dealing not with just regular files but also special files (like sockets). Indeed, try the following:

1) socat unix-listen:/tmp/soket stdio 2) touch /dev/socket 3) mount --bind /tmp/socket /dev/socket 4) virsh start anyDomain

Problem with my previous approach is that I wasn't creating the temporary location (where mount points under /dev are moved) for anything but directories and regular files.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e7404da6..212717c80 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, goto cleanup; }

- /* At this point, devMountsPath is either a regular file or a directory. */ + /* At this point, devMountsPath is either: + * a file (regular or special), or + * a directory. */ if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) || - (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) { + (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {

It would seem to me that this would open Pandora's box to all different types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of which it may not be so popular to perform a touch on. I think you should keep it specific... Perhaps use the list from qemuDomainCreateDeviceRecursive: isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode); John (FWIW: I used a cscope search on S_ISSOCK...)

virReportSystemError(errno, _("Failed to create %s"), devMountsSavePath[i]);

On 06/27/2017 11:37 PM, John Ferlan wrote:

On 06/22/2017 12:18 PM, Michal Privoznik wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1459592

In 290a00e41d I've tried to fix the process of building a qemu namespace when dealing with file mount points. What I haven't realized then is that we might be dealing not with just regular files but also special files (like sockets). Indeed, try the following:

1) socat unix-listen:/tmp/soket stdio 2) touch /dev/socket 3) mount --bind /tmp/socket /dev/socket 4) virsh start anyDomain

Problem with my previous approach is that I wasn't creating the temporary location (where mount points under /dev are moved) for anything but directories and regular files.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e7404da6..212717c80 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, goto cleanup; }

- /* At this point, devMountsPath is either a regular file or a directory. */ + /* At this point, devMountsPath is either: + * a file (regular or special), or + * a directory. */ if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) || - (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) { + (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {

It would seem to me that this would open Pandora's box to all different types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of which it may not be so popular to perform a touch on.

I think you should keep it specific... Perhaps use the list from qemuDomainCreateDeviceRecursive:

isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode);

Not really. mount --bind makes the target to be the correct type. IOW: # create a regular file touch /tmp/blah # here, assume /source/socket is a socket mount --bind /source/socket /tmp/blah # now, /tmp/blah will be type of socket too The only problem here is that for file based 'devices' (or things in general) we have to create the file whereas for directories we have to create directories. Just like in the example. BTW, what type of file are NAM, MPB, MPC, NWK? Michal

On 06/27/2017 11:40 PM, John Ferlan wrote:

On 06/22/2017 12:18 PM, Michal Privoznik wrote:

...

This function is going to be used on other places, so instead of copying code we can just call the function.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 212717c80..286d60761 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7572,6 +7572,41 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, }

/* Returns allocated string on success which the caller must free and NULL on failure */

Okay, I've extended this to: /** * qemuDomainGetPreservedMountPath: * @cfg: driver configuration data * @vm: domain object * @mount: mount point path to convert * * For given @mount point return new path where the mount point * should be moved temporarily whilst building the namespace. * * Returns: allocated string on success which the caller must free, * NULL on failure. */

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

Thanks. Michal

On 06/22/2017 12:18 PM, Michal Privoznik wrote:

Currently, we silently assume that file we are creating in the namespace is either a link or a device (character or block one). This is not always the case. Therefore instead of doing something wrong, claim about unsupported file type.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)

Similar comment about print style for octal value %o Reviewed-by: John Ferlan <jferlan@redhat.com> John

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e6fb041de..977b5c089 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8518,6 +8518,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, int ret = -1; bool delDevice = false; bool isLink = S_ISLNK(data->sb.st_mode); + bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);

qemuSecurityPostFork(data->driver->securityManager);

@@ -8539,7 +8540,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, } else { delDevice = true; } - } else { + } else if (isDev) { VIR_DEBUG("Creating dev %s (%d,%d)", data->file, major(data->sb.st_rdev), minor(data->sb.st_rdev)); if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0) { @@ -8556,6 +8557,11 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, } else { delDevice = true; } + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported device type %s %o"), ^^

s/%o/0%o

+ data->file, (int) data->sb.st_mode); + goto cleanup; }

if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {

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

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

On 06/22/2017 12:18 PM, Michal Privoznik wrote:

Currently, the only type of chardev that we create the backend for in the namespace is type='dev'. This is not enough, other backends might have files under /dev too. For instance channels might have a unix socket under /dev (well, bind mounted under /dev from a different place).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 51779c535..65eec26dd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7732,7 +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); + isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode);

/* Here, @device might be whatever path in the system. We * should create the path in the namespace iff it's "/dev" @@ -8129,11 +8129,17 @@ qemuDomainSetupChardev(virDomainDefPtr def ATTRIBUTE_UNUSED, void *opaque) { const struct qemuDomainCreateDeviceData *data = opaque; + const char *path = NULL;

- if (dev->source->type != VIR_DOMAIN_CHR_TYPE_DEV) + if (!(path = virDomainChrSourceDefPath(dev->source))) return 0;

- return qemuDomainCreateDevice(dev->source->data.file.path, data, false); + /* Socket created by qemu. It doesn't exist upfront. */ + if (dev->source->type == VIR_DOMAIN_CHR_TYPE_UNIX && + dev->source->data.nix.listen) + return 0; + + return qemuDomainCreateDevice(path, data, true); }

@@ -8531,7 +8537,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, bool delDevice = false; bool isLink = S_ISLNK(data->sb.st_mode); bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode); - bool isReg = S_ISREG(data->sb.st_mode); + bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) || S_ISSOCK(data->sb.st_mode);

qemuSecurityPostFork(data->driver->securityManager);

@@ -8576,7 +8582,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, * as its backing chain. This might however clash here. * Therefore do the cleanup here. */ if (umount(data->file) < 0 && - errno != ENOENT) { + errno != ENOENT && errno != EINVAL) {

Is this something that should be separate or as a result of the current changes? Should it go with the previous patch since that's where it's introduced? Reviewed-by: John Ferlan <jferlan@redhat.com> John

virReportSystemError(errno, _("Unable to umount %s"), data->file); @@ -8685,7 +8691,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, }

isLink = S_ISLNK(data.sb.st_mode); - isReg = S_ISREG(data.sb.st_mode); + isReg = S_ISREG(data.sb.st_mode) || S_ISFIFO(data.sb.st_mode) || S_ISSOCK(data.sb.st_mode);

if (isReg && STRPREFIX(file, DEVPREFIX)) { cfg = virQEMUDriverGetConfig(driver); @@ -9077,10 +9083,13 @@ qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver, if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0;

- if (chr->source->type != VIR_DOMAIN_CHR_TYPE_DEV) + if (!(path = virDomainChrSourceDefPath(chr->source))) return 0;

- path = chr->source->data.file.path; + /* Socket created by qemu. It doesn't exist upfront. */ + if (chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX && + chr->source->data.nix.listen) + return 0;

cfg = virQEMUDriverGetConfig(driver); if (qemuDomainGetPreservedMounts(cfg, vm,