[libvirt] [PATCH 0/4] Clean up machineName-related code

Just in case we'll want to use the "cleaned-up" machineName for anything else in the future, I decided to modify the code so that it is always generated, saved and cleaned-up. While doing that I identified some code duplication, leak and a TODO/XXX comment that are all fixed with this series. And it only adds four lines overall! Martin Kletzander (4): conf: Pass config.priv to xmlopt->privateData.alloc qemu: Save qemu driver in qemuDomainObjPrivateData lxc: Make lxcProcessStop callable even without PID being available Move machineName generation from virsystemd into domain_conf src/bhyve/bhyve_domain.c | 2 +- src/conf/domain_conf.c | 65 +++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 7 ++++- src/libvirt_private.syms | 2 +- src/libxl/libxl_domain.c | 2 +- src/lxc/lxc_cgroup.c | 5 +--- src/lxc/lxc_domain.c | 21 ++++++++++++++- src/lxc/lxc_domain.h | 3 +++ src/lxc/lxc_process.c | 57 +++++++++++----------------------------- src/qemu/qemu_cgroup.c | 24 ++++------------- src/qemu/qemu_domain.c | 24 ++++++++++++++++- src/qemu/qemu_domain.h | 5 ++++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 11 +++++--- src/uml/uml_driver.c | 2 +- src/util/vircgroup.c | 15 ++++------- src/util/vircgroup.h | 14 +++++----- src/util/virsystemd.c | 62 ------------------------------------------- src/util/virsystemd.h | 5 ---- src/vmware/vmware_driver.c | 2 +- src/vz/vz_utils.c | 2 +- tests/virsystemdtest.c | 4 +-- 22 files changed, 170 insertions(+), 166 deletions(-) -- 2.13.3

This will help us to get to some data more easily. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/bhyve/bhyve_domain.c | 2 +- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/uml/uml_driver.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_utils.c | 2 +- 9 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 71764554eb11..3c2344196949 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -33,7 +33,7 @@ VIR_LOG_INIT("bhyve.bhyve_domain"); static void * -bhyveDomainObjPrivateAlloc(void) +bhyveDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) { bhyveDomainObjPrivatePtr priv; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8b3c9b65cd6..2231b195b9c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3032,7 +3032,8 @@ virDomainObjNew(virDomainXMLOptionPtr xmlopt) } if (xmlopt->privateData.alloc) { - if (!(domain->privateData = (xmlopt->privateData.alloc)())) + domain->privateData = (xmlopt->privateData.alloc)(xmlopt->config.priv); + if (!domain->privateData) goto error; domain->privateDataFreeFunc = xmlopt->privateData.free; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4fef773efd93..dfc51208a029 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2562,7 +2562,7 @@ struct _virDomainDefParserConfig { unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; }; -typedef void *(*virDomainXMLPrivateDataAllocFunc)(void); +typedef void *(*virDomainXMLPrivateDataAllocFunc)(void *); typedef void (*virDomainXMLPrivateDataFreeFunc)(void *); typedef virObjectPtr (*virDomainXMLPrivateDataNewFunc)(void); typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 68a501cf1664..7caa6747494a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -206,7 +206,7 @@ libxlDomainJobUpdateTime(struct libxlDomainJobObj *job) } static void * -libxlDomainObjPrivateAlloc(void) +libxlDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) { libxlDomainObjPrivatePtr priv; diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 3a7404f407c9..7c1386e40c82 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -150,7 +150,7 @@ virLXCDomainObjEndJob(virLXCDriverPtr driver ATTRIBUTE_UNUSED, static void * -virLXCDomainObjPrivateAlloc(void) +virLXCDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) { virLXCDomainObjPrivatePtr priv; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2c8c9a74542b..f1e144d92b64 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm) static void * -qemuDomainObjPrivateAlloc(void) +qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) { qemuDomainObjPrivatePtr priv; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 224b71984250..1846835cc659 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -90,7 +90,7 @@ static int umlProcessAutoDestroyRemove(struct uml_driver *driver, static int umlStateCleanup(void); -static void *umlDomainObjPrivateAlloc(void) +static void *umlDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) { umlDomainObjPrivatePtr priv; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 0ee1c5bb983b..8b487c4a7ce0 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -60,7 +60,7 @@ vmwareDriverUnlock(struct vmware_driver *driver) } static void * -vmwareDataAllocFunc(void) +vmwareDataAllocFunc(void *opaque ATTRIBUTE_UNUSED) { vmwareDomainPtr dom; diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index a6d7b93cbc50..770b499c9286 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -581,7 +581,7 @@ int vzCheckUnsupportedGraphics(virDomainGraphicsDefPtr gr) } void* -vzDomObjAlloc(void) +vzDomObjAlloc(void *opaque ATTRIBUTE_UNUSED) { vzDomObjPtr pdom = NULL; -- 2.13.3

This way we can finally make it static and not use any externs anywhere. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 5 +---- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f1e144d92b64..0b7c45280321 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm) static void * -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) +qemuDomainObjPrivateAlloc(void *opaque) { qemuDomainObjPrivatePtr priv; @@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) goto error; priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; + priv->driver = opaque; return priv; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 365b23c96167..71567af034f5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo { typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { + virQEMUDriverPtr driver; + struct qemuDomainJobObj job; virBitmapPtr namespaces; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6568def156f4..9c54571cf078 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom, virDomainObjPtr vm, virDomainInterfacePtr **ifaces); -virQEMUDriverPtr qemu_driver = NULL; +static virQEMUDriverPtr qemu_driver; static void diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 525521aaf0ca..757f5d95e0b7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, } -/* XXX figure out how to remove this */ -extern virQEMUDriverPtr qemu_driver; - /* * This is a callback registered with a qemuAgentPtr instance, * and to be invoked when the agent console hits an end of file @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED, static void qemuProcessFakeReboot(void *opaque) { - virQEMUDriverPtr driver = qemu_driver; virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; virObjectEventPtr event = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainRunningReason reason = VIR_DOMAIN_RUNNING_BOOTED; -- 2.13.3

On 07/24/2017 04:09 PM, Martin Kletzander wrote:
This way we can finally make it static and not use any externs anywhere.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 5 +---- 4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f1e144d92b64..0b7c45280321 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
static void * -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) +qemuDomainObjPrivateAlloc(void *opaque) { qemuDomainObjPrivatePtr priv;
@@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) goto error;
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; + priv->driver = opaque;
return priv;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 365b23c96167..71567af034f5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo { typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { + virQEMUDriverPtr driver; + struct qemuDomainJobObj job;
virBitmapPtr namespaces; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6568def156f4..9c54571cf078 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom, virDomainObjPtr vm, virDomainInterfacePtr **ifaces);
-virQEMUDriverPtr qemu_driver = NULL; +static virQEMUDriverPtr qemu_driver;
static void diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 525521aaf0ca..757f5d95e0b7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, }
-/* XXX figure out how to remove this */ -extern virQEMUDriverPtr qemu_driver; - /* * This is a callback registered with a qemuAgentPtr instance, * and to be invoked when the agent console hits an end of file @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED, static void qemuProcessFakeReboot(void *opaque) { - virQEMUDriverPtr driver = qemu_driver; virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver;
Well, storing driver in private data looks like overkill for this purpose. qemuProcessFakeReboot() runs in a thread (which explains weird function arguments). But in order to pass qemu driver into this function we can make the function take a struct. On the other hand, it might come handy (even right now) as it enables us to clean up some code where we already have both priv and driver in function arguments. Frankly, I'm torn. So it's up to you whether you want to go this way or the one I'm suggesting. Michal

On Tue, Jul 25, 2017 at 03:20:48PM +0200, Michal Privoznik wrote:
On 07/24/2017 04:09 PM, Martin Kletzander wrote:
This way we can finally make it static and not use any externs anywhere.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 5 +---- 4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f1e144d92b64..0b7c45280321 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
static void * -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) +qemuDomainObjPrivateAlloc(void *opaque) { qemuDomainObjPrivatePtr priv;
@@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) goto error;
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; + priv->driver = opaque;
return priv;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 365b23c96167..71567af034f5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo { typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { + virQEMUDriverPtr driver; + struct qemuDomainJobObj job;
virBitmapPtr namespaces; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6568def156f4..9c54571cf078 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom, virDomainObjPtr vm, virDomainInterfacePtr **ifaces);
-virQEMUDriverPtr qemu_driver = NULL; +static virQEMUDriverPtr qemu_driver;
static void diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 525521aaf0ca..757f5d95e0b7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, }
-/* XXX figure out how to remove this */ -extern virQEMUDriverPtr qemu_driver; - /* * This is a callback registered with a qemuAgentPtr instance, * and to be invoked when the agent console hits an end of file @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED, static void qemuProcessFakeReboot(void *opaque) { - virQEMUDriverPtr driver = qemu_driver; virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver;
Well, storing driver in private data looks like overkill for this purpose. qemuProcessFakeReboot() runs in a thread (which explains weird function arguments). But in order to pass qemu driver into this function we can make the function take a struct. On the other hand, it might come handy (even right now) as it enables us to clean up some code where we already have both priv and driver in function arguments. Frankly, I'm torn. So it's up to you whether you want to go this way or the one I'm suggesting.
I'm perfectly fine with passing the struct and I have no problem with changing this that way. The clean up of qemuProcessFakeReboot is not the main purpose of this clean up; it's the last patch and enabling future clean ups. However, I have thought about it and when I pass the struct, it will eventually still get removed and it will end up in this form during the future clean ups. In other words, I can pass the struct, but it's not needed any more since the driver will be available even without that =) So I'll stick with this since you left me decide ;)
Michal

On 07/25/2017 03:48 PM, Martin Kletzander wrote:
On Tue, Jul 25, 2017 at 03:20:48PM +0200, Michal Privoznik wrote:
On 07/24/2017 04:09 PM, Martin Kletzander wrote:
This way we can finally make it static and not use any externs anywhere.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 5 +---- 4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f1e144d92b64..0b7c45280321 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
static void * -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) +qemuDomainObjPrivateAlloc(void *opaque) { qemuDomainObjPrivatePtr priv;
@@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED) goto error;
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; + priv->driver = opaque;
return priv;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 365b23c96167..71567af034f5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo { typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { + virQEMUDriverPtr driver; + struct qemuDomainJobObj job;
virBitmapPtr namespaces; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6568def156f4..9c54571cf078 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom, virDomainObjPtr vm, virDomainInterfacePtr **ifaces);
-virQEMUDriverPtr qemu_driver = NULL; +static virQEMUDriverPtr qemu_driver;
static void diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 525521aaf0ca..757f5d95e0b7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, }
-/* XXX figure out how to remove this */ -extern virQEMUDriverPtr qemu_driver; - /* * This is a callback registered with a qemuAgentPtr instance, * and to be invoked when the agent console hits an end of file @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED, static void qemuProcessFakeReboot(void *opaque) { - virQEMUDriverPtr driver = qemu_driver; virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver;
Well, storing driver in private data looks like overkill for this purpose. qemuProcessFakeReboot() runs in a thread (which explains weird function arguments). But in order to pass qemu driver into this function we can make the function take a struct. On the other hand, it might come handy (even right now) as it enables us to clean up some code where we already have both priv and driver in function arguments. Frankly, I'm torn. So it's up to you whether you want to go this way or the one I'm suggesting.
I'm perfectly fine with passing the struct and I have no problem with changing this that way. The clean up of qemuProcessFakeReboot is not the main purpose of this clean up; it's the last patch and enabling future clean ups. However, I have thought about it and when I pass the struct, it will eventually still get removed and it will end up in this form during the future clean ups. In other words, I can pass the struct, but it's not needed any more since the driver will be available even without that =)
So I'll stick with this since you left me decide ;)
Right. We don't need both approaches. ACK to this and to 1/4 too then. Although, let me just point out that it was difficult for me to track that it is indeed driver that is passed as @opaque to qemuDomainObjPrivateAlloc(). It isn't visible at the first glance. Michal

This way the function can work as a central point of clean-up code and we don't have to duplicate code. And it works similarly to the qemu driver. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/lxc/lxc_process.c | 32 ++------------------------------ 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 2658ea61f89e..724297d128d0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -845,12 +845,6 @@ int virLXCProcessStop(virLXCDriverPtr driver, priv = vm->privateData; - if (vm->pid <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid PID %d for container"), vm->pid); - return -1; - } - virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, false, false); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); @@ -895,7 +889,7 @@ int virLXCProcessStop(virLXCDriverPtr driver, _("Some processes refused to die")); return -1; } - } else { + } else if (vm->pid > 0) { /* If cgroup doesn't exist, just try cleaning up the * libvirt_lxc process */ if (virProcessKillPainfully(vm->pid, true) < 0) { @@ -1210,8 +1204,6 @@ int virLXCProcessStart(virConnectPtr conn, virCgroupPtr selfcgroup; int status; char *pidfile = NULL; - bool clearSeclabel = false; - bool need_stop = false; if (virCgroupNewSelf(&selfcgroup) < 0) return -1; @@ -1331,9 +1323,6 @@ int virLXCProcessStart(virConnectPtr conn, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); - clearSeclabel = vm->def->nseclabels == 0 || - vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DEFAULT; - if (vm->def->nseclabels && vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DEFAULT) vm->def->seclabels[0]->type = VIR_DOMAIN_SECLABEL_NONE; @@ -1468,7 +1457,6 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; } - need_stop = true; priv->stopReason = VIR_DOMAIN_EVENT_STOPPED_FAILED; priv->wantReboot = false; vm->def->id = vm->pid; @@ -1574,23 +1562,7 @@ int virLXCProcessStart(virConnectPtr conn, } if (rc != 0) { err = virSaveLastError(); - if (need_stop) { - virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); - } else { - virSecurityManagerRestoreAllLabel(driver->securityManager, - vm->def, false, false); - virSecurityManagerReleaseLabel(driver->securityManager, vm->def); - /* Clear out dynamically assigned labels */ - if (vm->def->nseclabels && - (vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC || - clearSeclabel)) { - VIR_FREE(vm->def->seclabels[0]->model); - VIR_FREE(vm->def->seclabels[0]->label); - VIR_FREE(vm->def->seclabels[0]->imagelabel); - VIR_DELETE_ELEMENT(vm->def->seclabels, 0, vm->def->nseclabels); - } - virLXCProcessCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); - } + virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); } virCommandFree(cmd); for (i = 0; i < nveths; i++) -- 2.13.3

On 07/24/2017 04:09 PM, Martin Kletzander wrote:
This way the function can work as a central point of clean-up code and we don't have to duplicate code. And it works similarly to the qemu driver.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/lxc/lxc_process.c | 32 ++------------------------------ 1 file changed, 2 insertions(+), 30 deletions(-)
ACK Michal

It is more related to a domain as we might use it even when there is no systemd and it does not use any dbus/systemd functions. In order not to use code from conf/ in util/ pass machineName in cgroups code as a parameter. That also fixes a leak of machineName in the lxc driver and cleans up and de-duplicates some code. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++++ src/libvirt_private.syms | 2 +- src/lxc/lxc_cgroup.c | 5 +--- src/lxc/lxc_domain.c | 19 +++++++++++++++ src/lxc/lxc_domain.h | 3 +++ src/lxc/lxc_process.c | 25 +++++++++---------- src/qemu/qemu_cgroup.c | 24 ++++--------------- src/qemu/qemu_domain.c | 21 ++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 6 +++++ src/util/vircgroup.c | 15 ++++-------- src/util/vircgroup.h | 14 +++++------ src/util/virsystemd.c | 62 ------------------------------------------------ src/util/virsystemd.h | 5 ---- tests/virsystemdtest.c | 4 ++-- 16 files changed, 153 insertions(+), 122 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2231b195b9c4..98e2e7292912 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27007,3 +27007,65 @@ virDomainDiskSetBlockIOTune(virDomainDiskDefPtr disk, return 0; } + +#define HOSTNAME_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-" + +static void +virDomainMachineNameAppendValid(virBufferPtr buf, + const char *name) +{ + bool skip_dot = false; + + for (; *name; name++) { + if (virBufferError(buf)) + break; + if (strlen(virBufferCurrentContent(buf)) >= 64) + break; + + if (*name == '.') { + if (!skip_dot) + virBufferAddChar(buf, *name); + skip_dot = true; + continue; + } + + skip_dot = false; + + if (!strchr(HOSTNAME_CHARS, *name)) + continue; + + virBufferAddChar(buf, *name); + } +} + +#undef HOSTNAME_CHARS + +char * +virDomainGenerateMachineName(const char *drivername, + int id, + const char *name, + bool privileged) +{ + char *machinename = NULL; + char *username = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (privileged) { + virBufferAsprintf(&buf, "%s-", drivername); + } else { + if (!(username = virGetUserName(geteuid()))) + goto cleanup; + + virBufferAsprintf(&buf, "%s-%s-", username, drivername); + } + + virBufferAsprintf(&buf, "%d-", id); + virDomainMachineNameAppendValid(&buf, name); + + machinename = virBufferContentAndReset(&buf); + cleanup: + VIR_FREE(username); + + return machinename; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index dfc51208a029..e422e01e71ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3341,4 +3341,9 @@ virDomainGetBlkioParametersAssignFromDef(virDomainDefPtr def, int virDomainDiskSetBlockIOTune(virDomainDiskDefPtr disk, virDomainBlockIoTuneInfo *info); +char * +virDomainGenerateMachineName(const char *drivername, + int id, + const char *name, + bool privileged); #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 87753758f9cc..c5c048d0189f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -340,6 +340,7 @@ virDomainFSTypeFromString; virDomainFSTypeToString; virDomainFSWrpolicyTypeFromString; virDomainFSWrpolicyTypeToString; +virDomainGenerateMachineName; virDomainGetFilesystemForTarget; virDomainGraphicsAuthConnectedTypeFromString; virDomainGraphicsAuthConnectedTypeToString; @@ -2711,7 +2712,6 @@ virSystemdCanSuspend; virSystemdCreateMachine; virSystemdGetMachineNameByPID; virSystemdHasMachinedResetCachedValue; -virSystemdMakeMachineName; virSystemdMakeScopeName; virSystemdMakeSliceName; virSystemdNotifyStartup; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 1c42ab5cde9d..336980187035 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -485,10 +485,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, int *nicindexes) { virCgroupPtr cgroup = NULL; - char *machineName = virSystemdMakeMachineName("lxc", - def->id, - def->name, - true); + char *machineName = virLXCDomainGetMachineName(def, 0); if (!machineName) goto cleanup; diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 7c1386e40c82..d23969a585f4 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -31,6 +31,7 @@ #include "virutil.h" #include "virfile.h" #include "virtime.h" +#include "virsystemd.h" #define VIR_FROM_THIS VIR_FROM_LXC #define LXC_NAMESPACE_HREF "http://libvirt.org/schemas/domain/lxc/1.0" @@ -397,3 +398,21 @@ virDomainDefParserConfig virLXCDriverDomainDefParserConfig = { .domainPostParseCallback = virLXCDomainDefPostParse, .devicesPostParseCallback = virLXCDomainDeviceDefPostParse, }; + + +char * +virLXCDomainGetMachineName(virDomainDefPtr def, pid_t pid) +{ + char *ret = NULL; + + if (pid) { + ret = virSystemdGetMachineNameByPID(pid); + if (!ret) + virResetLastError(); + } + + if (!ret) + ret = virDomainGenerateMachineName("lxc", def->id, def->name, true); + + return ret; +} diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h index 5c4f31e54625..b248cdfd6f34 100644 --- a/src/lxc/lxc_domain.h +++ b/src/lxc/lxc_domain.h @@ -107,4 +107,7 @@ virLXCDomainObjEndJob(virLXCDriverPtr driver, virDomainObjPtr obj); +char * +virLXCDomainGetMachineName(virDomainDefPtr def, pid_t pid); + #endif /* __LXC_DOMAIN_H__ */ diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 724297d128d0..05f1dec4c421 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -234,6 +234,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, * the bug we are working around here. */ virCgroupTerminateMachine(priv->machineName); + VIR_FREE(priv->machineName); /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_LXC)) { @@ -1494,13 +1495,17 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; } + priv->machineName = virLXCDomainGetMachineName(vm->def, vm->pid); + if (!priv->machineName) + goto cleanup; + /* We know the cgroup must exist by this synchronization * point so lets detect that first, since it gives us a * more reliable way to kill everything off if something * goes wrong from here onwards ... */ if (virCgroupNewDetectMachine(vm->def->name, "lxc", - vm->def->id, true, - vm->pid, -1, &priv->cgroup) < 0) + vm->pid, -1, priv->machineName, + &priv->cgroup) < 0) goto cleanup; if (!priv->cgroup) { @@ -1510,11 +1515,6 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; } - /* Get the machine name so we can properly delete it through - * systemd later */ - if (!(priv->machineName = virSystemdGetMachineNameByPID(vm->pid))) - virResetLastError(); - /* And we can get the first monitor connection now too */ if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) { /* Intentionally overwrite the real monitor error message, @@ -1666,8 +1666,12 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) goto error; - if (virCgroupNewDetectMachine(vm->def->name, "lxc", vm->def->id, true, - vm->pid, -1, &priv->cgroup) < 0) + priv->machineName = virLXCDomainGetMachineName(vm->def, vm->pid); + if (!priv->machineName) + goto cleanup; + + if (virCgroupNewDetectMachine(vm->def->name, "lxc", vm->pid, -1, + priv->machineName, &priv->cgroup) < 0) goto error; if (!priv->cgroup) { @@ -1677,9 +1681,6 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, goto error; } - if (!(priv->machineName = virSystemdGetMachineNameByPID(vm->pid))) - virResetLastError(); - if (virLXCUpdateActiveUSBHostdevs(driver, vm->def) < 0) goto error; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 36762d4f676e..7355527c1cc5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -852,17 +852,6 @@ qemuInitCgroup(virQEMUDriverPtr driver, goto cleanup; } - /* - * We need to do this because of systemd-machined, because - * CreateMachine requires the name to be a valid hostname. - */ - priv->machineName = virSystemdMakeMachineName("qemu", - vm->def->id, - vm->def->name, - virQEMUDriverIsPrivileged(driver)); - if (!priv->machineName) - goto cleanup; - if (virCgroupNewMachine(priv->machineName, "qemu", vm->def->uuid, @@ -978,21 +967,20 @@ qemuConnectCgroup(virQEMUDriverPtr driver, if (!virCgroupAvailable()) goto done; + priv->machineName = qemuDomainGetMachineName(vm); + if (!priv->machineName) + goto cleanup; + virCgroupFree(&priv->cgroup); if (virCgroupNewDetectMachine(vm->def->name, "qemu", - vm->def->id, - virQEMUDriverIsPrivileged(driver), vm->pid, cfg->cgroupControllers, + priv->machineName, &priv->cgroup) < 0) goto cleanup; - priv->machineName = virSystemdGetMachineNameByPID(vm->pid); - if (!priv->machineName) - virResetLastError(); - qemuRestoreCgroupState(vm); done: @@ -1164,8 +1152,6 @@ qemuRemoveCgroup(virDomainObjPtr vm) VIR_DEBUG("Failed to terminate cgroup for %s", vm->def->name); } - VIR_FREE(priv->machineName); - return virCgroupRemove(priv->cgroup); } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0b7c45280321..b71ca168ce18 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -49,6 +49,7 @@ #include "viratomic.h" #include "virprocess.h" #include "vircrypto.h" +#include "virsystemd.h" #include "secret_util.h" #include "logging/log_manager.h" #include "locking/domain_lock.h" @@ -9568,3 +9569,23 @@ qemuDomainUpdateCPU(virDomainObjPtr vm, return 0; } + +char * +qemuDomainGetMachineName(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + char *ret = NULL; + + if (vm->pid) { + ret = virSystemdGetMachineNameByPID(vm->pid); + if (!ret) + virResetLastError(); + } + + if (!ret) + ret = virDomainGenerateMachineName("qemu", vm->def->id, vm->def->name, + virQEMUDriverIsPrivileged(driver)); + + return ret; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 71567af034f5..4c9050aff000 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -934,4 +934,7 @@ qemuDomainUpdateCPU(virDomainObjPtr vm, virCPUDefPtr cpu, virCPUDefPtr *origCPU); +char * +qemuDomainGetMachineName(virDomainObjPtr vm); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 757f5d95e0b7..87ab85fdb476 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5242,6 +5242,10 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + priv->machineName = qemuDomainGetMachineName(vm); + if (!priv->machineName) + goto cleanup; + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ @@ -6307,6 +6311,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, } } + VIR_FREE(priv->machineName); + vm->taint = 0; vm->pid = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 2912fc9be539..f274aee81ab9 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -252,17 +252,14 @@ static bool virCgroupValidateMachineGroup(virCgroupPtr group, const char *name, const char *drivername, - int id, - bool privileged, - bool stripEmulatorSuffix) + bool stripEmulatorSuffix, + char *machinename) { size_t i; bool valid = false; char *partname = NULL; char *scopename_old = NULL; char *scopename_new = NULL; - char *machinename = virSystemdMakeMachineName(drivername, id, - name, privileged); char *partmachinename = NULL; if (virAsprintf(&partname, "%s.libvirt-%s", @@ -1539,10 +1536,9 @@ virCgroupNewDetect(pid_t pid, int virCgroupNewDetectMachine(const char *name, const char *drivername, - int id, - bool privileged, pid_t pid, int controllers, + char *machinename, virCgroupPtr *group) { if (virCgroupNewDetect(pid, controllers, group) < 0) { @@ -1552,7 +1548,7 @@ virCgroupNewDetectMachine(const char *name, } if (!virCgroupValidateMachineGroup(*group, name, drivername, - id, privileged, true)) { + true, machinename)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); virCgroupFree(group); @@ -4208,10 +4204,9 @@ virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED, int virCgroupNewDetectMachine(const char *name ATTRIBUTE_UNUSED, const char *drivername ATTRIBUTE_UNUSED, - int id ATTRIBUTE_UNUSED, - bool privileged ATTRIBUTE_UNUSED, pid_t pid ATTRIBUTE_UNUSED, int controllers ATTRIBUTE_UNUSED, + char *machinename ATTRIBUTE_UNUSED, virCgroupPtr *group ATTRIBUTE_UNUSED) { virReportSystemError(ENXIO, "%s", diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 2de1bf2de4ce..d83392767864 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -94,13 +94,13 @@ int virCgroupNewDetect(pid_t pid, int controllers, virCgroupPtr *group); -int virCgroupNewDetectMachine(const char *name, - const char *drivername, - int id, - bool privileged, - pid_t pid, - int controllers, - virCgroupPtr *group) +int +virCgroupNewDetectMachine(const char *name, + const char *drivername, + pid_t pid, + int controllers, + char *machinename, + virCgroupPtr *group) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virCgroupNewMachine(const char *name, diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 5d9746f9955f..829b92d54d94 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -125,68 +125,6 @@ char *virSystemdMakeSliceName(const char *partition) return virBufferContentAndReset(&buf); } -#define HOSTNAME_CHARS \ - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-" - -static void -virSystemdAppendValidMachineName(virBufferPtr buf, - const char *name) -{ - bool skip_dot = false; - - for (; *name; name++) { - if (virBufferError(buf)) - break; - if (strlen(virBufferCurrentContent(buf)) >= 64) - break; - - if (*name == '.') { - if (!skip_dot) - virBufferAddChar(buf, *name); - skip_dot = true; - continue; - } - - skip_dot = false; - - if (!strchr(HOSTNAME_CHARS, *name)) - continue; - - virBufferAddChar(buf, *name); - } -} - -#undef HOSTNAME_CHARS - -char * -virSystemdMakeMachineName(const char *drivername, - int id, - const char *name, - bool privileged) -{ - char *machinename = NULL; - char *username = NULL; - virBuffer buf = VIR_BUFFER_INITIALIZER; - - if (privileged) { - virBufferAsprintf(&buf, "%s-", drivername); - } else { - if (!(username = virGetUserName(geteuid()))) - goto cleanup; - - virBufferAsprintf(&buf, "%s-%s-", username, drivername); - } - - virBufferAsprintf(&buf, "%d-", id); - virSystemdAppendValidMachineName(&buf, name); - - machinename = virBufferContentAndReset(&buf); - cleanup: - VIR_FREE(username); - - return machinename; -} - static int virSystemdHasMachinedCachedValue = -1; /* Reset the cache from tests for testing the underlying dbus calls diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h index 93b0aae7e15f..9aedb40b86f1 100644 --- a/src/util/virsystemd.h +++ b/src/util/virsystemd.h @@ -29,11 +29,6 @@ char *virSystemdMakeScopeName(const char *name, bool legacy_behaviour); char *virSystemdMakeSliceName(const char *partition); -char *virSystemdMakeMachineName(const char *drivername, - int id, - const char *name, - bool privileged); - int virSystemdCreateMachine(const char *name, const char *drivername, const unsigned char *uuid, diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 15642759e94f..4f4f29bfacde 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -413,8 +413,8 @@ testMachineName(const void *opaque) int ret = -1; char *actual = NULL; - if (!(actual = virSystemdMakeMachineName("qemu", data->id, - data->name, true))) + if (!(actual = virDomainGenerateMachineName("qemu", data->id, + data->name, true))) goto cleanup; if (STRNEQ(actual, data->expected)) { -- 2.13.3

On 07/24/2017 04:09 PM, Martin Kletzander wrote:
It is more related to a domain as we might use it even when there is no systemd and it does not use any dbus/systemd functions. In order not to use code from conf/ in util/ pass machineName in cgroups code as a parameter. That also fixes a leak of machineName in the lxc driver and cleans up and de-duplicates some code.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++++ src/libvirt_private.syms | 2 +- src/lxc/lxc_cgroup.c | 5 +--- src/lxc/lxc_domain.c | 19 +++++++++++++++ src/lxc/lxc_domain.h | 3 +++ src/lxc/lxc_process.c | 25 +++++++++---------- src/qemu/qemu_cgroup.c | 24 ++++--------------- src/qemu/qemu_domain.c | 21 ++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 6 +++++ src/util/vircgroup.c | 15 ++++-------- src/util/vircgroup.h | 14 +++++------ src/util/virsystemd.c | 62 ------------------------------------------------ src/util/virsystemd.h | 5 ---- tests/virsystemdtest.c | 4 ++-- 16 files changed, 153 insertions(+), 122 deletions(-)
ACK Michal
participants (2)
-
Martin Kletzander
-
Michal Privoznik