'virObjectNew' can't return NULL. If we pre-check the arguments we don't need a cleanup label. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/datatypes.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index da8a9970f1..c83a74edd5 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -292,13 +292,11 @@ virGetDomain(virConnectPtr conn, if (virDataTypesInitialize() < 0) return NULL; - virCheckConnectGoto(conn, error); - virCheckNonNullArgGoto(name, error); - virCheckNonNullArgGoto(uuid, error); - - if (!(ret = virObjectNew(virDomainClass))) - goto error; + virCheckConnectReturn(conn, NULL); + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(uuid, NULL); + ret = virObjectNew(virDomainClass); ret->name = g_strdup(name); ret->conn = virObjectRef(conn); @@ -306,10 +304,6 @@ virGetDomain(virConnectPtr conn, memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); return ret; - - error: - virObjectUnref(ret); - return NULL; } /** -- 2.38.1

On 1/5/23 10:29 AM, Peter Krempa wrote: Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

Use the new style which doesn't require re-aligning the argument list once you change the return type. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/virdomainobjlist.h | 156 ++++++++++++++++++++---------------- 1 file changed, 86 insertions(+), 70 deletions(-) diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index 4169eb4f78..cfa165d56f 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -26,72 +26,85 @@ typedef struct _virDomainObjList virDomainObjList; -virDomainObjList *virDomainObjListNew(void); - -virDomainObj *virDomainObjListFindByID(virDomainObjList *doms, - int id); -virDomainObj *virDomainObjListFindByUUID(virDomainObjList *doms, - const unsigned char *uuid); -virDomainObj *virDomainObjListFindByName(virDomainObjList *doms, - const char *name); +virDomainObjList * +virDomainObjListNew(void); + +virDomainObj * +virDomainObjListFindByID(virDomainObjList *doms, + int id); +virDomainObj * +virDomainObjListFindByUUID(virDomainObjList *doms, + const unsigned char *uuid); +virDomainObj * +virDomainObjListFindByName(virDomainObjList *doms, + const char *name); enum { VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0), VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), }; -virDomainObj *virDomainObjListAdd(virDomainObjList *doms, - virDomainDef **def, - virDomainXMLOption *xmlopt, - unsigned int flags, - virDomainDef **oldDef); +virDomainObj * +virDomainObjListAdd(virDomainObjList *doms, + virDomainDef **def, + virDomainXMLOption *xmlopt, + unsigned int flags, + virDomainDef **oldDef); typedef int (*virDomainObjListRenameCallback)(virDomainObj *dom, const char *new_name, unsigned int flags, void *opaque); -int virDomainObjListRename(virDomainObjList *doms, - virDomainObj *dom, - const char *new_name, - unsigned int flags, - virDomainObjListRenameCallback callback, - void *opaque); - -void virDomainObjListRemove(virDomainObjList *doms, - virDomainObj *dom); -void virDomainObjListRemoveLocked(virDomainObjList *doms, - virDomainObj *dom); - -int virDomainObjListLoadAllConfigs(virDomainObjList *doms, - const char *configDir, - const char *autostartDir, - bool liveStatus, - virDomainXMLOption *xmlopt, - virDomainLoadConfigNotify notify, - void *opaque); - -int virDomainObjListNumOfDomains(virDomainObjList *doms, - bool active, - virDomainObjListACLFilter filter, - virConnectPtr conn); - -int virDomainObjListGetActiveIDs(virDomainObjList *doms, - int *ids, - int maxids, +int +virDomainObjListRename(virDomainObjList *doms, + virDomainObj *dom, + const char *new_name, + unsigned int flags, + virDomainObjListRenameCallback callback, + void *opaque); + +void +virDomainObjListRemove(virDomainObjList *doms, + virDomainObj *dom); +void +virDomainObjListRemoveLocked(virDomainObjList *doms, + virDomainObj *dom); + +int +virDomainObjListLoadAllConfigs(virDomainObjList *doms, + const char *configDir, + const char *autostartDir, + bool liveStatus, + virDomainXMLOption *xmlopt, + virDomainLoadConfigNotify notify, + void *opaque); + +int +virDomainObjListNumOfDomains(virDomainObjList *doms, + bool active, + virDomainObjListACLFilter filter, + virConnectPtr conn); + +int +virDomainObjListGetActiveIDs(virDomainObjList *doms, + int *ids, + int maxids, + virDomainObjListACLFilter filter, + virConnectPtr conn); +int +virDomainObjListGetInactiveNames(virDomainObjList *doms, + char **const names, + int maxnames, virDomainObjListACLFilter filter, virConnectPtr conn); -int virDomainObjListGetInactiveNames(virDomainObjList *doms, - char **const names, - int maxnames, - virDomainObjListACLFilter filter, - virConnectPtr conn); typedef int (*virDomainObjListIterator)(virDomainObj *dom, void *opaque); -int virDomainObjListForEach(virDomainObjList *doms, - bool modify, - virDomainObjListIterator callback, - void *opaque); +int +virDomainObjListForEach(virDomainObjList *doms, + bool modify, + virDomainObjListIterator callback, + void *opaque); #define VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE \ (VIR_CONNECT_LIST_DOMAINS_ACTIVE | \ @@ -132,23 +145,26 @@ int virDomainObjListForEach(virDomainObjList *doms, VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_CHECKPOINT) -int virDomainObjListCollect(virDomainObjList *doms, - virConnectPtr conn, - virDomainObj ***vms, - size_t *nvms, - virDomainObjListACLFilter filter, - unsigned int flags); -int virDomainObjListExport(virDomainObjList *doms, - virConnectPtr conn, - virDomainPtr **domains, - virDomainObjListACLFilter filter, - unsigned int flags); -int virDomainObjListConvert(virDomainObjList *domlist, - virConnectPtr conn, - virDomainPtr *doms, - size_t ndoms, - virDomainObj ***vms, - size_t *nvms, - virDomainObjListACLFilter filter, - unsigned int flags, - bool skip_missing); +int +virDomainObjListCollect(virDomainObjList *doms, + virConnectPtr conn, + virDomainObj ***vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags); +int +virDomainObjListExport(virDomainObjList *doms, + virConnectPtr conn, + virDomainPtr **domains, + virDomainObjListACLFilter filter, + unsigned int flags); +int +virDomainObjListConvert(virDomainObjList *domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObj ***vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing); -- 2.38.1

On 1/5/23 10:29 AM, Peter Krempa wrote: Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

The function can't fail so there's no point in returning anything. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/virdomainobjlist.c | 7 ++----- src/conf/virdomainobjlist.h | 2 +- src/qemu/qemu_driver.c | 7 +++---- src/test/test_driver.c | 4 +--- src/vz/vz_driver.c | 7 +++---- 5 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 4968dfcf3e..13675acda5 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -964,7 +964,7 @@ virDomainObjListFilter(virDomainObj ***list, } -int +void virDomainObjListCollect(virDomainObjList *domlist, virConnectPtr conn, virDomainObj ***vms, @@ -974,8 +974,6 @@ virDomainObjListCollect(virDomainObjList *domlist, { virDomainObjListCollectAll(domlist, vms, nvms); virDomainObjListFilter(vms, nvms, conn, filter, flags); - - return 0; } @@ -1046,8 +1044,7 @@ virDomainObjListExport(virDomainObjList *domlist, size_t i; int ret = -1; - if (virDomainObjListCollect(domlist, conn, &vms, &nvms, filter, flags) < 0) - return -1; + virDomainObjListCollect(domlist, conn, &vms, &nvms, filter, flags); if (domains) { doms = g_new0(virDomainPtr, nvms + 1); diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index 8c53680374..058f2c4ca6 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -149,7 +149,7 @@ void virDomainObjListCollectAll(virDomainObjList *domlist, virDomainObj ***vms, size_t *nvms); -int +void virDomainObjListCollect(virDomainObjList *doms, virConnectPtr conn, virDomainObj ***vms, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d509582719..1b3da86c81 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18665,10 +18665,9 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, lflags, true) < 0) return -1; } else { - if (virDomainObjListCollect(driver->domains, conn, &vms, &nvms, - virConnectGetAllDomainStatsCheckACL, - lflags) < 0) - return -1; + virDomainObjListCollect(driver->domains, conn, &vms, &nvms, + virConnectGetAllDomainStatsCheckACL, + lflags); } tmpstats = g_new0(virDomainStatsRecordPtr, nvms + 1); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6c21c6bcb4..bd6f063a00 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9923,9 +9923,7 @@ testConnectGetAllDomainStats(virConnectPtr conn, &nvms, NULL, lflags, true) < 0) return -1; } else { - if (virDomainObjListCollect(driver->domains, conn, &vms, &nvms, - NULL, lflags) < 0) - return -1; + virDomainObjListCollect(driver->domains, conn, &vms, &nvms, NULL, lflags); } tmpstats = g_new0(virDomainStatsRecordPtr, nvms + 1); diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index d5147a6c0d..327704b375 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3771,10 +3771,9 @@ vzConnectGetAllDomainStats(virConnectPtr conn, lflags, true) < 0) return -1; } else { - if (virDomainObjListCollect(driver->domains, conn, &doms, &ndoms, - virConnectGetAllDomainStatsCheckACL, - lflags) < 0) - return -1; + virDomainObjListCollect(driver->domains, conn, &doms, &ndoms, + virConnectGetAllDomainStatsCheckACL, + lflags); } tmpstats = g_new0(virDomainStatsRecordPtr, ndoms + 1); -- 2.38.1

On Thu, Jan 05, 2023 at 05:29:56PM +0100, Peter Krempa wrote: Incorrect indentation. Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt;

The rewrite is straightforward as LXC registers only the 'lxcProcessAutoDestroy' callback which by design doesn't need any special handling (there's just one caller which can start the VM thus implicitly there's only one possible registration for that function). Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/lxc/lxc_conf.c | 15 +++++++++++---- src/lxc/lxc_conf.h | 3 --- src/lxc/lxc_driver.c | 8 ++------ src/lxc/lxc_process.c | 8 +++----- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index fefe63bf20..146e43c5d5 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -184,12 +184,19 @@ virCaps *virLXCDriverGetCapabilities(virLXCDriver *driver, virDomainXMLOption * lxcDomainXMLConfInit(virLXCDriver *driver, const char *defsecmodel) { + virDomainXMLOption *ret = NULL; + virLXCDriverDomainDefParserConfig.priv = driver; virLXCDriverDomainDefParserConfig.defSecModel = defsecmodel; - return virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig, - &virLXCDriverPrivateDataCallbacks, - &virLXCDriverDomainXMLNamespace, - NULL, NULL, NULL); + + ret = virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig, + &virLXCDriverPrivateDataCallbacks, + &virLXCDriverDomainXMLNamespace, + NULL, NULL, NULL); + + virDomainXMLOptionSetCloseCallbackAlloc(ret, virCloseCallbacksDomainAlloc); + + return ret; } diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 6b9004aa3c..c0967ac63b 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -93,9 +93,6 @@ struct _virLXCDriver { /* Immutable pointer. self-locking APIs */ virSecurityManager *securityManager; - - /* Immutable pointer, self-locking APIs */ - virCloseCallbacks *closeCallbacks; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virLXCDriverConfig, virObjectUnref); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 5a16e7375e..f49964285b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -161,7 +161,8 @@ static int lxcConnectClose(virConnectPtr conn) { virLXCDriver *driver = conn->privateData; - virCloseCallbacksRun(driver->closeCallbacks, conn, driver->domains); + virCloseCallbacksDomainRunForConn(driver->domains, conn); + conn->privateData = NULL; return 0; } @@ -1496,9 +1497,6 @@ static int lxcStateInitialize(bool privileged, if (!(lxc_driver->xmlopt = lxcDomainXMLConfInit(lxc_driver, defsecmodel))) goto cleanup; - if (!(lxc_driver->closeCallbacks = virCloseCallbacksNew())) - goto cleanup; - if (g_mkdir_with_parents(cfg->stateDir, 0777) < 0) { virReportSystemError(errno, _("Failed to mkdir %s"), @@ -1586,8 +1584,6 @@ static int lxcStateCleanup(void) virObjectUnref(lxc_driver->domains); virObjectUnref(lxc_driver->domainEventState); - virObjectUnref(lxc_driver->closeCallbacks); - virSysinfoDefFree(lxc_driver->hostsysinfo); virObjectUnref(lxc_driver->hostdevMgr); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 2a753ae1da..14fe60b074 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -188,7 +188,7 @@ static void virLXCProcessCleanup(virLXCDriver *driver, /* Stop autodestroy in case guest is restarted */ if (flags & VIR_LXC_PROCESS_CLEANUP_AUTODESTROY) { - virCloseCallbacksUnset(driver->closeCallbacks, vm, lxcProcessAutoDestroy); + virCloseCallbacksDomainRemove(vm, NULL, lxcProcessAutoDestroy); } if (priv->monitor) { @@ -1504,10 +1504,8 @@ int virLXCProcessStart(virLXCDriver * driver, goto cleanup; } - if (autoDestroyConn && - virCloseCallbacksSet(driver->closeCallbacks, vm, - autoDestroyConn, lxcProcessAutoDestroy) < 0) - goto cleanup; + if (autoDestroyConn) + virCloseCallbacksDomainAdd(vm, autoDestroyConn, lxcProcessAutoDestroy); /* We don't need the temporary NIC names anymore, clear them */ virLXCProcessCleanInterfaces(vm->def); -- 2.38.1

The rewrite is straightforward as bhyve registers only the 'bhyveProcessAutoDestroy' callback which by design doesn't need any special handling (there's just one caller which can start the VM thus implicitly there's only one possible registration for that function). Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/bhyve/bhyve_domain.c | 15 +++++++++++---- src/bhyve/bhyve_driver.c | 6 +----- src/bhyve/bhyve_process.c | 9 +++------ src/bhyve/bhyve_utils.h | 2 -- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index b7b2db57b8..a1d1ebc706 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -217,11 +217,18 @@ bhyveDomainDefAssignAddresses(virDomainDef *def, virDomainXMLOption * virBhyveDriverCreateXMLConf(struct _bhyveConn *driver) { + virDomainXMLOption *ret = NULL; + virBhyveDriverDomainDefParserConfig.priv = driver; - return virDomainXMLOptionNew(&virBhyveDriverDomainDefParserConfig, - &virBhyveDriverPrivateDataCallbacks, - &virBhyveDriverDomainXMLNamespace, - NULL, NULL, NULL); + + ret = virDomainXMLOptionNew(&virBhyveDriverDomainDefParserConfig, + &virBhyveDriverPrivateDataCallbacks, + &virBhyveDriverDomainXMLNamespace, + NULL, NULL, NULL); + + virDomainXMLOptionSetCloseCallbackAlloc(ret, virCloseCallbacksDomainAlloc); + + return ret; } diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index e0bf2a19a6..d100adeb8f 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -207,7 +207,7 @@ bhyveConnectClose(virConnectPtr conn) { struct _bhyveConn *privconn = conn->privateData; - virCloseCallbacksRun(privconn->closeCallbacks, conn, privconn->domains); + virCloseCallbacksDomainRunForConn(privconn->domains, conn); conn->privateData = NULL; return 0; @@ -1161,7 +1161,6 @@ bhyveStateCleanup(void) virObjectUnref(bhyve_driver->caps); virObjectUnref(bhyve_driver->xmlopt); virSysinfoDefFree(bhyve_driver->hostsysinfo); - virObjectUnref(bhyve_driver->closeCallbacks); virObjectUnref(bhyve_driver->domainEventState); virObjectUnref(bhyve_driver->config); virPortAllocatorRangeFree(bhyve_driver->remotePorts); @@ -1203,9 +1202,6 @@ bhyveStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_ERROR; } - if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew())) - goto cleanup; - if (!(bhyve_driver->caps = virBhyveCapsBuild())) goto cleanup; diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index d46786d393..eee0c4bf1d 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -268,10 +268,8 @@ virBhyveProcessStart(virConnectPtr conn, if (bhyveProcessStartHook(vm, VIR_HOOK_BHYVE_OP_PREPARE) < 0) return -1; - if (flags & VIR_BHYVE_PROCESS_START_AUTODESTROY && - virCloseCallbacksSet(driver->closeCallbacks, vm, - conn, bhyveProcessAutoDestroy) < 0) - return -1; + if (flags & VIR_BHYVE_PROCESS_START_AUTODESTROY) + virCloseCallbacksDomainAdd(vm, conn, bhyveProcessAutoDestroy); if (bhyveProcessPrepareDomain(driver, vm, flags) < 0) return -1; @@ -325,8 +323,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, ret = 0; - virCloseCallbacksUnset(driver->closeCallbacks, vm, - bhyveProcessAutoDestroy); + virCloseCallbacksDomainRemove(vm, NULL, bhyveProcessAutoDestroy); virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); vm->pid = 0; diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h index 5d6e198b09..0680ae4cd1 100644 --- a/src/bhyve/bhyve_utils.h +++ b/src/bhyve/bhyve_utils.h @@ -59,8 +59,6 @@ struct _bhyveConn { virObjectEventState *domainEventState; - virCloseCallbacks *closeCallbacks; - virPortAllocatorRange *remotePorts; unsigned bhyvecaps; -- 2.38.1

The qemu driver uses connection close callbacks in more places requiring more changes than other drivers, but luckily the changes are very straightforward. The migration code was written in a way ensuring that there's just one callback present so this can be preserved directly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_conf.c | 17 +++++++++----- src/qemu/qemu_conf.h | 3 --- src/qemu/qemu_driver.c | 8 +------ src/qemu/qemu_migration.c | 47 +++++++++++++-------------------------- src/qemu/qemu_process.c | 35 ++++------------------------- src/qemu/qemu_process.h | 11 ++------- 6 files changed, 34 insertions(+), 87 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ae5bbcd138..0f10c5bf93 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1311,17 +1311,22 @@ virQEMUDriverCreateXMLConf(virQEMUDriver *driver, const char *defsecmodel) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainXMLOption *ret = NULL; virQEMUDriverDomainDefParserConfig.priv = driver; virQEMUDriverDomainDefParserConfig.defSecModel = defsecmodel; virQEMUDriverDomainJobConfig.maxQueuedJobs = cfg->maxQueuedJobs; - return virDomainXMLOptionNew(&virQEMUDriverDomainDefParserConfig, - &virQEMUDriverPrivateDataCallbacks, - &virQEMUDriverDomainXMLNamespace, - &virQEMUDriverDomainABIStability, - &virQEMUDriverDomainSaveCookie, - &virQEMUDriverDomainJobConfig); + ret = virDomainXMLOptionNew(&virQEMUDriverDomainDefParserConfig, + &virQEMUDriverPrivateDataCallbacks, + &virQEMUDriverDomainXMLNamespace, + &virQEMUDriverDomainABIStability, + &virQEMUDriverDomainSaveCookie, + &virQEMUDriverDomainJobConfig); + + virDomainXMLOptionSetCloseCallbackAlloc(ret, virCloseCallbacksDomainAlloc); + + return ret; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8cf2dd2ec5..b7ed00ca41 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -314,9 +314,6 @@ struct _virQEMUDriver { /* Immutable pointer. lockless access */ virLockManagerPlugin *lockManager; - /* Immutable pointer, self-clocking APIs */ - virCloseCallbacks *closeCallbacks; - /* Immutable pointer, self-locking APIs */ virHashAtomic *migrationErrors; }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b3da86c81..bd8c907567 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -846,9 +846,6 @@ qemuStateInitialize(bool privileged, 0, S_IXGRP | S_IXOTH) < 0) goto error; - if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew())) - goto error; - /* Get all the running persistent or transient configs first */ if (virDomainObjListLoadAllConfigs(qemu_driver->domains, cfg->stateDir, @@ -1053,7 +1050,6 @@ qemuStateCleanup(void) return -1; virObjectUnref(qemu_driver->migrationErrors); - virObjectUnref(qemu_driver->closeCallbacks); virLockManagerPluginUnref(qemu_driver->lockManager); virSysinfoDefFree(qemu_driver->hostsysinfo); virPortAllocatorRangeFree(qemu_driver->migrationPorts); @@ -1146,9 +1142,7 @@ static int qemuConnectClose(virConnectPtr conn) { virQEMUDriver *driver = conn->privateData; - /* Get rid of callbacks registered for this conn */ - virCloseCallbacksRun(driver->closeCallbacks, conn, driver->domains); - + virCloseCallbacksDomainRunForConn(driver->domains, conn); conn->privateData = NULL; return 0; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f4441d61ae..61a2bc39c4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1371,7 +1371,7 @@ qemuDomainGetMigrationBlockers(virDomainObj *vm, * false otherwise. */ bool -qemuMigrationSrcIsAllowed(virQEMUDriver *driver, +qemuMigrationSrcIsAllowed(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *vm, bool remote, int asyncJob, @@ -1437,7 +1437,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, return false; } - if (qemuProcessAutoDestroyActive(driver, vm)) { + if (virCloseCallbacksDomainIsRegistered(vm, NULL, qemuProcessAutoDestroy)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is marked for auto destroy")); return false; @@ -2735,7 +2735,7 @@ qemuMigrationSrcBeginResume(virDomainObj *vm, static char * qemuMigrationSrcBeginResumePhase(virConnectPtr conn, - virQEMUDriver *driver, + virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *vm, const char *xmlin, char **cookieout, @@ -2753,15 +2753,12 @@ qemuMigrationSrcBeginResumePhase(virConnectPtr conn, if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_BEGIN_RESUME) < 0) return NULL; - virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); xml = qemuMigrationSrcBeginResume(vm, xmlin, cookieout, cookieoutlen, flags); - if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationAnyConnectionClosed) < 0) - g_clear_pointer(&xml, g_free); + virCloseCallbacksDomainAdd(vm, conn, qemuMigrationAnyConnectionClosed); if (!xml) ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)); @@ -2837,9 +2834,7 @@ qemuMigrationSrcBegin(virConnectPtr conn, * This prevents any other APIs being invoked while migration is taking * place. */ - if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationAnyConnectionClosed) < 0) - goto endjob; + virCloseCallbacksDomainAdd(vm, conn, qemuMigrationAnyConnectionClosed); } ret = g_steal_pointer(&xml); @@ -3490,8 +3485,7 @@ qemuMigrationDstPrepareResume(virQEMUDriver *driver, QEMU_MIGRATION_COOKIE_CAPS) < 0) VIR_WARN("Unable to encode migration cookie"); - virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainAdd(vm, conn, qemuMigrationAnyConnectionClosed); if (autoPort) priv->migrationPort = port; @@ -4031,8 +4025,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, if (qemuMigrationJobStartPhase(vm, phase) < 0) goto cleanup; - virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); ret = qemuMigrationSrcConfirmPhase(driver, vm, @@ -5285,8 +5278,7 @@ qemuMigrationSrcPerformResume(virQEMUDriver *driver, if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PERFORM_RESUME) < 0) return -1; - virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); ret = qemuMigrationSrcPerformNative(driver, vm, NULL, uri, @@ -5294,9 +5286,7 @@ qemuMigrationSrcPerformResume(virQEMUDriver *driver, cookieout, cookieoutlen, flags, 0, NULL, NULL, 0, NULL, migParams, NULL); - if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationAnyConnectionClosed) < 0) - ret = -1; + virCloseCallbacksDomainAdd(vm, conn, qemuMigrationAnyConnectionClosed); if (ret < 0) ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)); @@ -6034,8 +6024,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_BEGIN_RESUME) < 0) goto cleanup; - virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); } else { if (qemuMigrationJobStart(vm, VIR_ASYNC_JOB_MIGRATION_OUT, @@ -6167,8 +6156,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PERFORM3) < 0) goto cleanup; - virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); if (qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, cookieout, cookieoutlen, @@ -6176,9 +6164,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, nmigrate_disks, migrate_disks, migParams, nbdURI) < 0) goto cleanup; - if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationAnyConnectionClosed) < 0) - goto cleanup; + virCloseCallbacksDomainAdd(vm, conn, qemuMigrationAnyConnectionClosed); ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE)); ret = 0; @@ -6434,7 +6420,7 @@ qemuMigrationDstComplete(virQEMUDriver *driver, * nothing to remove when we are resuming post-copy migration. */ if (job->phase < QEMU_MIGRATION_PHASE_POSTCOPY_FAILED) - qemuProcessAutoDestroyRemove(driver, vm); + virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); /* Remove completed stats for post-copy, everything but timing fields * is obsolete anyway. @@ -6727,7 +6713,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, if (virDomainObjIsFailedPostcopy(vm)) { ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)); - qemuProcessAutoDestroyRemove(driver, vm); + virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); *finishJob = false; } else { qemuMigrationParamsReset(vm, VIR_ASYNC_JOB_MIGRATION_IN, @@ -6787,8 +6773,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, goto cleanup; if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { - virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationAnyConnectionClosed); + virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); } else { qemuDomainCleanupRemove(vm, qemuMigrationDstPrepareCleanup); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b6adcf2f2a..fc4e58f95b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7901,9 +7901,8 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessRefreshBalloonState(vm, asyncJob) < 0) goto cleanup; - if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY && - qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) - goto cleanup; + if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY) + virCloseCallbacksDomainAdd(vm, conn, qemuProcessAutoDestroy); if (!incoming && !snapshot) { VIR_DEBUG("Setting up transient disk"); @@ -8381,7 +8380,7 @@ void qemuProcessStop(virQEMUDriver *driver, virFileDeleteTree(priv->channelTargetDir); /* Stop autodestroy in case guest is restarted */ - qemuProcessAutoDestroyRemove(driver, vm); + virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { @@ -8601,7 +8600,7 @@ void qemuProcessStop(virQEMUDriver *driver, } -static void +void qemuProcessAutoDestroy(virDomainObj *dom, virConnectPtr conn) { @@ -8641,32 +8640,6 @@ qemuProcessAutoDestroy(virDomainObj *dom, virObjectEventStateQueue(driver->domainEventState, event); } -int qemuProcessAutoDestroyAdd(virQEMUDriver *driver, - virDomainObj *vm, - virConnectPtr conn) -{ - VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn); - return virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuProcessAutoDestroy); -} - -int qemuProcessAutoDestroyRemove(virQEMUDriver *driver, - virDomainObj *vm) -{ - VIR_DEBUG("vm=%s", vm->def->name); - return virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuProcessAutoDestroy); -} - -bool qemuProcessAutoDestroyActive(virQEMUDriver *driver, - virDomainObj *vm) -{ - virCloseCallback cb; - VIR_DEBUG("vm=%s", vm->def->name); - cb = virCloseCallbacksGet(driver->closeCallbacks, vm, NULL); - return cb == qemuProcessAutoDestroy; -} - int qemuProcessRefreshDisks(virDomainObj *vm, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 9a24745f15..b171f0464c 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -168,15 +168,8 @@ int qemuProcessKill(virDomainObj *vm, unsigned int flags); void qemuProcessShutdownOrReboot(virDomainObj *vm); -int qemuProcessAutoDestroyInit(virQEMUDriver *driver); -void qemuProcessAutoDestroyShutdown(virQEMUDriver *driver); -int qemuProcessAutoDestroyAdd(virQEMUDriver *driver, - virDomainObj *vm, - virConnectPtr conn); -int qemuProcessAutoDestroyRemove(virQEMUDriver *driver, - virDomainObj *vm); -bool qemuProcessAutoDestroyActive(virQEMUDriver *driver, - virDomainObj *vm); +void qemuProcessAutoDestroy(virDomainObj *dom, + virConnectPtr conn); int qemuProcessSetSchedParams(int id, pid_t pid, size_t nsp, virDomainThreadSchedParam *sp); -- 2.38.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 8 +++----- src/qemu/qemu_migration.h | 3 +-- src/qemu/qemu_snapshot.c | 4 ++-- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bd8c907567..37bdc81378 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2624,7 +2624,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, VIR_DOMAIN_JOB_OPERATION_SAVE, flags) < 0) goto cleanup; - if (!qemuMigrationSrcIsAllowed(driver, vm, false, VIR_ASYNC_JOB_SAVE, 0)) + if (!qemuMigrationSrcIsAllowed(vm, false, VIR_ASYNC_JOB_SAVE, 0)) goto endjob; if (!virDomainObjIsActive(vm)) { @@ -3129,7 +3129,7 @@ doCoreDump(virQEMUDriver *driver, goto cleanup; } - if (!qemuMigrationSrcIsAllowed(driver, vm, false, VIR_ASYNC_JOB_DUMP, 0)) + if (!qemuMigrationSrcIsAllowed(vm, false, VIR_ASYNC_JOB_DUMP, 0)) goto cleanup; if (qemuMigrationSrcToFile(driver, vm, fd, compressor, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 61a2bc39c4..a561e1bc63 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1358,7 +1358,6 @@ qemuDomainGetMigrationBlockers(virDomainObj *vm, /** * qemuMigrationSrcIsAllowed: - * @driver: qemu driver struct * @vm: domain object * @remote: migration is remote * @flags: migration flags (see struct virDomainMigrateFlags) @@ -1371,8 +1370,7 @@ qemuDomainGetMigrationBlockers(virDomainObj *vm, * false otherwise. */ bool -qemuMigrationSrcIsAllowed(virQEMUDriver *driver G_GNUC_UNUSED, - virDomainObj *vm, +qemuMigrationSrcIsAllowed(virDomainObj *vm, bool remote, int asyncJob, unsigned int flags) @@ -2546,7 +2544,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_BEGIN3) < 0) return NULL; - if (!qemuMigrationSrcIsAllowed(driver, vm, true, vm->job->asyncJob, flags)) + if (!qemuMigrationSrcIsAllowed(vm, true, vm->job->asyncJob, flags)) return NULL; if (!(flags & (VIR_MIGRATE_UNSAFE | VIR_MIGRATE_OFFLINE)) && @@ -6034,7 +6032,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, if (!(flags & VIR_MIGRATE_OFFLINE) && virDomainObjCheckActive(vm) < 0) goto endjob; - if (!qemuMigrationSrcIsAllowed(driver, vm, true, VIR_ASYNC_JOB_MIGRATION_OUT, flags)) + if (!qemuMigrationSrcIsAllowed(vm, true, VIR_ASYNC_JOB_MIGRATION_OUT, flags)) goto endjob; if (!(flags & (VIR_MIGRATE_UNSAFE | VIR_MIGRATE_OFFLINE)) && diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 38a961f4e9..d21b6f67e8 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -226,8 +226,7 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver, qemuMonitorMigrationStatus status); bool -qemuMigrationSrcIsAllowed(virQEMUDriver *driver, - virDomainObj *vm, +qemuMigrationSrcIsAllowed(virDomainObj *vm, bool remote, int asyncJob, unsigned int flags); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d7983c134f..8b5bcd9770 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -291,7 +291,7 @@ qemuSnapshotCreateActiveInternal(virQEMUDriver *driver, virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); int ret = -1; - if (!qemuMigrationSrcIsAllowed(driver, vm, false, VIR_ASYNC_JOB_SNAPSHOT, 0)) + if (!qemuMigrationSrcIsAllowed(vm, false, VIR_ASYNC_JOB_SNAPSHOT, 0)) goto cleanup; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { @@ -1331,7 +1331,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, /* do the memory snapshot if necessary */ if (memory) { /* check if migration is possible */ - if (!qemuMigrationSrcIsAllowed(driver, vm, false, VIR_ASYNC_JOB_SNAPSHOT, 0)) + if (!qemuMigrationSrcIsAllowed(vm, false, VIR_ASYNC_JOB_SNAPSHOT, 0)) goto cleanup; qemuDomainJobSetStatsType(vm->job->current, -- 2.38.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a561e1bc63..54626bd851 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2733,7 +2733,6 @@ qemuMigrationSrcBeginResume(virDomainObj *vm, static char * qemuMigrationSrcBeginResumePhase(virConnectPtr conn, - virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *vm, const char *xmlin, char **cookieout, @@ -2792,7 +2791,7 @@ qemuMigrationSrcBegin(virConnectPtr conn, } if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { - ret = qemuMigrationSrcBeginResumePhase(conn, driver, vm, xmlin, + ret = qemuMigrationSrcBeginResumePhase(conn, vm, xmlin, cookieout, cookieoutlen, flags); goto cleanup; } -- 2.38.1

Now that all code was refactored to use the new version we can remove the old code. For now the new close callbacks code has no error messages so syntax-check forced me to remove the POTFILES entry for virclosecallbacks.c Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- po/POTFILES | 1 - src/hypervisor/virclosecallbacks.c | 282 ----------------------------- src/hypervisor/virclosecallbacks.h | 27 --- src/libvirt_private.syms | 5 - 4 files changed, 315 deletions(-) diff --git a/po/POTFILES b/po/POTFILES index 169e2a41dc..b2297be84e 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -91,7 +91,6 @@ src/hyperv/hyperv_util.c src/hyperv/hyperv_wmi.c src/hypervisor/domain_cgroup.c src/hypervisor/domain_driver.c -src/hypervisor/virclosecallbacks.c src/hypervisor/virhostdev.c src/interface/interface_backend_netcf.c src/interface/interface_backend_udev.c diff --git a/src/hypervisor/virclosecallbacks.c b/src/hypervisor/virclosecallbacks.c index 21b97cce12..4fad4c8288 100644 --- a/src/hypervisor/virclosecallbacks.c +++ b/src/hypervisor/virclosecallbacks.c @@ -29,288 +29,6 @@ VIR_LOG_INIT("util.closecallbacks"); -typedef struct _virDriverCloseDef virDriverCloseDef; -struct _virDriverCloseDef { - virConnectPtr conn; - virCloseCallback cb; -}; - -struct _virCloseCallbacks { - virObjectLockable parent; - - /* UUID string to qemuDriverCloseDef mapping */ - GHashTable *list; -}; - - -static virClass *virCloseCallbacksClass; -static void virCloseCallbacksDispose(void *obj); - -static int virCloseCallbacksOnceInit(void) -{ - if (!VIR_CLASS_NEW(virCloseCallbacks, virClassForObjectLockable())) - return -1; - - return 0; -} - -VIR_ONCE_GLOBAL_INIT(virCloseCallbacks); - - -virCloseCallbacks * -virCloseCallbacksNew(void) -{ - virCloseCallbacks *closeCallbacks; - - if (virCloseCallbacksInitialize() < 0) - return NULL; - - if (!(closeCallbacks = virObjectLockableNew(virCloseCallbacksClass))) - return NULL; - - closeCallbacks->list = virHashNew(g_free); - - return closeCallbacks; -} - -static void -virCloseCallbacksDispose(void *obj) -{ - virCloseCallbacks *closeCallbacks = obj; - - g_clear_pointer(&closeCallbacks->list, g_hash_table_unref); -} - -int -virCloseCallbacksSet(virCloseCallbacks *closeCallbacks, - virDomainObj *vm, - virConnectPtr conn, - virCloseCallback cb) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virDriverCloseDef *closeDef; - int ret = -1; - - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p", - vm->def->name, uuidstr, conn, cb); - - virObjectLock(closeCallbacks); - - closeDef = virHashLookup(closeCallbacks->list, uuidstr); - if (closeDef) { - if (closeDef->conn != conn) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Close callback for domain %s already registered" - " with another connection %p"), - vm->def->name, closeDef->conn); - goto cleanup; - } - if (closeDef->cb && closeDef->cb != cb) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Another close callback is already defined for" - " domain %s"), vm->def->name); - goto cleanup; - } - - closeDef->cb = cb; - } else { - closeDef = g_new0(virDriverCloseDef, 1); - closeDef->conn = conn; - closeDef->cb = cb; - if (virHashAddEntry(closeCallbacks->list, uuidstr, closeDef) < 0) { - VIR_FREE(closeDef); - goto cleanup; - } - virObjectRef(vm); - } - - virObjectRef(closeCallbacks); - ret = 0; - cleanup: - virObjectUnlock(closeCallbacks); - return ret; -} - -int -virCloseCallbacksUnset(virCloseCallbacks *closeCallbacks, - virDomainObj *vm, - virCloseCallback cb) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virDriverCloseDef *closeDef; - int ret = -1; - - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s, uuid=%s, cb=%p", - vm->def->name, uuidstr, cb); - - virObjectLock(closeCallbacks); - - closeDef = virHashLookup(closeCallbacks->list, uuidstr); - if (!closeDef) - goto cleanup; - - if (closeDef->cb && closeDef->cb != cb) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Trying to remove mismatching close callback for" - " domain %s"), vm->def->name); - goto cleanup; - } - - if (virHashRemoveEntry(closeCallbacks->list, uuidstr) < 0) - goto cleanup; - - virObjectUnref(vm); - ret = 0; - cleanup: - virObjectUnlock(closeCallbacks); - if (!ret) - virObjectUnref(closeCallbacks); - return ret; -} - -virCloseCallback -virCloseCallbacksGet(virCloseCallbacks *closeCallbacks, - virDomainObj *vm, - virConnectPtr conn) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virDriverCloseDef *closeDef; - virCloseCallback cb = NULL; - - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s, uuid=%s, conn=%p", - vm->def->name, uuidstr, conn); - - virObjectLock(closeCallbacks); - - closeDef = virHashLookup(closeCallbacks->list, uuidstr); - if (closeDef && (!conn || closeDef->conn == conn)) - cb = closeDef->cb; - - virObjectUnlock(closeCallbacks); - - VIR_DEBUG("cb=%p", cb); - return cb; -} - -typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry; -struct _virCloseCallbacksListEntry { - unsigned char uuid[VIR_UUID_BUFLEN]; - virCloseCallback callback; -}; - -typedef struct _virCloseCallbacksList virCloseCallbacksList; -struct _virCloseCallbacksList { - size_t nentries; - virCloseCallbacksListEntry *entries; -}; - -struct virCloseCallbacksData { - virConnectPtr conn; - virCloseCallbacksList *list; -}; - -static int -virCloseCallbacksGetOne(void *payload, - const char *key, - void *opaque) -{ - struct virCloseCallbacksData *data = opaque; - virDriverCloseDef *closeDef = payload; - const char *uuidstr = key; - unsigned char uuid[VIR_UUID_BUFLEN]; - - if (virUUIDParse(uuidstr, uuid) < 0) - return 0; - - VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", - closeDef->conn, data->conn, uuidstr, closeDef->cb); - - if (data->conn != closeDef->conn || !closeDef->cb) - return 0; - - VIR_EXPAND_N(data->list->entries, data->list->nentries, 1); - - memcpy(data->list->entries[data->list->nentries - 1].uuid, - uuid, VIR_UUID_BUFLEN); - data->list->entries[data->list->nentries - 1].callback = closeDef->cb; - return 0; -} - -static virCloseCallbacksList * -virCloseCallbacksGetForConn(virCloseCallbacks *closeCallbacks, - virConnectPtr conn) -{ - virCloseCallbacksList *list = NULL; - struct virCloseCallbacksData data; - - list = g_new0(virCloseCallbacksList, 1); - - data.conn = conn; - data.list = list; - - virHashForEach(closeCallbacks->list, virCloseCallbacksGetOne, &data); - - return list; -} - - -void -virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, - virConnectPtr conn, - virDomainObjList *domains) -{ - virCloseCallbacksList *list; - size_t i; - - VIR_DEBUG("conn=%p", conn); - - /* We must not hold the lock while running the callbacks, - * so first we obtain the list of callbacks, then remove - * them all from the hash. At that point we can release - * the lock and run the callbacks safely. */ - - virObjectLock(closeCallbacks); - list = virCloseCallbacksGetForConn(closeCallbacks, conn); - if (!list) { - virObjectUnlock(closeCallbacks); - return; - } - - for (i = 0; i < list->nentries; i++) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(list->entries[i].uuid, uuidstr); - virHashRemoveEntry(closeCallbacks->list, uuidstr); - } - virObjectUnlock(closeCallbacks); - - for (i = 0; i < list->nentries; i++) { - virDomainObj *vm; - - /* Grab a ref and lock to the vm */ - if (!(vm = virDomainObjListFindByUUID(domains, - list->entries[i].uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(list->entries[i].uuid, uuidstr); - VIR_DEBUG("No domain object with UUID %s", uuidstr); - continue; - } - - /* Remove the ref taken out during virCloseCallbacksSet since - * we're about to call the callback function and we have another - * ref anyway (so it cannot be deleted). - * - * Call the callback function and end the API usage. */ - virObjectUnref(vm); - list->entries[i].callback(vm, conn); - virDomainObjEndAPI(&vm); - } - VIR_FREE(list->entries); - VIR_FREE(list); -} - struct _virCloseCallbacksDomainData { virConnectPtr conn; diff --git a/src/hypervisor/virclosecallbacks.h b/src/hypervisor/virclosecallbacks.h index b471f6b160..893292763f 100644 --- a/src/hypervisor/virclosecallbacks.h +++ b/src/hypervisor/virclosecallbacks.h @@ -22,36 +22,9 @@ #include "conf/virdomainobjlist.h" -typedef struct _virCloseCallbacks virCloseCallbacks; - typedef void (*virCloseCallback)(virDomainObj *vm, virConnectPtr conn); -virCloseCallbacks * -virCloseCallbacksNew(void); - -int -virCloseCallbacksSet(virCloseCallbacks *closeCallbacks, - virDomainObj *vm, - virConnectPtr conn, - virCloseCallback cb); -int -virCloseCallbacksUnset(virCloseCallbacks *closeCallbacks, - virDomainObj *vm, - virCloseCallback cb); - -virCloseCallback -virCloseCallbacksGet(virCloseCallbacks *closeCallbacks, - virDomainObj *vm, - virConnectPtr conn); - -void -virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, - virConnectPtr conn, - virDomainObjList *domains); - -/* ---- */ - virObject * virCloseCallbacksDomainAlloc(void); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fa23729a..b81c2cc7da 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1612,11 +1612,6 @@ virCloseCallbacksDomainAlloc; virCloseCallbacksDomainIsRegistered; virCloseCallbacksDomainRemove; virCloseCallbacksDomainRunForConn; -virCloseCallbacksGet; -virCloseCallbacksNew; -virCloseCallbacksRun; -virCloseCallbacksSet; -virCloseCallbacksUnset; # hypervisor/virhostdev.h -- 2.38.1

Require check of return value of the ACL checking functions. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/rpc/gendispatch.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 5f2b163ea0..54d55d91e7 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -2182,7 +2182,7 @@ elsif ($mode eq "client") { } if ($mode eq "aclheader") { - print "extern $ret $apiname(" . join(", ", @argdecls) . ");\n"; + print "extern $ret $apiname(" . join(", ", @argdecls) . ") G_GNUC_WARN_UNUSED_RESULT;\n"; } else { my @argvars; push @argvars, "mgr"; -- 2.38.1

The function currently didn't have a return value. Returning the 'virLockGuard' struct allows the callers to use automatic unlocking of the mutex. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/remote/remote_driver.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b0dba9057b..8310326ad0 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -105,9 +105,17 @@ enum { }; -static void remoteDriverLock(struct private_data *driver) +/** + * remoteDriverLock: + * @driver: private data of the remote driver + * + * Locks the internal mutex of the private driver. Callers may optionally use + * the returned virLockGuard struct to automatically unlock the driver. + */ +static virLockGuard +remoteDriverLock(struct private_data *driver) { - virMutexLock(&driver->lock); + return virLockGuardLock(&driver->lock); } static void remoteDriverUnlock(struct private_data *driver) -- 2.38.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/remote/remote_driver.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8310326ad0..7946e9d9bd 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7978,12 +7978,10 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol, virStorageVolInfoPtr result, unsigned int flags) { - int rv = -1; struct private_data *priv = vol->conn->privateData; remote_storage_vol_get_info_flags_args args; remote_storage_vol_get_info_flags_ret ret; - - remoteDriverLock(priv); + VIR_LOCK_GUARD lock = remoteDriverLock(priv); make_nonnull_storage_vol(&args.vol, vol); args.flags = flags; @@ -7994,18 +7992,14 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol, (xdrproc_t)xdr_remote_storage_vol_get_info_flags_args, (char *)&args, (xdrproc_t)xdr_remote_storage_vol_get_info_flags_ret, - (char *)&ret) == -1) { - goto done; - } + (char *)&ret) == -1) + return -1; result->type = ret.type; result->capacity = ret.capacity; result->allocation = ret.allocation; - rv = 0; - done: - remoteDriverUnlock(priv); - return rv; + return 0; } @@ -8143,17 +8137,15 @@ remoteDomainAuthorizedSSHKeysSet(virDomainPtr domain, unsigned int nkeys, unsigned int flags) { - int rv = -1; struct private_data *priv = domain->conn->privateData; remote_domain_authorized_ssh_keys_set_args args; - - remoteDriverLock(priv); + VIR_LOCK_GUARD lock = remoteDriverLock(priv); if (nkeys > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) { virReportError(VIR_ERR_RPC, "%s", _("remoteDomainAuthorizedSSHKeysSet: " "returned number of keys exceeds limit")); - goto cleanup; + return -1; } make_nonnull_domain(&args.dom, domain); @@ -8164,15 +8156,10 @@ remoteDomainAuthorizedSSHKeysSet(virDomainPtr domain, if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET, (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_set_args, (char *)&args, - (xdrproc_t) xdr_void, (char *) NULL) == -1) { - goto cleanup; - } - - rv = 0; + (xdrproc_t) xdr_void, (char *) NULL) == -1) + return -1; - cleanup: - remoteDriverUnlock(priv); - return rv; + return 0; } -- 2.38.1

Convert to a switch instead of a bunch of 'if (type == ...). Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/storage_source_conf.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 2b4cf5e241..84df8d28fa 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -929,7 +929,8 @@ virStorageSourceIsSameLocation(virStorageSource *a, STRNEQ_NULLABLE(a->snapshot, b->snapshot)) return false; - if (a->type == VIR_STORAGE_TYPE_NETWORK) { + switch ((virStorageType) virStorageSourceGetActualType(a)) { + case VIR_STORAGE_TYPE_NETWORK: if (a->protocol != b->protocol || a->nhosts != b->nhosts) return false; @@ -941,11 +942,23 @@ virStorageSourceIsSameLocation(virStorageSource *a, STRNEQ_NULLABLE(a->hosts[i].socket, b->hosts[i].socket)) return false; } - } + break; - if (a->type == VIR_STORAGE_TYPE_NVME && - !virStorageSourceNVMeDefIsEqual(a->nvme, b->nvme)) - return false; + case VIR_STORAGE_TYPE_NVME: + if (!virStorageSourceNVMeDefIsEqual(a->nvme, b->nvme)) + return false; + break; + + case VIR_STORAGE_TYPE_VHOST_USER: + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_LAST: + case VIR_STORAGE_TYPE_VOLUME: + /* nothing to do */ + break; + } return true; } -- 2.38.1

Commit da9f3cd84b250088 added the seclabel example into the 'disk-backing-chains' case. Since the only thing that 'disk-backing-chains' tests which 'disk-backing-chains-(no)index' don't test is the seclabel we'll be able to remove the test case if we add the seclabel example. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemuxml2argvdata/disk-backing-chains-index.xml | 6 +++++- tests/qemuxml2argvdata/disk-backing-chains-noindex.xml | 6 +++++- .../qemuxml2xmloutdata/disk-backing-chains-index-active.xml | 6 +++++- .../disk-backing-chains-index-inactive.xml | 6 +++++- tests/qemuxml2xmloutdata/disk-backing-chains-noindex.xml | 6 +++++- 5 files changed, 25 insertions(+), 5 deletions(-) diff --git a/tests/qemuxml2argvdata/disk-backing-chains-index.xml b/tests/qemuxml2argvdata/disk-backing-chains-index.xml index 9054963f1f..90da6a97e9 100644 --- a/tests/qemuxml2argvdata/disk-backing-chains-index.xml +++ b/tests/qemuxml2argvdata/disk-backing-chains-index.xml @@ -38,7 +38,11 @@ <source file='/tmp/image2'/> <backingStore type='file' index='6'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file' index='5'> <format type='qcow2'/> <source file='/tmp/image4.qcow'> diff --git a/tests/qemuxml2argvdata/disk-backing-chains-noindex.xml b/tests/qemuxml2argvdata/disk-backing-chains-noindex.xml index ca7dc2d2b3..dc928f7075 100644 --- a/tests/qemuxml2argvdata/disk-backing-chains-noindex.xml +++ b/tests/qemuxml2argvdata/disk-backing-chains-noindex.xml @@ -38,7 +38,11 @@ <source file='/tmp/image2'/> <backingStore type='file'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file'> <format type='qcow2'/> <source file='/tmp/image4.qcow'> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-index-active.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-index-active.xml index ca7684d3b6..4f30bc7111 100644 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-index-active.xml +++ b/tests/qemuxml2xmloutdata/disk-backing-chains-index-active.xml @@ -39,7 +39,11 @@ <source file='/tmp/image2'/> <backingStore type='file' index='6'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file' index='5'> <format type='qcow2'/> <source file='/tmp/image4.qcow'/> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-index-inactive.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-index-inactive.xml index 74e7a74b48..c992679b36 100644 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-index-inactive.xml +++ b/tests/qemuxml2xmloutdata/disk-backing-chains-index-inactive.xml @@ -39,7 +39,11 @@ <source file='/tmp/image2'/> <backingStore type='file'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file'> <format type='qcow2'/> <source file='/tmp/image4.qcow'/> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-noindex.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-noindex.xml index 74e7a74b48..c992679b36 100644 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-noindex.xml +++ b/tests/qemuxml2xmloutdata/disk-backing-chains-noindex.xml @@ -39,7 +39,11 @@ <source file='/tmp/image2'/> <backingStore type='file'> <format type='qcow2'/> - <source file='/tmp/image3.qcow'/> + <source file='/tmp/image3.qcow'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <backingStore type='file'> <format type='qcow2'/> <source file='/tmp/image4.qcow'/> -- 2.38.1

The test is superseded by 'disk-backing-chains-(no)index' cases. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- .../qemuxml2argvdata/disk-backing-chains.xml | 98 ---------------- .../disk-backing-chains-active.xml | 110 ------------------ .../disk-backing-chains-inactive.xml | 110 ------------------ tests/qemuxml2xmltest.c | 1 - 4 files changed, 319 deletions(-) delete mode 100644 tests/qemuxml2argvdata/disk-backing-chains.xml delete mode 100644 tests/qemuxml2xmloutdata/disk-backing-chains-active.xml delete mode 100644 tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml diff --git a/tests/qemuxml2argvdata/disk-backing-chains.xml b/tests/qemuxml2argvdata/disk-backing-chains.xml deleted file mode 100644 index 7fe84da3f0..0000000000 --- a/tests/qemuxml2argvdata/disk-backing-chains.xml +++ /dev/null @@ -1,98 +0,0 @@ -<domain type='qemu' id='1'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219136</memory> - <currentMemory unit='KiB'>219136</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-i386</emulator> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <source protocol='gluster' name='Volume2/Image'> - <host transport='unix' socket='/path/to/sock'/> - </source> - <backingStore type='file' index='1'> - <format type='qcow2'/> - <source file='/tmp/missing-backing-store.qcow'/> - </backingStore> - <target dev='vda' bus='virtio'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <source protocol='nbd' name='bar'> - <host transport='unix' socket='/var/run/nbdsock'/> - </source> - <backingStore type='block' index='1'> - <format type='qcow2'/> - <source dev='/dev/HostVG/QEMUGuest1'/> - <backingStore type='file' index='2'> - <format type='qcow2'/> - <source file='/tmp/image2.qcow'/> - <backingStore type='file' index='3'> - <format type='qcow2'/> - <source file='/tmp/image3.qcow'> - <seclabel model='selinux' relabel='yes'> - <label>system_u:system_r:public_content_t:s0</label> - </seclabel> - </source> - <backingStore type='file' index='4'> - <format type='qcow2'/> - <source file='/tmp/image4.qcow'/> - <backingStore type='file' index='5'> - <source file='/tmp/image5.qcow'/> - <format type='qcow2'/> - <backingStore type='file' index='6'> - <format type='raw'/> - <source file='/tmp/Fedora-17-x86_64-Live-KDE.iso'/> - <backingStore/> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - <target dev='vdb' bus='virtio'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='raw'/> - <backingStore/> - <source protocol='gluster' name='Volume1/Image'> - <host name='example.org' port='6000'/> - </source> - <target dev='vdc' bus='virtio'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <auth username='myname'> - <secret type='ceph' usage='mycluster_myname'/> - </auth> - <source protocol='rbd' name='pool/image'> - <host name='mon1.example.org' port='6321'/> - <host name='mon2.example.org' port='6322'/> - <host name='mon3.example.org' port='6322'/> - </source> - <backingStore type='file' index='1'> - <source file='/tmp/image.qcow'/> - <backingStore/> - <format type='qcow2'/> - </backingStore> - <target dev='vdd' bus='virtio'/> - </disk> - <disk type='block' device='disk'> - <driver name='qemu' type='qcow2'/> - <source dev='/dev/HostVG/QEMUGuest11'/> - <target dev='vde' bus='virtio'/> - </disk> - <controller type='usb' index='0'/> - <controller type='pci' index='0' model='pci-root'/> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml deleted file mode 100644 index 5901aef3d3..0000000000 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-active.xml +++ /dev/null @@ -1,110 +0,0 @@ -<domain type='qemu' id='1'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219136</memory> - <currentMemory unit='KiB'>219136</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-i386</emulator> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <source protocol='gluster' name='Volume2/Image'> - <host transport='unix' socket='/path/to/sock'/> - </source> - <backingStore type='file' index='1'> - <format type='qcow2'/> - <source file='/tmp/missing-backing-store.qcow'/> - </backingStore> - <target dev='vda' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <source protocol='nbd' name='bar'> - <host transport='unix' socket='/var/run/nbdsock'/> - </source> - <backingStore type='block' index='1'> - <format type='qcow2'/> - <source dev='/dev/HostVG/QEMUGuest1'/> - <backingStore type='file' index='2'> - <format type='qcow2'/> - <source file='/tmp/image2.qcow'/> - <backingStore type='file' index='3'> - <format type='qcow2'/> - <source file='/tmp/image3.qcow'> - <seclabel model='selinux' relabel='yes'> - <label>system_u:system_r:public_content_t:s0</label> - </seclabel> - </source> - <backingStore type='file' index='4'> - <format type='qcow2'/> - <source file='/tmp/image4.qcow'/> - <backingStore type='file' index='5'> - <format type='qcow2'/> - <source file='/tmp/image5.qcow'/> - <backingStore type='file' index='6'> - <format type='raw'/> - <source file='/tmp/Fedora-17-x86_64-Live-KDE.iso'/> - <backingStore/> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - <target dev='vdb' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='raw'/> - <source protocol='gluster' name='Volume1/Image'> - <host name='example.org' port='6000'/> - </source> - <backingStore/> - <target dev='vdc' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <auth username='myname'> - <secret type='ceph' usage='mycluster_myname'/> - </auth> - <source protocol='rbd' name='pool/image'> - <host name='mon1.example.org' port='6321'/> - <host name='mon2.example.org' port='6322'/> - <host name='mon3.example.org' port='6322'/> - </source> - <backingStore type='file' index='1'> - <format type='qcow2'/> - <source file='/tmp/image.qcow'/> - <backingStore/> - </backingStore> - <target dev='vdd' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> - </disk> - <disk type='block' device='disk'> - <driver name='qemu' type='qcow2'/> - <source dev='/dev/HostVG/QEMUGuest11'/> - <target dev='vde' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> - </disk> - <controller type='usb' index='0'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> - </controller> - <controller type='pci' index='0' model='pci-root'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <audio id='1' type='none'/> - <memballoon model='virtio'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> - </memballoon> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml b/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml deleted file mode 100644 index 1d9adc85ba..0000000000 --- a/tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml +++ /dev/null @@ -1,110 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219136</memory> - <currentMemory unit='KiB'>219136</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-i386</emulator> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <source protocol='gluster' name='Volume2/Image'> - <host transport='unix' socket='/path/to/sock'/> - </source> - <backingStore type='file'> - <format type='qcow2'/> - <source file='/tmp/missing-backing-store.qcow'/> - </backingStore> - <target dev='vda' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <source protocol='nbd' name='bar'> - <host transport='unix' socket='/var/run/nbdsock'/> - </source> - <backingStore type='block'> - <format type='qcow2'/> - <source dev='/dev/HostVG/QEMUGuest1'/> - <backingStore type='file'> - <format type='qcow2'/> - <source file='/tmp/image2.qcow'/> - <backingStore type='file'> - <format type='qcow2'/> - <source file='/tmp/image3.qcow'> - <seclabel model='selinux' relabel='yes'> - <label>system_u:system_r:public_content_t:s0</label> - </seclabel> - </source> - <backingStore type='file'> - <format type='qcow2'/> - <source file='/tmp/image4.qcow'/> - <backingStore type='file'> - <format type='qcow2'/> - <source file='/tmp/image5.qcow'/> - <backingStore type='file'> - <format type='raw'/> - <source file='/tmp/Fedora-17-x86_64-Live-KDE.iso'/> - <backingStore/> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - </backingStore> - <target dev='vdb' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='raw'/> - <source protocol='gluster' name='Volume1/Image'> - <host name='example.org' port='6000'/> - </source> - <backingStore/> - <target dev='vdc' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> - </disk> - <disk type='network' device='disk'> - <driver name='qemu' type='qcow2'/> - <auth username='myname'> - <secret type='ceph' usage='mycluster_myname'/> - </auth> - <source protocol='rbd' name='pool/image'> - <host name='mon1.example.org' port='6321'/> - <host name='mon2.example.org' port='6322'/> - <host name='mon3.example.org' port='6322'/> - </source> - <backingStore type='file'> - <format type='qcow2'/> - <source file='/tmp/image.qcow'/> - <backingStore/> - </backingStore> - <target dev='vdd' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> - </disk> - <disk type='block' device='disk'> - <driver name='qemu' type='qcow2'/> - <source dev='/dev/HostVG/QEMUGuest11'/> - <target dev='vde' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> - </disk> - <controller type='usb' index='0'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> - </controller> - <controller type='pci' index='0' model='pci-root'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <audio id='1' type='none'/> - <memballoon model='virtio'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> - </memballoon> - </devices> -</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0e729ca905..8d885d6d9f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -906,7 +906,6 @@ mymain(void) DO_TEST("panic-no-address", QEMU_CAPS_DEVICE_PANIC); DO_TEST_CAPS_ARCH_LATEST("panic-pseries", "ppc64"); - DO_TEST_NOCAPS("disk-backing-chains"); DO_TEST_NOCAPS("disk-backing-chains-index"); DO_TEST_NOCAPS("disk-backing-chains-noindex"); -- 2.38.1

The API can be used to associate one or more (e.g. a RO and RW fd for a disk backend image) FDs to a VM. They can be then used per definition. The primary use case for now is for complex deployment where libvirtd/virtqemud may be run inside a container and getting the image into the container is complicated. In the future it will also allow passing e.g. vhost FDs and other resources to a VM without the need to have a filesystem representation for it. Passing raw FDs has few intricacies and thus libvirt will by default not restore security labels. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 22 ++++++++ src/driver-hypervisor.h | 8 +++ src/libvirt-domain.c | 82 +++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++ src/remote/remote_daemon_dispatch.c | 40 ++++++++++++++ src/remote/remote_driver.c | 27 ++++++++++ src/remote/remote_protocol.x | 14 ++++- src/remote_protocol-structs | 6 +++ 8 files changed, 203 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 295fd30c93..a1e39f2f70 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -6457,4 +6457,26 @@ int virDomainStartDirtyRateCalc(virDomainPtr domain, int seconds, unsigned int flags); + +/** + * virDomainFDAssociateFlags: + * + * Since: 9.0.0 + */ +typedef enum { + /* Attempt a best-effort restore of security labels after use (Since: 9.0.0) */ + VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE = (1 << 0), + /* Require mandatory restore of security labels after use (Since: 9.0.0) */ + VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE_REQUIRE = (1 << 1), + /* Use a seclabel allowing writes for the FD even if usage implies read-only mode (Since: 9.0.0) */ + VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_WRITABLE = (1 << 2), +} virDomainFDAssociateFlags; + + +int virDomainFDAssociate(virDomainPtr domain, + const char *name, + unsigned int nfds, + int *fds, + unsigned int flags); + #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 016d5cec7c..5219344b72 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1441,6 +1441,13 @@ typedef int int seconds, unsigned int flags); +typedef int +(*virDrvDomainFDAssociate)(virDomainPtr domain, + const char *name, + unsigned int nfds, + int *fds, + unsigned int flags); + typedef struct _virHypervisorDriver virHypervisorDriver; /** @@ -1712,4 +1719,5 @@ struct _virHypervisorDriver { virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet; virDrvDomainGetMessages domainGetMessages; virDrvDomainStartDirtyRateCalc domainStartDirtyRateCalc; + virDrvDomainFDAssociate domainFDAssociate; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 78c26b2219..12056d7ce2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13972,3 +13972,85 @@ virDomainStartDirtyRateCalc(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainFDAssociate: + * @domain: a domain object + * @name: name for the file descriptor group + * @nfds: number of fds in @fds + * @fds: file descriptors to associate with domain + * @flags: optional flags; bitwise-OR of supported virDomainFDAssociateFlags + * + * Associate the FDs in @fd with @domain under @name. The FDs are associated as + * long as the connection used to associated exists and are disposed of + * afterwards. FD may still be kept open by the hypervisor for as long as it's + * needed. + * + * Security labelling (e.g. via the selinux) may be applied on the passed FDs + * when required for usage by the VM. By default libvirt does not restore the + * seclabels on the FDs afterwards to avoid keeping it open unnecessarily. + * + * Restoring of the security label can be requested by passing either + * VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE for a best-effort attempt to restore + * the security label after use, and + * VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE_REQUIRE to request a mandatory + * restore in all cases. Note that hypervisors currently don't support the + * VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE_REQUIRE flag. Requesting the restore + * of security label will require that the file descriptors are kept open for + * the whole time they are used by the hypervisor, or other additional overhead. + * + * In certain cases usage of the fd group would imply read-only access. Passing + * VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_WRITABLE in @flags ensures that a writable + * security label is picked in case when the file represented by the fds may + * be used in write mode. + * + * Returns 0 on success, -1 on error. + * + * Since: 9.0.0 + */ +int +virDomainFDAssociate(virDomainPtr domain, + const char *name, + unsigned int nfds, + int *fds, + unsigned int flags) +{ + virConnectPtr conn; + int rc; + + VIR_DOMAIN_DEBUG(domain, + "name='%s', nfds=%u, fds=%p, flags=0x%x", + name, nfds, fds, flags); + + virResetLastError(); + + conn = domain->conn; + + if ((rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, VIR_DRV_FEATURE_FD_PASSING)) < 0) + goto error; + + if (rc == 0) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("fd passing is not supported by this connection")); + goto error; + } + + virCheckNonZeroArgGoto(nfds, error); + virCheckNonNullArgGoto(fds, error); + virCheckReadOnlyGoto(conn->flags, error); + + if (!conn->driver->domainFDAssociate) { + virReportUnsupportedError(); + goto error; + } + + if ((rc = conn->driver->domainFDAssociate(domain, name, nfds, fds, flags)) < 0) + goto error; + + return rc; + + error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 297a2c436a..80742f268e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -927,4 +927,9 @@ LIBVIRT_8.5.0 { virDomainAbortJobFlags; } LIBVIRT_8.4.0; +LIBVIRT_9.0.0 { + global: + virDomainFDAssociate; +} LIBVIRT_8.5.0; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 7efe58b36b..40c734ce6b 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -7443,3 +7443,43 @@ remoteDispatchDomainGetMessages(virNetServer *server G_GNUC_UNUSED, return rv; } + + +static int +remoteDispatchDomainFdAssociate(virNetServer *server G_GNUC_UNUSED, + virNetServerClient *client, + virNetMessage *msg, + struct virNetMessageError *rerr, + remote_domain_fd_associate_args *args) +{ + virDomainPtr dom = NULL; + int *fds = NULL; + unsigned int nfds = 0; + int rv = -1; + virConnectPtr conn = remoteGetHypervisorConn(client); + size_t i; + + if (!conn) + goto cleanup; + + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + + fds = g_new0(int, msg->nfds); + for (i = 0; i < msg->nfds; i++) { + if ((fds[i] = virNetMessageDupFD(msg, i)) < 0) + goto cleanup; + nfds++; + } + + if (virDomainFDAssociate(dom, args->name, nfds, fds, args->flags) < 0) + goto cleanup; + + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virObjectUnref(dom); + return rv; +} diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7946e9d9bd..2c7c17c6c9 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8206,6 +8206,32 @@ remoteDomainGetMessages(virDomainPtr domain, return rv; } + +static int +remoteDomainFDAssociate(virDomainPtr domain, + const char *name, + unsigned int nfds, + int *fds, + unsigned int flags) +{ + remote_domain_fd_associate_args args; + struct private_data *priv = domain->conn->privateData; + VIR_LOCK_GUARD lock = remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.name = (char *)name; + args.flags = flags; + + if (callFull(domain->conn, priv, 0, fds, nfds, NULL, NULL, + REMOTE_PROC_DOMAIN_FD_ASSOCIATE, + (xdrproc_t) xdr_remote_domain_fd_associate_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + return -1; + + return 0; +} + + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -8646,6 +8672,7 @@ static virHypervisorDriver hypervisor_driver = { .domainGetMessages = remoteDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = remoteDomainStartDirtyRateCalc, /* 7.2.0 */ .domainSetLaunchSecurityState = remoteDomainSetLaunchSecurityState, /* 8.0.0 */ + .domainFDAssociate = remoteDomainFDAssociate, /* 8.9.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 7dfb4548f4..c34d6f189d 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3929,6 +3929,12 @@ struct remote_domain_event_memory_device_size_change_msg { unsigned hyper size; }; + +struct remote_domain_fd_associate_args { + remote_nonnull_domain dom; + remote_nonnull_string name; + unsigned int flags; +}; /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6961,5 +6967,11 @@ enum remote_procedure { * @generate: both * @acl: domain:write */ - REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442 + REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442, + + /** + * @generate: none + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index ca5222439d..3c6c230a16 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3268,6 +3268,11 @@ struct remote_domain_event_memory_device_size_change_msg { remote_nonnull_string alias; uint64_t size; }; +struct remote_domain_fd_associate_args { + remote_nonnull_domain dom; + remote_nonnull_string name; + u_int flags; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3711,4 +3716,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SAVE_PARAMS = 440, REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441, REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442, + REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443, }; -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:09PM +0100, Peter Krempa wrote: Do we need to introduce flag that is not supported by any hypervisor? It should be perfectly fine to introduce the flag once there is actual usage for it or am I missing something? Pavel

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/manpages/virsh.rst | 22 +++++++++++ tools/virsh-domain.c | 83 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index c85bc8151d..7e57796384 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5225,6 +5225,28 @@ If *--print-xml* is specified, the XML that would be used to change media is printed instead of changing the media. +dom-fd-associate +---------------- + +**Syntax:** + +:: + + dom-fd-associate domain --name FDGROUPNAME --pass-fds M,N,.... + [--seclabel-writable] [--seclabel-restore] [--seclabel-restore-require] + +Associate one or more fds described via *--pass-fds* argument to *domain* as +*--name*. The lifetime of the passed fd group is the same as the connection, thus +exitting virsh un-registers them afterwards. + +By default security labels are applied if needed but they are not restored after +use to avoid keeping them open unnecessarily. Best-effort security label restore +may be requested by using the *--seclabel-restore* flag. + +Passing *--seclabel-restore-require* instructs the hypervisor to try harder to +restore security labels. **Note:** Hypervisors currently don't support this flag. + + NODEDEV COMMANDS ================ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2d162cf8c0..5cbbb4bd28 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9816,6 +9816,83 @@ cmdDomSetLaunchSecState(vshControl * ctl, const vshCmd * cmd) return ret; } + +/* + * "dom-fd-associate" command + */ +static const vshCmdInfo info_dom_fd_associate[] = { + {.name = "help", + .data = N_("associate a FD with a domain") + }, + {.name = "desc", + .data = N_("associate a FD with a domain") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_dom_fd_associate[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "name", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .completer = virshCompleteEmpty, + .help = N_("name of the FD group") + }, + {.name = "pass-fds", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .completer = virshCompleteEmpty, + .help = N_("file descriptors N,M,... to associate") + }, + {.name = "seclabel-writable", + .type = VSH_OT_BOOL, + .help = N_("use seclabels allowing writes") + }, + {.name = "seclabel-restore", + .type = VSH_OT_BOOL, + .help = N_("Try to restore security label after use if possible") + }, + {.name = "seclabel-restore-require", + .type = VSH_OT_BOOL, + .help = N_("require that security label is restored after use") + }, + {.name = NULL} +}; + +static bool +cmdDomFdAssociate(vshControl *ctl, const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + const char *name = NULL; + unsigned int flags = 0; + g_autofree int *fds = NULL; + size_t nfds = 0; + + if (vshCommandOptBool(cmd, "seclabel-writable")) + flags |= VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_WRITABLE; + + if (vshCommandOptBool(cmd, "seclabel-restore")) + flags |= VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE; + + if (vshCommandOptBool(cmd, "seclabel-restore-require")) + flags |= VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_WRITABLE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0) + return false; + + if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0) + return false; + + if (virDomainFDAssociate(dom, name, nfds, fds, flags) < 0) + return false; + + return true; +} + + /* * "qemu-monitor-command" command */ @@ -14417,5 +14494,11 @@ const vshCmdDef domManagementCmds[] = { .info = info_domdirtyrate_calc, .flags = 0 }, + {.name = "dom-fd-associate", + .handler = cmdDomFdAssociate, + .opts = opts_dom_fd_associate, + .info = info_dom_fd_associate, + .flags = 0 + }, {.name = NULL} }; -- 2.38.1

For FD-passing of disk sources we'll need to keep the FDs around. Introduce a data type helper based on a g_object so that we get reference counting. One instance will (due to security labelling) will need to be part of the virStorageSource struct thus it's declared in the storage_source_conf module. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/storage_source_conf.c | 41 ++++++++++++++++++++++++++++++++++ src/conf/storage_source_conf.h | 17 ++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 59 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 84df8d28fa..3fe8c4a900 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -28,6 +28,7 @@ #include "virerror.h" #include "virlog.h" #include "virstring.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1361,3 +1362,43 @@ virStorageSourceInitiatorClear(virStorageSourceInitiatorDef *initiator) { VIR_FREE(initiator->iqn); } + +G_DEFINE_TYPE(virStorageSourceFDTuple, vir_storage_source_fd_tuple, G_TYPE_OBJECT); + +static void +vir_storage_source_fd_tuple_init(virStorageSourceFDTuple *fdt G_GNUC_UNUSED) +{ +} + + +static void +virStorageSourceFDTupleFinalize(GObject *object) +{ + virStorageSourceFDTuple *fdt = VIR_STORAGE_SOURCE_FD_TUPLE(object); + size_t i; + + if (!fdt) + return; + + for (i = 0; i < fdt->nfds; i++) + VIR_FORCE_CLOSE(fdt->fds[i]); + + g_free(fdt->fds); + G_OBJECT_CLASS(vir_storage_source_fd_tuple_parent_class)->finalize(object); +} + + +static void +vir_storage_source_fd_tuple_class_init(virStorageSourceFDTupleClass *klass) +{ + GObjectClass *obj = G_OBJECT_CLASS(klass); + + obj->finalize = virStorageSourceFDTupleFinalize; +} + + +virStorageSourceFDTuple * +virStorageSourceFDTupleNew(void) +{ + return g_object_new(vir_storage_source_fd_tuple_get_type(), NULL); +} diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index f2440cec6a..9cd1a0c137 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -258,6 +258,23 @@ struct _virStorageSourceSlice { }; +struct _virStorageSourceFDTuple { + GObject parent; + int *fds; + size_t nfds; + + bool writable; + bool tryRestoreLabel; + + /* connection this FD tuple is associated with for auto-closing */ + virConnect *conn; +}; +G_DECLARE_FINAL_TYPE(virStorageSourceFDTuple, vir_storage_source_fd_tuple, VIR, STORAGE_SOURCE_FD_TUPLE, GObject); + +virStorageSourceFDTuple * +virStorageSourceFDTupleNew(void); + + typedef struct _virStorageSource virStorageSource; /* Stores information related to a host resource. In the case of backing diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b81c2cc7da..ef88e2b49f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1117,6 +1117,7 @@ virStorageSourceChainHasManagedPR; virStorageSourceChainHasNVMe; virStorageSourceClear; virStorageSourceCopy; +virStorageSourceFDTupleNew; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; virStorageSourceHasBacking; -- 2.38.1

Implement passing and storage of FDs for the qemu driver. The FD tuples are g_object instances stored in a per-domain hash table and are automatically removed once the connection is closed. In the future we can consider supporting also to not tie the lifetime of the passed FDs bound to the connection. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 2 ++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5c05032ce3..33a9145cc9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1845,6 +1845,7 @@ qemuDomainObjPrivateFree(void *data) qemuDomainMasterKeyFree(priv); g_clear_pointer(&priv->blockjobs, g_hash_table_unref); + g_clear_pointer(&priv->fds, g_hash_table_unref); /* This should never be non-NULL if we get here, but just in case... */ if (priv->eventThread) { @@ -1872,6 +1873,7 @@ qemuDomainObjPrivateAlloc(void *opaque) return NULL; priv->blockjobs = virHashNew(virObjectUnref); + priv->fds = virHashNew(g_object_unref); /* agent commands block by default, user can choose different behavior */ priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2f027fad87..1cba3fa394 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -253,6 +253,9 @@ struct _qemuDomainObjPrivate { pid_t schedCoreChildFD; GSList *threadContextAliases; /* List of IDs of thread-context objects */ + + /* named file descriptor groups associated with the VM */ + GHashTable *fds; }; #define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37bdc81378..c086d4069b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20642,6 +20642,72 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom, } +static void +qemuDomainFDHashCloseConnect(virDomainObj *vm, + virConnectPtr conn) +{ + qemuDomainObjPrivate *priv = QEMU_DOMAIN_PRIVATE(vm); + virStorageSourceFDTuple *data; + GHashTableIter htitr; + + if (!priv->fds) + return; + + g_hash_table_iter_init(&htitr, priv->fds); + + while (g_hash_table_iter_next(&htitr, NULL, (void **) &data)) { + if (data->conn == conn) + g_hash_table_iter_remove(&htitr); + } +} + + +static int +qemuDomainFDAssociate(virDomainPtr domain, + const char *name, + unsigned int nfds, + int *fds, + unsigned int flags) +{ + virDomainObj *vm = NULL; + qemuDomainObjPrivate *priv; + virStorageSourceFDTuple *new; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE | + VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_WRITABLE, -1); + + if (nfds == 0) + return 0; + + if (!(vm = qemuDomainObjFromDomain(domain))) + return -1; + + if (virDomainFdAssociateEnsureACL(domain->conn, vm->def)) + goto cleanup; + + priv = vm->privateData; + + new = virStorageSourceFDTupleNew(); + new->fds = fds; + new->nfds = nfds; + new->conn = domain->conn; + + new->writable = flags & VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_WRITABLE; + new->tryRestoreLabel = flags & VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE; + + virCloseCallbacksDomainAdd(vm, domain->conn, qemuDomainFDHashCloseConnect); + + g_hash_table_insert(priv->fds, g_strdup(name), new); + + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -20890,6 +20956,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */ .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */ + .domainFDAssociate = qemuDomainFDAssociate, /* 9.0.0 */ }; -- 2.38.1

Introduce a new argument type for testQemuInfoSetArgs named ARG_FD_GROUP which allows users to instantiate tests with populated FD passing hash table. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/storage_source_conf.c | 1 + src/conf/storage_source_conf.h | 1 + tests/qemuxml2argvtest.c | 5 +++++ tests/testutilsqemu.c | 33 +++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 2 ++ 5 files changed, 42 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 3fe8c4a900..e9d9c3a558 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -1384,6 +1384,7 @@ virStorageSourceFDTupleFinalize(GObject *object) VIR_FORCE_CLOSE(fdt->fds[i]); g_free(fdt->fds); + g_free(fdt->testfds); G_OBJECT_CLASS(vir_storage_source_fd_tuple_parent_class)->finalize(object); } diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index 9cd1a0c137..7c99ac8976 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -262,6 +262,7 @@ struct _virStorageSourceFDTuple { GObject parent; int *fds; size_t nfds; + int *testfds; /* populated by tests to ensure stable FDs */ bool writable; bool tryRestoreLabel; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2db0e90f2b..b4b60a0130 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -705,6 +705,11 @@ testCompareXMLToArgv(const void *data) } priv = vm->privateData; + if (info->args.fds) { + g_clear_pointer(&priv->fds, g_hash_table_unref); + priv->fds = g_steal_pointer(&info->args.fds); + } + if (virBitmapParse("0-3", &priv->autoNodeset, 4) < 0) goto cleanup; diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 6d3decdc16..396803c40b 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -932,6 +932,38 @@ testQemuInfoSetArgs(struct testQemuInfo *info, info->args.hostOS = va_arg(argptr, int); break; + case ARG_FD_GROUP: { + virStorageSourceFDTuple *new = virStorageSourceFDTupleNew(); + const char *fdname = va_arg(argptr, char *); + VIR_AUTOCLOSE fakefd = open("/dev/zero", O_RDWR); + size_t i; + + new->nfds = va_arg(argptr, unsigned int); + new->fds = g_new0(int, new->nfds); + new->testfds = g_new0(int, new->nfds); + + for (i = 0; i < new->nfds; i++) { + new->testfds[i] = va_arg(argptr, unsigned int); + + if (fcntl(new->testfds[i], F_GETFD) != -1) { + fprintf(stderr, "fd '%d' is already in use\n", new->fds[i]); + abort(); + } + + if ((new->fds[i] = dup(fakefd)) < 0) { + fprintf(stderr, "failed to duplicate fake fd: %s", + g_strerror(errno)); + abort(); + } + } + + if (!info->args.fds) + info->args.fds = virHashNew(g_object_unref); + + g_hash_table_insert(info->args.fds, g_strdup(fdname), new); + break; + } + case ARG_END: default: info->args.invalidarg = true; @@ -1037,6 +1069,7 @@ testQemuInfoClear(struct testQemuInfo *info) VIR_FREE(info->errfile); virObjectUnref(info->qemuCaps); g_clear_pointer(&info->args.fakeCaps, virObjectUnref); + g_clear_pointer(&info->args.fds, g_hash_table_unref); } diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 943958d02a..51c072cb13 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -52,6 +52,7 @@ typedef enum { ARG_CAPS_VER, ARG_CAPS_HOST_CPU_MODEL, ARG_HOST_OS, + ARG_FD_GROUP, /* name, nfds, fd[0], ... fd[n-1] */ ARG_END, } testQemuInfoArgName; @@ -87,6 +88,7 @@ struct testQemuArgs { qemuTestCPUDef capsHostCPUModel; int gic; testQemuHostOS hostOS; + GHashTable *fds; bool invalidarg; }; -- 2.38.1

The 'fdgroup' will allow users to specify a passed FD (via the 'virDomainFDAssociate()' API) use instead of opening a path. This is useful in cases when e.g. the file is not accessible from inside a container. Since this uses the same disk type as when we open files via names this patch also introduces a hypervisor feature which the hypervisor asserts that code paths are ready for this possibility. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/formatdomain.rst | 8 +++++ src/conf/domain_conf.c | 2 ++ src/conf/domain_conf.h | 1 + src/conf/domain_postparse.c | 9 +++++ src/conf/schemas/domaincommon.rng | 3 ++ src/conf/storage_source_conf.c | 2 ++ src/conf/storage_source_conf.h | 1 + src/security/virt-aa-helper.c | 3 +- tests/qemuxml2argvdata/disk-source-fd.xml | 40 +++++++++++++++++++++++ 9 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/disk-source-fd.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index d7fffc6e0b..109a2ac45a 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2701,6 +2701,14 @@ paravirtualized driver is specified via the ``disk`` element. ``file`` The ``file`` attribute specifies the fully-qualified path to the file holding the disk. :since:`Since 0.0.3` + + :since:`Since 9.0.0` a new optional attribute ``fdgroup`` can be added + instructing to access the disk via file descriptiors associated to the + domain object via the ``virDomainFDAssociate()`` API rather than opening + the files. The files do not necessarily have to be accessible by libvirt + via the filesystem. The filename passed via ``file`` can still be used + to generate paths to write into image metadata when doing block operations + but libvirt will not access these natively. ``block`` The ``dev`` attribute specifies the fully-qualified path to the host device to serve as the disk. :since:`Since 0.0.3` diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66189277fd..939b221bc7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7345,6 +7345,7 @@ virDomainStorageSourceParse(xmlNodePtr node, switch (src->type) { case VIR_STORAGE_TYPE_FILE: src->path = virXMLPropString(node, "file"); + src->fdgroup = virXMLPropString(node, "fdgroup"); break; case VIR_STORAGE_TYPE_BLOCK: src->path = virXMLPropString(node, "dev"); @@ -21877,6 +21878,7 @@ virDomainDiskSourceFormat(virBuffer *buf, switch (src->type) { case VIR_STORAGE_TYPE_FILE: virBufferEscapeString(&attrBuf, " file='%s'", src->path); + virBufferEscapeString(&attrBuf, " fdgroup='%s'", src->fdgroup); break; case VIR_STORAGE_TYPE_BLOCK: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 33c4ff69dd..0b7a095ffd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3167,6 +3167,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER = (1 << 6), VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT = (1 << 7), VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING = (1 << 8), + VIR_DOMAIN_DEF_FEATURE_DISK_FD = (1 << 9), } virDomainDefFeatures; diff --git a/src/conf/domain_postparse.c b/src/conf/domain_postparse.c index 9a3e8f494c..d1f0b80338 100644 --- a/src/conf/domain_postparse.c +++ b/src/conf/domain_postparse.c @@ -885,6 +885,15 @@ virDomainDeviceDefPostParseCheckFeatures(virDomainDeviceDef *dev, return -1; } + if (dev->type == VIR_DOMAIN_DEVICE_DISK && + dev->data.disk->src->fdgroup && + UNSUPPORTED(VIR_DOMAIN_DEF_FEATURE_DISK_FD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("driver does not support FD passing for disk '%s'"), + dev->data.disk->dst); + return -1; + } + return 0; } #undef UNSUPPORTED diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index c588a48fd2..ccc114beff 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1806,6 +1806,9 @@ <ref name="vmwarePath"/> </choice> </attribute> + <optional> + <attribute name="fdgroup"/> + </optional> </optional> <ref name="diskSourceCommon"/> <optional> diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index e9d9c3a558..395b78844d 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -817,6 +817,7 @@ virStorageSourceCopy(const virStorageSource *src, def->drv = NULL; def->path = g_strdup(src->path); + def->fdgroup = g_strdup(src->fdgroup); def->volume = g_strdup(src->volume); def->relPath = g_strdup(src->relPath); def->backingStoreRaw = g_strdup(src->backingStoreRaw); @@ -1123,6 +1124,7 @@ virStorageSourceClear(virStorageSource *def) return; VIR_FREE(def->path); + VIR_FREE(def->fdgroup); VIR_FREE(def->volume); VIR_FREE(def->snapshot); VIR_FREE(def->configFile); diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index 7c99ac8976..ef82104e6c 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -289,6 +289,7 @@ struct _virStorageSource { unsigned int id; /* backing chain identifier, 0 is unset */ virStorageType type; char *path; + char *fdgroup; /* name of group of file descriptors the user wishes to use instead of 'path' */ int protocol; /* virStorageNetProtocol */ char *volume; /* volume name for remote storage */ char *snapshot; /* for storage systems supporting internal snapshots */ diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 53a1cd1048..c8db925094 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -607,7 +607,8 @@ virDomainDefParserConfig virAAHelperDomainDefParserConfig = { .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG | VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN | VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS | - VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING, + VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING | + VIR_DOMAIN_DEF_FEATURE_DISK_FD, }; static int diff --git a/tests/qemuxml2argvdata/disk-source-fd.xml b/tests/qemuxml2argvdata/disk-source-fd.xml new file mode 100644 index 0000000000..d8c47fa364 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-source-fd.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/path/to/blah' fdgroup='testgroup2'/> + <target dev='vde' bus='virtio'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/rhel7.1484071880' fdgroup='testgroup5'/> + <backingStore type='file'> + <format type='qcow2'/> + <source file='/var/lib/libvirt/images/rhel7.1484071877' fdgroup='testgroup6'/> + <backingStore type='file'> + <format type='qcow2'/> + <source file='/var/lib/libvirt/images/rhel7.1484071876'/> + <backingStore/> + </backingStore> + </backingStore> + <target dev='vdf' bus='virtio'/> + </disk> + <controller type='usb'/> + <controller type='pci' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> -- 2.38.1

The new helper qemuDomainStartupCleanup is used to perform cleanup after a startup of a VM (successful or not). The initial implementation just calls qemuDomainSecretDestroy, which can be un-exported. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 15 ++++++++++++++- src/qemu/qemu_domain.h | 3 +-- src/qemu/qemu_process.c | 2 +- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 33a9145cc9..1f288fa0cf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1552,7 +1552,7 @@ qemuDomainSecretGraphicsPrepare(virQEMUDriverConfig *cfg, * * Removes all unnecessary data which was needed to generate 'secret' objects. */ -void +static void qemuDomainSecretDestroy(virDomainObj *vm) { size_t i; @@ -12279,3 +12279,16 @@ qemuDomainSchedCoreStop(qemuDomainObjPrivate *priv) priv->schedCoreChildPID = -1; } } + + +/** + * qemuDomainStartupCleanup: + * + * Performs a cleanup of data which is not required after a startup of a VM + * (successful or not). + */ +void +qemuDomainStartupCleanup(virDomainObj *vm) +{ + qemuDomainSecretDestroy(vm); +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1cba3fa394..057de1e974 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -932,8 +932,7 @@ int qemuDomainSecretChardevPrepare(virQEMUDriverConfig *cfg, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); -void qemuDomainSecretDestroy(virDomainObj *vm) - ATTRIBUTE_NONNULL(1); +void qemuDomainStartupCleanup(virDomainObj *vm); int qemuDomainSecretPrepare(virQEMUDriver *driver, virDomainObj *vm) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fc4e58f95b..8591a026cc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7921,7 +7921,7 @@ qemuProcessLaunch(virConnectPtr conn, cleanup: qemuDomainSchedCoreStop(priv); - qemuDomainSecretDestroy(vm); + qemuDomainStartupCleanup(vm); return ret; } -- 2.38.1

The helper will be used in various places that need to check that a disk source struct is using FD passing. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/storage_source_conf.c | 7 +++++++ src/conf/storage_source_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 11 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 395b78844d..45defbe687 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -1057,6 +1057,13 @@ virStorageSourceIsLocalStorage(const virStorageSource *src) } +bool +virStorageSourceIsFD(const virStorageSource *src) +{ + return src->fdgroup; +} + + /** * virStorageSourceIsEmpty: * diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index ef82104e6c..9c07eef200 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -494,6 +494,9 @@ virStorageSourceGetActualType(const virStorageSource *def); bool virStorageSourceIsLocalStorage(const virStorageSource *src); +bool +virStorageSourceIsFD(const virStorageSource *src); + bool virStorageSourceIsEmpty(virStorageSource *src); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ef88e2b49f..b4c6e6a09e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1129,6 +1129,7 @@ virStorageSourceInitiatorParseXML; virStorageSourceIsBacking; virStorageSourceIsBlockLocal; virStorageSourceIsEmpty; +virStorageSourceIsFD; virStorageSourceIsLocalStorage; virStorageSourceIsRelative; virStorageSourceIsSameLocation; -- 2.38.1

When starting up a VM with FD-passed images we need to look up the corresponding named FD set and associate it with the virStorageSource based on the name. The association is brought into virStorageSource as security labelling code will need to access the FD to perform selinux labelling. Similarly when startup is complete in certain cases we no longer need to keep the copy of FDs and thus can close them. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/storage_source_conf.c | 5 ++ src/conf/storage_source_conf.h | 2 + src/qemu/qemu_domain.c | 86 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 ++ src/qemu/qemu_hotplug.c | 1 + 5 files changed, 99 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 45defbe687..c16016aabc 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -886,6 +886,9 @@ virStorageSourceCopy(const virStorageSource *src, return NULL; } + if (src->fdtuple) + def->fdtuple = g_object_ref(src->fdtuple); + /* ssh config passthrough for libguestfs */ def->ssh_host_key_check_disabled = src->ssh_host_key_check_disabled; def->ssh_user = g_strdup(src->ssh_user); @@ -1170,6 +1173,8 @@ virStorageSourceClear(virStorageSource *def) virStorageSourceInitiatorClear(&def->initiator); + g_clear_pointer(&def->fdtuple, g_object_unref); + /* clear everything except the class header as the object APIs * will break otherwise */ memset((char *) def + sizeof(def->parent), 0, diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index 9c07eef200..f981261ff4 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -415,6 +415,8 @@ struct _virStorageSource { * registered with a full index (vda[3]) so that we can properly report just * one event for it */ bool thresholdEventWithIndex; + + virStorageSourceFDTuple *fdtuple; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1f288fa0cf..7dc4ef4ddb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -850,6 +850,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj) g_clear_pointer(&priv->encinfo, qemuDomainSecretInfoFree); g_clear_pointer(&priv->httpcookie, qemuDomainSecretInfoFree); g_clear_pointer(&priv->tlsKeySecret, qemuDomainSecretInfoFree); + g_clear_pointer(&priv->fdpass, qemuFDPassFree); } @@ -10892,6 +10893,61 @@ qemuDomainPrepareDiskSourceLegacy(virDomainDiskDef *disk, } +static int +qemuDomainPrepareStorageSourceFDs(virStorageSource *src, + qemuDomainObjPrivate *priv) +{ + qemuDomainStorageSourcePrivate *srcpriv = NULL; + virStorageType actualType = virStorageSourceGetActualType(src); + virStorageSourceFDTuple *fdt = NULL; + size_t i; + + if (actualType != VIR_STORAGE_TYPE_FILE && + actualType != VIR_STORAGE_TYPE_BLOCK) + return 0; + + if (!virStorageSourceIsFD(src)) + return 0; + + if (!(fdt = virHashLookup(priv->fds, src->fdgroup))) { + virReportError(VIR_ERR_INVALID_ARG, + _("file descriptor group '%s' was not associated with the domain"), + src->fdgroup); + return -1; + } + + srcpriv = qemuDomainStorageSourcePrivateFetch(src); + + srcpriv->fdpass = qemuFDPassNew(src->nodestorage, priv); + + for (i = 0; i < fdt->nfds; i++) { + g_autofree char *idx = g_strdup_printf("%zu", i); + int tmpfd; + + if (fdt->testfds) { + /* when testing we want to use stable FD numbers provided by the test + * case */ + tmpfd = dup2(fdt->fds[i], fdt->testfds[i]); + } else { + tmpfd = dup(fdt->fds[i]); + } + + if (tmpfd < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to duplicate file descriptor for fd group '%s'"), + src->fdgroup); + return -1; + } + + qemuFDPassAddFD(srcpriv->fdpass, &tmpfd, idx); + } + + src->fdtuple = g_object_ref(fdt); + + return 0; +} + + int qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, virStorageSource *src, @@ -10929,6 +10985,9 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, if (qemuDomainPrepareStorageSourceNFS(src) < 0) return -1; + if (qemuDomainPrepareStorageSourceFDs(src, priv) < 0) + return -1; + return 0; } @@ -12281,6 +12340,28 @@ qemuDomainSchedCoreStop(qemuDomainObjPrivate *priv) } +/** + * qemuDomainCleanupStorageSourceFD: + * @src: start of the chain to clear + * + * Cleans up the backing chain starting at @src of FD tuple structures for + * all FD-tuples which didn't request explicit relabelling and thus the struct + * is no longer needed. + */ +void +qemuDomainCleanupStorageSourceFD(virStorageSource *src) +{ + virStorageSource *n; + + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virStorageSourceIsFD(n) && n->fdtuple) { + if (!n->fdtuple->tryRestoreLabel) + g_clear_pointer(&n->fdtuple, g_object_unref); + } + } +} + + /** * qemuDomainStartupCleanup: * @@ -12290,5 +12371,10 @@ qemuDomainSchedCoreStop(qemuDomainObjPrivate *priv) void qemuDomainStartupCleanup(virDomainObj *vm) { + size_t i; + qemuDomainSecretDestroy(vm); + + for (i = 0; i < vm->def->ndisks; i++) + qemuDomainCleanupStorageSourceFD(vm->def->disks[i]->src); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 057de1e974..add653d9db 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -305,6 +305,9 @@ struct _qemuDomainStorageSourcePrivate { /* key for decrypting TLS certificate */ qemuDomainSecretInfo *tlsKeySecret; + + /* file descriptors if user asks for FDs to be passed */ + qemuFDPass *fdpass; }; virObject *qemuDomainStorageSourcePrivateNew(void); @@ -932,6 +935,8 @@ int qemuDomainSecretChardevPrepare(virQEMUDriverConfig *cfg, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +void qemuDomainCleanupStorageSourceFD(virStorageSource *src); + void qemuDomainStartupCleanup(virDomainObj *vm); int qemuDomainSecretPrepare(virQEMUDriver *driver, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6e300f547c..dba699a8a8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1016,6 +1016,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, ignore_value(qemuHotplugRemoveManagedPR(vm, VIR_ASYNC_JOB_NONE)); } qemuDomainSecretDiskDestroy(disk); + qemuDomainCleanupStorageSourceFD(disk->src); return ret; } -- 2.38.1

Prepare the internal data for passing FDs instead of having qemu open the file internally. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 31 ++++++++++++++++++++++++++++--- src/qemu/qemu_command.c | 22 ++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 8a6f601b29..a672ad6f54 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -673,22 +673,47 @@ qemuBlockStorageSourceGetSshProps(virStorageSource *src) static virJSONValue * qemuBlockStorageSourceGetFileProps(virStorageSource *src, - bool onlytarget) + bool onlytarget, + virTristateBool *autoReadOnly, + virTristateBool *readOnly) { + const char *path = src->path; const char *iomode = NULL; const char *prManagerAlias = NULL; virJSONValue *ret = NULL; if (!onlytarget) { + qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + if (src->pr) prManagerAlias = src->pr->mgralias; if (src->iomode != VIR_DOMAIN_DISK_IO_DEFAULT) iomode = virDomainDiskIoTypeToString(src->iomode); + + if (srcpriv && srcpriv->fdpass) { + path = qemuFDPassGetPath(srcpriv->fdpass); + + /* when passing a FD to qemu via the /dev/fdset mechanism qemu + * fetches the appropriate FD from the fdset by checking that it has + * the correct accessmode. Now with 'auto-read-only' in effect qemu + * wants to use a read-only FD first. If the user didn't pass multiple + * FDs the feature will not work regardless, so we'll disable it. */ + if (src->fdtuple->nfds == 1) { + *autoReadOnly = VIR_TRISTATE_BOOL_ABSENT; + + /* now we setup the normal readonly flag. If user requested write + * access honour it */ + if (src->fdtuple->writable) + *readOnly = VIR_TRISTATE_BOOL_NO; + else + *readOnly = virTristateBoolFromBool(src->readonly); + } + } } ignore_value(virJSONValueObjectAdd(&ret, - "s:filename", src->path, + "s:filename", path, "S:aio", iomode, "S:pr-manager", prManagerAlias, NULL) < 0); @@ -818,7 +843,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource *src, driver = "file"; } - if (!(fileprops = qemuBlockStorageSourceGetFileProps(src, onlytarget))) + if (!(fileprops = qemuBlockStorageSourceGetFileProps(src, onlytarget, &aro, &ro))) return NULL; break; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 06e29ff8ae..ed159bc8b1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2146,6 +2146,25 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd, } +static int +qemuBuildDiskSourceCommandLineFDs(virCommand *cmd, + virDomainDiskDef *disk) +{ + virStorageSource *n; + + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(n); + + if (!srcpriv || !srcpriv->fdpass) + continue; + + qemuFDPassTransferCommand(srcpriv->fdpass, cmd); + } + + return 0; +} + + static int qemuBuildDiskSourceCommandLine(virCommand *cmd, virDomainDiskDef *disk, @@ -2163,6 +2182,9 @@ qemuBuildDiskSourceCommandLine(virCommand *cmd, if (virStorageSourceIsEmpty(disk->src)) return 0; + if (qemuBuildDiskSourceCommandLineFDs(cmd, disk) < 0) + return -1; + if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src))) return -1; -- 2.38.1

On Fri, Jan 06, 2023 at 14:33:51 +0100, Pavel Hrdina wrote: qemu will complain that it can't find the appropriate one. Given that this is for very specific use cases I didn't want to overdo it with validation.

On Thu, Jan 05, 2023 at 05:30:21PM +0100, Peter Krempa wrote: This change makes the compiler complain with the following error: ../src/storage_file/storage_source.c: In function ‘virStorageSourceGetMetadataRecurse’: ../src/storage_file/storage_source.c:1343:9: error: ‘headerLen’ may be used uninitialized [-Werror=maybe-uninitialized] 1343 | if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/storage_file/storage_source.c:1311:12: note: ‘headerLen’ was declared here 1311 | size_t headerLen; The issue is that at the end of this function we set `*headerLen = len;` but it doesn't happen with FD storage source, instead we return the len and the return value of virStorageSourceGetMetadataRecurseReadHeader() is only used in if condition. This seems like no-op as virStorageSourceIsFD() does the same. Pavel

On Thu, Jan 05, 2023 at 05:30:22PM +0100, Peter Krempa wrote: s/FD-passed/FD passed/ s/fd-passed/FD passed/ Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt;