11:18 a.m.
I've encountered couple of problems while playing with the namespaces. For the
first issue (2/8) I've got a confirmation from openstack guys that it fixes
their issue.
Michal Privoznik (8):
conf: Rename and expose virDomainChrSourceDefPath
qemuDomainBuildNamespace: Handle special file mount points
qemu: Move preserved mount points path generation into a separate
function
qemuDomainCreateDeviceRecursive: Fail on unsupported file type
qemuDomainAttachDeviceMknodHelper: Fail on unsupported file type
qemuDomainCreateDeviceRecursive: Support file mount points
qemuDomainAttachDeviceMknodRecursive: Support file mount points
qemu ns: Create chardev backends more frequently
src/conf/domain_audit.c | 44 ++---------
src/conf/domain_conf.c | 33 +++++++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 186 ++++++++++++++++++++++++++++++++++-------------
5 files changed, 178 insertions(+), 88 deletions(-)
--
2.13.0
11:18 a.m.
New subject: [libvirt] [PATCH 1/8] conf: Rename and expose virDomainChrSourceDefPath
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(a)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)
+{
+ 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);
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;
--
2.13.0
4 p.m.
New subject: [libvirt] [PATCH 1/8] conf: Rename and expose virDomainChrSourceDefPath
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(a)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(a)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;
11:18 a.m.
New subject: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
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(a)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)) {
virReportSystemError(errno,
_("Failed to create %s"),
devMountsSavePath[i]);
--
2.13.0
4:37 p.m.
New subject: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
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(a)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]);
4:47 p.m.
New subject: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
On 06/27/2017 05: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(a)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
I guess it's obvious now that I've actually paged in patches 4-8 that I
used the same sandbox I'm reviewing in order to make this comment!
I think the same concern applies though...
John
(FWIW: I used a cscope search on S_ISSOCK...)
> virReportSystemError(errno,
> _("Failed to create %s"),
> devMountsSavePath[i]);
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:14 a.m.
New subject: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
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(a)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
5:05 a.m.
New subject: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
On 07/11/2017 01:14 AM, Michal Privoznik wrote:
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(a)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:
OK - I wasn't sure what all those other things were and going with
!IS_DIR just seemed to open the door to unsurety for me. Since there
was other code that was more restrictive to decide "ifReg", I figured
using that same list would be fine, but I'm not sure if I ever check
where in the scheme of the path the other check is make.
If you're fine with how things are, then fine consider it...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
# 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
11:18 a.m.
New subject: [libvirt] [PATCH 3/8] qemu: Move preserved mount points path generation into a separate function
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(a)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,
}
+static char *
+qemuDomainGetPreservedMountPath(virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr vm,
+ const char *mount)
+{
+ char *path = NULL;
+ char *tmp;
+ const char *suffix = mount + strlen(DEVPREFIX);
+ size_t off;
+
+ if (STREQ(mount, "/dev"))
+ suffix = "dev";
+
+ if (virAsprintf(&path, "%s/%s.%s",
+ cfg->stateDir, vm->def->name, suffix) < 0)
+ return NULL;
+
+ /* Now consider that @mount is "/dev/blah/blah2".
+ * @suffix then points to "blah/blah2". However, caller
+ * expects all the @paths to be the same depth. The
+ * caller doesn't always do `mkdir -p` but sometimes bare
+ * `touch`. Therefore fix all the suffixes. */
+ off = strlen(path) - strlen(suffix);
+
+ tmp = path + off;
+ while (*tmp) {
+ if (*tmp == '/')
+ *tmp = '.';
+ tmp++;
+ }
+
+ return path;
+}
+
+
/**
* qemuDomainGetPreservedMounts:
*
@@ -7628,30 +7663,8 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
goto error;
for (i = 0; i < nmounts; i++) {
- char *tmp;
- const char *suffix = mounts[i] + strlen(DEVPREFIX);
- size_t off;
-
- if (STREQ(mounts[i], "/dev"))
- suffix = "dev";
-
- if (virAsprintf(&paths[i], "%s/%s.%s",
- cfg->stateDir, vm->def->name, suffix) < 0)
+ if (!(paths[i] = qemuDomainGetPreservedMountPath(cfg, vm, mounts[i])))
goto error;
-
- /* Now consider that mounts[i] is "/dev/blah/blah2".
- * @suffix then points to "blah/blah2". However, caller
- * expects all the @paths to be the same depth. The
- * caller doesn't always do `mkdir -p` but sometimes bare
- * `touch`. Therefore fix all the suffixes. */
- off = strlen(paths[i]) - strlen(suffix);
-
- tmp = paths[i] + off;
- while (*tmp) {
- if (*tmp == '/')
- *tmp = '.';
- tmp++;
- }
}
if (devPath)
--
2.13.0
4:40 p.m.
New subject: [libvirt] [PATCH 3/8] qemu: Move preserved mount points path generation into a separate function
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(a)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 */
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:09 a.m.
New subject: [libvirt] [PATCH 3/8] qemu: Move preserved mount points path generation into a separate function
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(a)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(a)redhat.com>
Thanks.
Michal
9:14 a.m.
New subject: [libvirt] [PATCH 3/8] qemu: Move preserved mount points path generation into a separate function
On 07/10/2017 09:09 AM, Michal Privoznik wrote:
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(a)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. */
I hope the */ ended up on a new line...
>
> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks.
Michal
11:18 a.m.
New subject: [libvirt] [PATCH 4/8] qemuDomainCreateDeviceRecursive: Fail on unsupported file type
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 286d60761..e6fb041de 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7707,6 +7707,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
struct stat sb;
int ret = -1;
bool isLink = false;
+ bool isDev = false;
bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
@@ -7729,6 +7730,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
}
isLink = S_ISLNK(sb.st_mode);
+ isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
/* Here, @device might be whatever path in the system. We
* should create the path in the namespace iff it's "/dev"
@@ -7828,7 +7830,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
if (qemuDomainCreateDeviceRecursive(target, data,
allow_noent, ttl - 1) < 0)
goto cleanup;
- } else {
+ } else if (isDev) {
if (create &&
mknod(devicePath, sb.st_mode, sb.st_rdev) < 0) {
if (errno == EEXIST) {
@@ -7850,6 +7852,11 @@ qemuDomainCreateDeviceRecursive(const char *device,
devicePath);
goto cleanup;
}
+ } else {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("unsupported device type %s %o"),
+ device, (int) sb.st_mode);
+ goto cleanup;
}
if (!create) {
--
2.13.0
4:55 p.m.
New subject: [libvirt] [PATCH 4/8] qemuDomainCreateDeviceRecursive: Fail on unsupported file type
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
Your call - should the "%o" be a "0%o" (see
virStoragePoolDefFormatBuf
and virFileMakePathHelper)? I think that's the more "proper" formatting
style used...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
-
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 286d60761..e6fb041de 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7707,6 +7707,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
struct stat sb;
int ret = -1;
bool isLink = false;
+ bool isDev = false;
bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
@@ -7729,6 +7730,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
}
isLink = S_ISLNK(sb.st_mode);
+ isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
/* Here, @device might be whatever path in the system. We
* should create the path in the namespace iff it's "/dev"
@@ -7828,7 +7830,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
if (qemuDomainCreateDeviceRecursive(target, data,
allow_noent, ttl - 1) < 0)
goto cleanup;
- } else {
+ } else if (isDev) {
if (create &&
mknod(devicePath, sb.st_mode, sb.st_rdev) < 0) {
if (errno == EEXIST) {
@@ -7850,6 +7852,11 @@ qemuDomainCreateDeviceRecursive(const char *device,
devicePath);
goto cleanup;
}
+ } else {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("unsupported device type %s %o"),
^^
e.g. s/%o/0%o
(that's a zero not a capital Oh)
+ device, (int) sb.st_mode);
+ goto cleanup;
}
if (!create) {
8:24 a.m.
New subject: [libvirt] [PATCH 4/8] qemuDomainCreateDeviceRecursive: Fail on unsupported file type
On 06/27/2017 11:55 PM, John Ferlan wrote:
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(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
Your call - should the "%o" be a "0%o" (see
virStoragePoolDefFormatBuf
and virFileMakePathHelper)? I think that's the more "proper" formatting
style used...
Ah, good point. It really should be "0%o". I'll fix that.
Also, as I am looking around now what we use in other areas I've found
that we even have a syntax-check rule for using the latter. For some
reason it isn't caught here...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks.
Michal
11:18 a.m.
New subject: [libvirt] [PATCH 5/8] qemuDomainAttachDeviceMknodHelper: Fail on unsupported file type
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
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"),
+ data->file, (int) data->sb.st_mode);
+ goto cleanup;
}
if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
--
2.13.0
4:57 p.m.
New subject: [libvirt] [PATCH 5/8] qemuDomainAttachDeviceMknodHelper: Fail on unsupported file type
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(a)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(a)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) {
11:18 a.m.
New subject: [libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points
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(a)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
5:11 p.m.
New subject: [libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points
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(a)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(a)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);
8:33 a.m.
New subject: [libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points
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(a)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(a)redhat.com>
Thanks.
Michal
9:20 a.m.
New subject: [libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points
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(a)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(a)redhat.com>
Thanks.
Michal
11:18 a.m.
New subject: [libvirt] [PATCH 7/8] qemuDomainAttachDeviceMknodRecursive: Support file mount points
https://bugzilla.redhat.com/show_bug.cgi?id=1462060
Just like in the previous commit, when attaching a file based
device which has its source living under /dev (that is not a
device rather than a regular file), calling mknod() is no help.
We need to:
1) bind mount device to some temporary location
2) enter the namespace
3) move the mount point to desired place
4) umount it in the parent namespace from the temporary location
At the same time, the check in qemuDomainNamespaceSetupDisk makes
no longer sense. Therefore remove it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d7c218a2..51779c535 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8531,6 +8531,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);
qemuSecurityPostFork(data->driver->securityManager);
@@ -8569,6 +8570,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
} else {
delDevice = true;
}
+ } else if (isReg) {
+ /* We are not cleaning up disks on virDomainDetachDevice
+ * because disk might be still in use by different disk
+ * as its backing chain. This might however clash here.
+ * Therefore do the cleanup here. */
+ if (umount(data->file) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno,
+ _("Unable to umount %s"),
+ data->file);
+ goto cleanup;
+ }
+ if (virFileTouch(data->file, data->sb.st_mode) < 0)
+ goto cleanup;
+ delDevice = true;
+ /* Just create the file here so that code below sets
+ * proper owner and mode. Move the mount only after that. */
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s %o"),
@@ -8583,6 +8601,15 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
goto cleanup;
}
+ /* Symlinks don't have mode */
+ if (!isLink &&
+ chmod(data->file, data->sb.st_mode) < 0) {
+ virReportSystemError(errno,
+ _("Failed to set permissions for device %s"),
+ data->file);
+ goto cleanup;
+ }
+
/* Symlinks don't have ACLs. */
if (!isLink &&
virFileSetACLs(data->file, data->acl) < 0 &&
@@ -8606,6 +8633,11 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
}
#endif
+ /* Finish mount process started earlier. */
+ if (isReg &&
+ virFileMoveMount(data->target, data->file) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
if (ret < 0 && delDevice)
@@ -8626,10 +8658,12 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
size_t ndevMountsPath,
unsigned int ttl)
{
+ virQEMUDriverConfigPtr cfg = NULL;
struct qemuDomainAttachDeviceMknodData data;
int ret = -1;
char *target = NULL;
bool isLink;
+ bool isReg;
if (!ttl) {
virReportSystemError(ELOOP,
@@ -8651,8 +8685,18 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
}
isLink = S_ISLNK(data.sb.st_mode);
+ isReg = S_ISREG(data.sb.st_mode);
- if (isLink) {
+ if (isReg && STRPREFIX(file, DEVPREFIX)) {
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!(target = qemuDomainGetPreservedMountPath(cfg, vm, file)))
+ goto cleanup;
+
+ if (virFileBindMountDevice(file, target) < 0)
+ goto cleanup;
+
+ data.target = target;
+ } else if (isLink) {
if (virFileReadLink(file, &target) < 0) {
virReportSystemError(errno,
_("unable to resolve symlink %s"),
@@ -8739,7 +8783,10 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
freecon(data.tcon);
#endif
virFileFreeACLs(&data.acl);
+ if (isReg && target)
+ umount(target);
VIR_FREE(target);
+ virObjectUnref(cfg);
return ret;
}
@@ -8817,7 +8864,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
char **devMountsPath = NULL;
size_t ndevMountsPath = 0;
virStorageSourcePtr next;
- struct stat sb;
int ret = -1;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
@@ -8836,15 +8882,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
continue;
}
- if (stat(next->path, &sb) < 0) {
- virReportSystemError(errno,
- _("Unable to access %s"), next->path);
- goto cleanup;
- }
-
- if (!S_ISBLK(sb.st_mode))
- continue;
-
if (qemuDomainAttachDeviceMknod(driver,
vm,
next->path,
--
2.13.0
5:31 p.m.
New subject: [libvirt] [PATCH 7/8] qemuDomainAttachDeviceMknodRecursive: Support file mount points
On 06/22/2017 12:18 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1462060
Just like in the previous commit, when attaching a file based
device which has its source living under /dev (that is not a
device rather than a regular file), calling mknod() is no help.
We need to:
1) bind mount device to some temporary location
2) enter the namespace
3) move the mount point to desired place
4) umount it in the parent namespace from the temporary location
At the same time, the check in qemuDomainNamespaceSetupDisk makes
no longer sense. Therefore remove it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
I have the same !isLink comment as last patch, but I see it's even more
prevalent now so I guess it's fine the way it is...
11:18 a.m.
New subject: [libvirt] [PATCH 8/8] qemu ns: Create chardev backends more frequently
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(a)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) {
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,
--
2.13.0
6 p.m.
New subject: [libvirt] [PATCH 8/8] qemu ns: Create chardev backends more frequently
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(a)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(a)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,
8:51 a.m.
New subject: [libvirt] [PATCH 8/8] qemu ns: Create chardev backends more frequently
On 06/28/2017 01:00 AM, John Ferlan wrote:
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(a)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?
It's a result of this change. umout() is not consistent in returned
errno. Depending on type of file that we wanted to umount we might get
either ENOENT or EINVAL.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks.
Michal
2691
days inactive
2710
days old
25 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik