[PATCH 00/36] Add support for passing FDs to access disk images

First part of the series refactors close callbacks to allow having more of them and then implements new API virDomainFDAssociate and plumbs it to pass to qemu. Peter Krempa (36): datatypes: Simplify error path of 'virGetDomain' datatypes: Clean up whitespace in definition of struct _virConnect conf: virdomainobjlist: Convert header to contemporary style conf: virdomainobjlist: Introduce 'virDomainObjListCollectAll' conf: virdomainobjlist: Remove return value from virDomainObjListCollect conf: domain: Add helper infrastructure for new connection close callbacks virclosecallbacks: Add new close callbacks APIs lxc: Use new connection close callbacks API bhyve: Use new connection close callbacks API qemu: Use new connection close callbacks API qemuMigrationSrcIsAllowed: Remove unused 'driver' argument qemuMigrationSrcBeginResumePhase: Remove unused 'driver' argument virclosecallbacks: Remove old close callbacks code gendispatch: Add 'G_GNUC_WARN_UNUSED_RESULT' to output of 'aclheader' remote_driver: Return 'virLockGuard' from 'remoteDriverLock' remote_driver: Refactor few functions as example of auto-locking virStorageSourceIsSameLocation: Use switch statement for individual storage types qemuxml2argvtest: Add seclabels in <backingStore> to disk-backing-chains-(no)index qemuxml2xmltest: Remove 'disk-backing-chain' case and output files lib: Introduce virDomainFDAssociate API virsh: Introduce 'dom-fd-associate' for invoking virDomainFDAssociate() conf: storage_source: Introduce type for storing FDs associated for storage qemu: Implement qemuDomainFDAssociate qemuxml2argvtest: Add support for populating 'fds' in private data conf: Add 'fdgroup' attribute for 'file' disks qemu: domain: Introduce qemuDomainStartupCleanup conf: storage_source: Introduce virStorageSourceIsFD qemu: Prepare data for FD-passed disk image sources qemu: block: Add support for passing FDs of disk images secuirity: DAC: Don't relabel FD-passed virStorageSource images security: selinux: Handle security labelling of FD-passed images qemu: Prepare storage backing chain traversal code for FD passed images qemu: driver: Don't allow certain operations with FD-passed disks qemu: cgroup: Don't setup cgroups for FD-passed images qemu: Enable support for FD passed disk sources qemuxml2*test: Enable testing of disks with 'fdgroup' docs/formatdomain.rst | 8 + docs/manpages/virsh.rst | 22 + include/libvirt/libvirt-domain.h | 22 + po/POTFILES | 1 - src/bhyve/bhyve_domain.c | 15 +- src/bhyve/bhyve_driver.c | 6 +- src/bhyve/bhyve_process.c | 9 +- src/bhyve/bhyve_utils.h | 2 - src/conf/domain_conf.c | 15 + src/conf/domain_conf.h | 26 + src/conf/domain_postparse.c | 9 + src/conf/schemas/domaincommon.rng | 3 + src/conf/storage_source_conf.c | 80 ++- src/conf/storage_source_conf.h | 27 ++ src/conf/virdomainobjlist.c | 39 +- src/conf/virdomainobjlist.h | 160 +++--- src/datatypes.c | 14 +- src/datatypes.h | 5 +- src/driver-hypervisor.h | 8 + src/hypervisor/virclosecallbacks.c | 454 ++++++++++-------- src/hypervisor/virclosecallbacks.h | 37 +- src/libvirt-domain.c | 82 ++++ src/libvirt_private.syms | 14 +- src/libvirt_public.syms | 5 + src/lxc/lxc_conf.c | 15 +- src/lxc/lxc_conf.h | 3 - src/lxc/lxc_driver.c | 8 +- src/lxc/lxc_process.c | 8 +- src/qemu/qemu_block.c | 31 +- src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 22 + src/qemu/qemu_conf.c | 17 +- src/qemu/qemu_conf.h | 3 - src/qemu/qemu_domain.c | 129 ++++- src/qemu/qemu_domain.h | 11 +- src/qemu/qemu_driver.c | 108 ++++- src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_migration.c | 54 +-- src/qemu/qemu_migration.h | 3 +- src/qemu/qemu_process.c | 37 +- src/qemu/qemu_process.h | 11 +- src/qemu/qemu_snapshot.c | 4 +- src/remote/remote_daemon_dispatch.c | 40 ++ src/remote/remote_driver.c | 70 ++- src/remote/remote_protocol.x | 14 +- src/remote_protocol-structs | 6 + src/rpc/gendispatch.pl | 2 +- src/security/security_dac.c | 16 +- src/security/security_selinux.c | 32 +- src/security/virt-aa-helper.c | 3 +- src/storage_file/storage_source.c | 14 + src/test/test_driver.c | 4 +- src/vz/vz_driver.c | 7 +- .../disk-backing-chains-index.xml | 6 +- .../disk-backing-chains-noindex.xml | 6 +- .../qemuxml2argvdata/disk-backing-chains.xml | 98 ---- .../disk-source-fd.x86_64-latest.args | 49 ++ tests/qemuxml2argvdata/disk-source-fd.xml | 40 ++ tests/qemuxml2argvtest.c | 9 + .../disk-backing-chains-active.xml | 110 ----- .../disk-backing-chains-inactive.xml | 110 ----- .../disk-backing-chains-index-active.xml | 6 +- .../disk-backing-chains-index-inactive.xml | 6 +- .../disk-backing-chains-noindex.xml | 6 +- .../disk-source-fd.x86_64-latest.xml | 52 ++ tests/qemuxml2xmltest.c | 3 +- tests/testutilsqemu.c | 33 ++ tests/testutilsqemu.h | 2 + tools/virsh-domain.c | 83 ++++ 69 files changed, 1512 insertions(+), 847 deletions(-) delete mode 100644 tests/qemuxml2argvdata/disk-backing-chains.xml create mode 100644 tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-source-fd.xml delete mode 100644 tests/qemuxml2xmloutdata/disk-backing-chains-active.xml delete mode 100644 tests/qemuxml2xmloutdata/disk-backing-chains-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/disk-source-fd.x86_64-latest.xml -- 2.38.1

'virObjectNew' can't return NULL. If we pre-check the arguments we don't need a cleanup label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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:
'virObjectNew' can't return NULL. If we pre-check the arguments we don't need a cleanup label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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; }
/**
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On Thu, Jan 05, 2023 at 05:29:50PM +0100, Peter Krempa wrote:
'virObjectNew' can't return NULL. If we pre-check the arguments we don't need a cleanup label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/datatypes.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Remove extraneous spaces and put comment on a single line. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/datatypes.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/datatypes.h b/src/datatypes.h index 0f9730d9e8..f8b1a83407 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -528,11 +528,10 @@ struct _virConnect { virSecretDriver *secretDriver; virNWFilterDriver *nwfilterDriver; - /* Private data pointer which can be used by domain driver as - * it pleases. + /* Private data pointer which can be used by domain driver as it pleases. * NB: 'private' is a reserved word in C++. */ - void * privateData; + void *privateData; /* * Object lock must be acquired before accessing/changing any of -- 2.38.1

On 1/5/23 10:29 AM, Peter Krempa wrote:
Remove extraneous spaces and put comment on a single line.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/datatypes.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/datatypes.h b/src/datatypes.h index 0f9730d9e8..f8b1a83407 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -528,11 +528,10 @@ struct _virConnect { virSecretDriver *secretDriver; virNWFilterDriver *nwfilterDriver;
- /* Private data pointer which can be used by domain driver as - * it pleases. + /* Private data pointer which can be used by domain driver as it pleases. * NB: 'private' is a reserved word in C++. */ - void * privateData; + void *privateData;
/* * Object lock must be acquired before accessing/changing any of
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On Thu, Jan 05, 2023 at 05:29:51PM +0100, Peter Krempa wrote:
Remove extraneous spaces and put comment on a single line.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/datatypes.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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:
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@redhat.com> --- 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);
Fine with me as long as others don't object to mass reformatting Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On Thu, Jan 05, 2023 at 05:29:52PM +0100, Peter Krempa wrote:
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@redhat.com> --- src/conf/virdomainobjlist.h | 156 ++++++++++++++++++++---------------- 1 file changed, 86 insertions(+), 70 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Introduce a helper which will return a list of all domain objects inside of the list without filtering and thus without the need to lock individual members. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainobjlist.c | 32 ++++++++++++++++++++------------ src/conf/virdomainobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 2569454ff8..4968dfcf3e 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -913,6 +913,24 @@ virDomainObjListCollectIterator(void *payload, } +void +virDomainObjListCollectAll(virDomainObjList *domlist, + virDomainObj ***vms, + size_t *nvms) +{ + struct virDomainListData data = { NULL, 0 }; + + virObjectRWLockRead(domlist); + data.vms = g_new0(virDomainObj *, virHashSize(domlist->objs)); + + virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); + virObjectRWUnlock(domlist); + + *nvms = data.nvms; + *vms = data.vms; +} + + static void virDomainObjListFilter(virDomainObj ***list, size_t *nvms, @@ -954,18 +972,8 @@ virDomainObjListCollect(virDomainObjList *domlist, virDomainObjListACLFilter filter, unsigned int flags) { - struct virDomainListData data = { NULL, 0 }; - - virObjectRWLockRead(domlist); - data.vms = g_new0(virDomainObj *, virHashSize(domlist->objs)); - - virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); - virObjectRWUnlock(domlist); - - virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags); - - *nvms = data.nvms; - *vms = data.vms; + virDomainObjListCollectAll(domlist, vms, nvms); + virDomainObjListFilter(vms, nvms, conn, filter, flags); return 0; } diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index cfa165d56f..8c53680374 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -145,6 +145,10 @@ virDomainObjListForEach(virDomainObjList *doms, VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_CHECKPOINT) +void +virDomainObjListCollectAll(virDomainObjList *domlist, + virDomainObj ***vms, + size_t *nvms); int virDomainObjListCollect(virDomainObjList *doms, virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ae746a2d51..54a3859604 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1209,6 +1209,7 @@ virDomainMomentObjNew; # conf/virdomainobjlist.h virDomainObjListAdd; virDomainObjListCollect; +virDomainObjListCollectAll; virDomainObjListConvert; virDomainObjListExport; virDomainObjListFindByID; -- 2.38.1

On 1/5/23 10:29 AM, Peter Krempa wrote:
Introduce a helper which will return a list of all domain objects inside of the list without filtering and thus without the need to lock individual members.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainobjlist.c | 32 ++++++++++++++++++++------------ src/conf/virdomainobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 2569454ff8..4968dfcf3e 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -913,6 +913,24 @@ virDomainObjListCollectIterator(void *payload, }
+void +virDomainObjListCollectAll(virDomainObjList *domlist, + virDomainObj ***vms, + size_t *nvms) +{ + struct virDomainListData data = { NULL, 0 }; + + virObjectRWLockRead(domlist); + data.vms = g_new0(virDomainObj *, virHashSize(domlist->objs)); + + virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); + virObjectRWUnlock(domlist); + + *nvms = data.nvms; + *vms = data.vms; +} + + static void virDomainObjListFilter(virDomainObj ***list, size_t *nvms, @@ -954,18 +972,8 @@ virDomainObjListCollect(virDomainObjList *domlist, virDomainObjListACLFilter filter, unsigned int flags) { - struct virDomainListData data = { NULL, 0 }; - - virObjectRWLockRead(domlist); - data.vms = g_new0(virDomainObj *, virHashSize(domlist->objs)); - - virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); - virObjectRWUnlock(domlist); - - virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags); - - *nvms = data.nvms; - *vms = data.vms; + virDomainObjListCollectAll(domlist, vms, nvms); + virDomainObjListFilter(vms, nvms, conn, filter, flags);
return 0; } diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index cfa165d56f..8c53680374 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -145,6 +145,10 @@ virDomainObjListForEach(virDomainObjList *doms, VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_CHECKPOINT)
+void +virDomainObjListCollectAll(virDomainObjList *domlist, + virDomainObj ***vms, + size_t *nvms); int virDomainObjListCollect(virDomainObjList *doms, virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ae746a2d51..54a3859604 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1209,6 +1209,7 @@ virDomainMomentObjNew; # conf/virdomainobjlist.h virDomainObjListAdd; virDomainObjListCollect; +virDomainObjListCollectAll; virDomainObjListConvert; virDomainObjListExport; virDomainObjListFindByID;
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On Thu, Jan 05, 2023 at 05:29:53PM +0100, Peter Krempa wrote:
Introduce a helper which will return a list of all domain objects inside of the list without filtering and thus without the need to lock individual members.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainobjlist.c | 32 ++++++++++++++++++++------------ src/conf/virdomainobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 25 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

The function can't fail so there's no point in returning anything. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 1/5/23 10:29 AM, Peter Krempa wrote:
The function can't fail so there's no point in returning anything.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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);
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On Thu, Jan 05, 2023 at 05:29:54PM +0100, Peter Krempa wrote:
The function can't fail so there's no point in returning anything.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

The new connect close callbacks for domains will be represented by a virObject associated with the domain object itself. To simplify handling the pointer to the close callback data will be done by an immutable pointer allocated directly when allocating the corresponding virDomainObj struct. This patch adds the 'closecallbacks' field to virDomainObj and a corresponding callback to allocate it into virDomainXMLOption. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 13 +++++++++++++ src/conf/domain_conf.h | 25 +++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c088ff295..66189277fd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1722,6 +1722,14 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOption *xmlopt) } +void +virDomainXMLOptionSetCloseCallbackAlloc(virDomainXMLOption *xmlopt, + virDomainCloseCallbackDataAlloc cb) +{ + xmlopt->closecallbackAlloc = cb; +} + + void virDomainXMLOptionSetMomentPostParse(virDomainXMLOption *xmlopt, virDomainMomentPostParseCallback cb) @@ -3906,6 +3914,7 @@ static void virDomainObjDispose(void *obj) virDomainSnapshotObjListFree(dom->snapshots); virDomainCheckpointObjListFree(dom->checkpoints); virDomainJobObjFree(dom->job); + virObjectUnref(dom->closecallbacks); } virDomainObj * @@ -3932,6 +3941,10 @@ virDomainObjNew(virDomainXMLOption *xmlopt) domain->privateDataFreeFunc = xmlopt->privateData.free; } + if (xmlopt->closecallbackAlloc) { + domain->closecallbacks = (xmlopt->closecallbackAlloc)(); + } + if (!(domain->snapshots = virDomainSnapshotObjListNew())) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1404c55053..33c4ff69dd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3126,6 +3126,23 @@ struct _virDomainObj { void *privateData; void (*privateDataFreeFunc)(void *); + /* Connection close callbacks helper data + * + * Immutable pointer sharing lifetime of the virDomainObj. May be NULL, if + * the hypervisor driver doesn't use close callbacks. + * + * The closecallbacks helper data may be accessed without holding the + * virDomainObj lock to check whether a connection being closed has a + * registered close callback. + * + * Otherwise virDomainObj must be held and acquired before the lock on the + * closecallbacks data. + * + * The above rules ensure minimal lock contention when closing the + * connection while also allowing correct handling. + */ + virObject *closecallbacks; + int taint; size_t ndeprecations; char **deprecations; @@ -3304,6 +3321,11 @@ struct _virDomainJobObjConfig { unsigned int maxQueuedJobs; }; + +typedef virObject *(*virDomainCloseCallbackDataAlloc)(void); +void virDomainXMLOptionSetCloseCallbackAlloc(virDomainXMLOption *xmlopt, + virDomainCloseCallbackDataAlloc cb); + virDomainXMLOption *virDomainXMLOptionNew(virDomainDefParserConfig *config, virDomainXMLPrivateDataCallbacks *priv, virXMLNamespace *xmlns, @@ -3352,6 +3374,9 @@ struct _virDomainXMLOption { /* virDomainJobObj callbacks, private data callbacks and defaults */ virDomainJobObjConfig jobObjConfig; + + /* closecallback allocation callback */ + virDomainCloseCallbackDataAlloc closecallbackAlloc; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainXMLOption, virObjectUnref); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 54a3859604..8f50f9fa1e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -699,6 +699,7 @@ virDomainXenPassthroughModeTypeToString; virDomainXMLOptionGetNamespace; virDomainXMLOptionGetSaveCookie; virDomainXMLOptionNew; +virDomainXMLOptionSetCloseCallbackAlloc; virDomainXMLOptionSetMomentPostParse; -- 2.38.1

On Thu, Jan 05, 2023 at 05:29:55PM +0100, Peter Krempa wrote:
The new connect close callbacks for domains will be represented by a virObject associated with the domain object itself.
To simplify handling the pointer to the close callback data will be done by an immutable pointer allocated directly when allocating the corresponding virDomainObj struct.
This patch adds the 'closecallbacks' field to virDomainObj and a corresponding callback to allocate it into virDomainXMLOption.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 13 +++++++++++++ src/conf/domain_conf.h | 25 +++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

The new APIs store the list of callbacks for a VM inside the virDomainObj and also allow registering multiple callbacks for a single domain and also for multiple connections. For now this code is dormant until each driver using the old APIs is not refactored to use the new APIs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/hypervisor/virclosecallbacks.c | 336 +++++++++++++++++++++++++++++ src/hypervisor/virclosecallbacks.h | 24 +++ src/libvirt_private.syms | 5 + 3 files changed, 365 insertions(+) diff --git a/src/hypervisor/virclosecallbacks.c b/src/hypervisor/virclosecallbacks.c index a08464438a..21b97cce12 100644 --- a/src/hypervisor/virclosecallbacks.c +++ b/src/hypervisor/virclosecallbacks.c @@ -310,3 +310,339 @@ virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, VIR_FREE(list->entries); VIR_FREE(list); } + + +struct _virCloseCallbacksDomainData { + virConnectPtr conn; + virCloseCallback cb; +}; +typedef struct _virCloseCallbacksDomainData virCloseCallbacksDomainData; + + +static void +virCloseCallbacksDomainDataFree(virCloseCallbacksDomainData* data) +{ + g_free(data); +} +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCloseCallbacksDomainData, virCloseCallbacksDomainDataFree); + + +virClass *virCloseCallbacksDomainListClass; + +struct _virCloseCallbacksDomainList { + virObjectLockable parent; + + GList *callbacks; +}; +typedef struct _virCloseCallbacksDomainList virCloseCallbacksDomainList; + + +static void +virCloseCallbacksDomainListDispose(void *obj G_GNUC_UNUSED) +{ + virCloseCallbacksDomainList *cc = obj; + + g_list_free_full(cc->callbacks, (GDestroyNotify) virCloseCallbacksDomainDataFree); +} + + +static int +virCloseCallbacksDomainListOnceInit(void) +{ + if (!(VIR_CLASS_NEW(virCloseCallbacksDomainList, virClassForObjectLockable()))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virCloseCallbacksDomainList); + + +/** + * virCloseCallbacksDomainAlloc: + * + * Allocates and returns a data structure for holding close callback data in + * a virDomainObj. + */ +virObject * +virCloseCallbacksDomainAlloc(void) +{ + if (virCloseCallbacksDomainListInitialize() < 0) + abort(); + + return virObjectNew(virCloseCallbacksDomainListClass); +} + + +/** + * virCloseCallbacksDomainAdd: + * @vm: domain object + * @conn: pointer to the connection which should trigger the close callback + * @cb: pointer to the callback function + * + * Registers @cb as a connection close callback for the @conn connection with + * the @vm domain. Duplicate registrations are ignored. + * + * Caller must hold lock on @vm. + */ +void +virCloseCallbacksDomainAdd(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + if (!conn || !cb) + return; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + virCloseCallbacksDomainData *data; + GList *n; + + for (n = cc->callbacks; n; n = n->next) { + data = n->data; + + if (data->cb == cb && data->conn == conn) + return; + } + + data = g_new0(virCloseCallbacksDomainData, 1); + data->conn = conn; + data->cb = cb; + + cc->callbacks = g_list_prepend(cc->callbacks, data); + } +} + + +/** + * virCloseCallbacksDomainMatch: + * @data: pointer to a close callback data structure + * @conn: connection pointer matched against @data + * @cb: callback pointer matched against @data + * + * Returns true if the @data callback structure matches the requested @conn + * and/or @cb parameters. If either of @conn/@cb is NULL it is interpreted as + * a wildcard. + */ +static bool +virCloseCallbacksDomainMatch(virCloseCallbacksDomainData *data, + virConnectPtr conn, + virCloseCallback cb) +{ + if (conn && cb) + return data->conn == conn && data->cb == cb; + + if (conn) + return data->conn == conn; + + if (cb) + return data->cb == cb; + + return true; +} + + +/** + * virCloseCallbacksDomainIsRegistered: + * @vm: domain object + * @conn: connection pointer + * @cb: callback pointer + * + * Returns true if @vm has one or more matching (see virCloseCallbacksDomainMatch) + * callback(s) registered. Caller must hold lock on @vm. + */ +bool +virCloseCallbacksDomainIsRegistered(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n; + + for (n = cc->callbacks; n; n = n->next) { + virCloseCallbacksDomainData *data = n->data; + + if (virCloseCallbacksDomainMatch(data, conn, cb)) + return true; + } + } + + return false; +} + + +/** + * virCloseCallbacksDomainRemove: + * @vm: domain object + * @conn: connection pointer + * @cb: callback pointer + * + * Removes all the registered matching (see virCloseCallbacksDomainMatch) + * callbacks for @vm. Caller must hold lock on @vm. + */ +void +virCloseCallbacksDomainRemove(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n = cc->callbacks; + + while (n) { + GList *cur = n; + + n = n->next; + + if (virCloseCallbacksDomainMatch(cur->data, conn, cb)) { + cc->callbacks = g_list_remove_link(cc->callbacks, cur); + g_list_free_full(cur, (GDestroyNotify) virCloseCallbacksDomainDataFree); + } + } + } +} + + +/** + * virCloseCallbacksDomainFetchForConn: + * @vm: domain object + * @conn: pointer to connection being closed + * + * Fetches connection close callbacks for @conn from @vm. The fetched close + * callbacks are removed from the list of callbacks of @vm. This function + * must be called with lock on @vm held. Caller is responsible for freeing the + * returned list. + */ +static GList * +virCloseCallbacksDomainFetchForConn(virDomainObj *vm, + virConnectPtr conn) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + GList *conncallbacks = NULL; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n; + + for (n = cc->callbacks; n;) { + virCloseCallbacksDomainData *data = n->data; + GList *cur = n; + + n = n->next; + + if (data->conn == conn) { + cc->callbacks = g_list_remove_link(cc->callbacks, cur); + conncallbacks = g_list_concat(cur, conncallbacks); + } + } + } + + return conncallbacks; +} + + +/** + * virCloseCallbacksDomainRun + * @vm: domain object + * @conn: pointer to connection being closed + * + * Fetches and sequentially calls all connection close callbacks for @conn from + * @vm. This function must be called with lock on @vm held. + */ +static void +virCloseCallbacksDomainRun(virDomainObj *vm, + virConnectPtr conn) +{ + g_autolist(virCloseCallbacksDomainData) callbacks = NULL; + GList *n; + + callbacks = virCloseCallbacksDomainFetchForConn(vm, conn); + + for (n = callbacks; n; n = n->next) { + virCloseCallbacksDomainData *data = n->data; + + VIR_DEBUG("vm='%s' cb='%p'", vm->def->name, data->cb); + + (data->cb)(vm, conn); + } +} + + +/** + * virCloseCallbacksDomainHasCallbackForConn: + * @vm: domain object + * @conn: connection being closed + * + * Returns true if @vm has a callback registered for the @conn connection. This + * function doesn't require a lock being held on @vm. + */ +static bool +virCloseCallbacksDomainHasCallbackForConn(virDomainObj *vm, + virConnectPtr conn) +{ + /* we can access vm->closecallbacks as it's a immutable pointer */ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + if (!cc) + return false; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n; + + for (n = cc->callbacks; n; n = n->next) { + virCloseCallbacksDomainData *data = n->data; + + if (data->conn == conn) + return true; + } + } + + return false; +} + + +/** + * virCloseCallbacksDomainRunForConn: + * @domains: domain list object + * @conn: connection being closed + * + * Finds all domains in @domains which registered one or more connection close + * callbacks for @conn and calls the callbacks. This function is designed to + * be called from virDrvConnectClose function of individual drivers. + * + * To minimize lock contention the function first fetches a list of all domain + * objects, then checks whether a connect close callback is actually registered + * for the domain object and just then acquires the lock on the VM object. + */ +void +virCloseCallbacksDomainRunForConn(virDomainObjList *domains, + virConnectPtr conn) +{ + virDomainObj **vms = NULL; + size_t nvms; + size_t i; + + VIR_DEBUG("conn=%p", conn); + + virDomainObjListCollectAll(domains, &vms, &nvms); + + for (i = 0; i < nvms; i++) { + virDomainObj *vm = vms[i]; + + if (!virCloseCallbacksDomainHasCallbackForConn(vm, conn)) + continue; + + VIR_WITH_OBJECT_LOCK_GUARD(vm) { + /* VIR_WITH_OBJECT_LOCK_GUARD is a for loop, so this break applies to that */ + if (vm->removing) + break; + + virCloseCallbacksDomainRun(vm, conn); + } + } + + virObjectListFreeCount(vms, nvms); +} diff --git a/src/hypervisor/virclosecallbacks.h b/src/hypervisor/virclosecallbacks.h index 7afb0e5640..b471f6b160 100644 --- a/src/hypervisor/virclosecallbacks.h +++ b/src/hypervisor/virclosecallbacks.h @@ -49,3 +49,27 @@ void virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, virConnectPtr conn, virDomainObjList *domains); + +/* ---- */ + +virObject * +virCloseCallbacksDomainAlloc(void); + +void +virCloseCallbacksDomainAdd(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb); + +void +virCloseCallbacksDomainRemove(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb); + +bool +virCloseCallbacksDomainIsRegistered(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb); + +void +virCloseCallbacksDomainRunForConn(virDomainObjList *domains, + virConnectPtr conn); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8f50f9fa1e..b1fa23729a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1607,6 +1607,11 @@ virDomainDriverSetupPersistentDefBlkioParams; # hypervisor/virclosecallbacks.h +virCloseCallbacksDomainAdd; +virCloseCallbacksDomainAlloc; +virCloseCallbacksDomainIsRegistered; +virCloseCallbacksDomainRemove; +virCloseCallbacksDomainRunForConn; virCloseCallbacksGet; virCloseCallbacksNew; virCloseCallbacksRun; -- 2.38.1

On Thu, Jan 05, 2023 at 05:29:56PM +0100, Peter Krempa wrote:
The new APIs store the list of callbacks for a VM inside the virDomainObj and also allow registering multiple callbacks for a single domain and also for multiple connections.
For now this code is dormant until each driver using the old APIs is not refactored to use the new APIs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/hypervisor/virclosecallbacks.c | 336 +++++++++++++++++++++++++++++ src/hypervisor/virclosecallbacks.h | 24 +++ src/libvirt_private.syms | 5 + 3 files changed, 365 insertions(+)
diff --git a/src/hypervisor/virclosecallbacks.c b/src/hypervisor/virclosecallbacks.c index a08464438a..21b97cce12 100644 --- a/src/hypervisor/virclosecallbacks.c +++ b/src/hypervisor/virclosecallbacks.c @@ -310,3 +310,339 @@ virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, VIR_FREE(list->entries); VIR_FREE(list); } + + +struct _virCloseCallbacksDomainData { + virConnectPtr conn; + virCloseCallback cb; +}; +typedef struct _virCloseCallbacksDomainData virCloseCallbacksDomainData; + + +static void +virCloseCallbacksDomainDataFree(virCloseCallbacksDomainData* data) +{ + g_free(data); +} +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCloseCallbacksDomainData, virCloseCallbacksDomainDataFree); + + +virClass *virCloseCallbacksDomainListClass; + +struct _virCloseCallbacksDomainList { + virObjectLockable parent; + + GList *callbacks; +}; +typedef struct _virCloseCallbacksDomainList virCloseCallbacksDomainList; + + +static void +virCloseCallbacksDomainListDispose(void *obj G_GNUC_UNUSED) +{ + virCloseCallbacksDomainList *cc = obj; + + g_list_free_full(cc->callbacks, (GDestroyNotify) virCloseCallbacksDomainDataFree); +} + + +static int +virCloseCallbacksDomainListOnceInit(void) +{ + if (!(VIR_CLASS_NEW(virCloseCallbacksDomainList, virClassForObjectLockable()))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virCloseCallbacksDomainList); + + +/** + * virCloseCallbacksDomainAlloc: + * + * Allocates and returns a data structure for holding close callback data in + * a virDomainObj. + */ +virObject * +virCloseCallbacksDomainAlloc(void) +{ + if (virCloseCallbacksDomainListInitialize() < 0) + abort(); + + return virObjectNew(virCloseCallbacksDomainListClass); +} + + +/** + * virCloseCallbacksDomainAdd: + * @vm: domain object + * @conn: pointer to the connection which should trigger the close callback + * @cb: pointer to the callback function + * + * Registers @cb as a connection close callback for the @conn connection with + * the @vm domain. Duplicate registrations are ignored. + * + * Caller must hold lock on @vm. + */ +void +virCloseCallbacksDomainAdd(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb)
Incorrect indentation. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

On 1/5/23 10:29 AM, Peter Krempa wrote:
The new APIs store the list of callbacks for a VM inside the virDomainObj and also allow registering multiple callbacks for a single domain and also for multiple connections.
For now this code is dormant until each driver using the old APIs is not refactored to use the new APIs.
I think you meant "is refactored" rather than "is not refactored" here.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/hypervisor/virclosecallbacks.c | 336 +++++++++++++++++++++++++++++ src/hypervisor/virclosecallbacks.h | 24 +++ src/libvirt_private.syms | 5 + 3 files changed, 365 insertions(+)
diff --git a/src/hypervisor/virclosecallbacks.c b/src/hypervisor/virclosecallbacks.c index a08464438a..21b97cce12 100644 --- a/src/hypervisor/virclosecallbacks.c +++ b/src/hypervisor/virclosecallbacks.c @@ -310,3 +310,339 @@ virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, VIR_FREE(list->entries); VIR_FREE(list); } + + +struct _virCloseCallbacksDomainData { + virConnectPtr conn; + virCloseCallback cb; +}; +typedef struct _virCloseCallbacksDomainData virCloseCallbacksDomainData; + + +static void +virCloseCallbacksDomainDataFree(virCloseCallbacksDomainData* data) +{ + g_free(data); +} +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCloseCallbacksDomainData, virCloseCallbacksDomainDataFree); + + +virClass *virCloseCallbacksDomainListClass; + +struct _virCloseCallbacksDomainList { + virObjectLockable parent; + + GList *callbacks; +}; +typedef struct _virCloseCallbacksDomainList virCloseCallbacksDomainList; + + +static void +virCloseCallbacksDomainListDispose(void *obj G_GNUC_UNUSED) +{ + virCloseCallbacksDomainList *cc = obj; + + g_list_free_full(cc->callbacks, (GDestroyNotify) virCloseCallbacksDomainDataFree); +} + + +static int +virCloseCallbacksDomainListOnceInit(void) +{ + if (!(VIR_CLASS_NEW(virCloseCallbacksDomainList, virClassForObjectLockable()))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virCloseCallbacksDomainList); + + +/** + * virCloseCallbacksDomainAlloc: + * + * Allocates and returns a data structure for holding close callback data in + * a virDomainObj. + */ +virObject * +virCloseCallbacksDomainAlloc(void) +{ + if (virCloseCallbacksDomainListInitialize() < 0) + abort(); + + return virObjectNew(virCloseCallbacksDomainListClass); +} + + +/** + * virCloseCallbacksDomainAdd: + * @vm: domain object + * @conn: pointer to the connection which should trigger the close callback + * @cb: pointer to the callback function + * + * Registers @cb as a connection close callback for the @conn connection with + * the @vm domain. Duplicate registrations are ignored. + * + * Caller must hold lock on @vm. + */ +void +virCloseCallbacksDomainAdd(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + if (!conn || !cb) + return; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + virCloseCallbacksDomainData *data; + GList *n; + + for (n = cc->callbacks; n; n = n->next) { + data = n->data; + + if (data->cb == cb && data->conn == conn) + return; + } + + data = g_new0(virCloseCallbacksDomainData, 1); + data->conn = conn; + data->cb = cb; + + cc->callbacks = g_list_prepend(cc->callbacks, data); + } +} + + +/** + * virCloseCallbacksDomainMatch: + * @data: pointer to a close callback data structure + * @conn: connection pointer matched against @data + * @cb: callback pointer matched against @data + * + * Returns true if the @data callback structure matches the requested @conn + * and/or @cb parameters. If either of @conn/@cb is NULL it is interpreted as + * a wildcard. + */ +static bool +virCloseCallbacksDomainMatch(virCloseCallbacksDomainData *data, + virConnectPtr conn, + virCloseCallback cb) +{ + if (conn && cb) + return data->conn == conn && data->cb == cb; + + if (conn) + return data->conn == conn; + + if (cb) + return data->cb == cb; + + return true; +} + + +/** + * virCloseCallbacksDomainIsRegistered: + * @vm: domain object + * @conn: connection pointer + * @cb: callback pointer + * + * Returns true if @vm has one or more matching (see virCloseCallbacksDomainMatch) + * callback(s) registered. Caller must hold lock on @vm. + */ +bool +virCloseCallbacksDomainIsRegistered(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n; + + for (n = cc->callbacks; n; n = n->next) { + virCloseCallbacksDomainData *data = n->data; + + if (virCloseCallbacksDomainMatch(data, conn, cb)) + return true; + } + } + + return false; +} + + +/** + * virCloseCallbacksDomainRemove: + * @vm: domain object + * @conn: connection pointer + * @cb: callback pointer + * + * Removes all the registered matching (see virCloseCallbacksDomainMatch) + * callbacks for @vm. Caller must hold lock on @vm. + */ +void +virCloseCallbacksDomainRemove(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n = cc->callbacks; + + while (n) { + GList *cur = n; + + n = n->next; + + if (virCloseCallbacksDomainMatch(cur->data, conn, cb)) { + cc->callbacks = g_list_remove_link(cc->callbacks, cur); + g_list_free_full(cur, (GDestroyNotify) virCloseCallbacksDomainDataFree); + } + } + } +} + + +/** + * virCloseCallbacksDomainFetchForConn: + * @vm: domain object + * @conn: pointer to connection being closed + * + * Fetches connection close callbacks for @conn from @vm. The fetched close + * callbacks are removed from the list of callbacks of @vm. This function + * must be called with lock on @vm held. Caller is responsible for freeing the + * returned list. + */ +static GList * +virCloseCallbacksDomainFetchForConn(virDomainObj *vm, + virConnectPtr conn) +{ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + GList *conncallbacks = NULL; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n; + + for (n = cc->callbacks; n;) { + virCloseCallbacksDomainData *data = n->data; + GList *cur = n; + + n = n->next; + + if (data->conn == conn) { + cc->callbacks = g_list_remove_link(cc->callbacks, cur); + conncallbacks = g_list_concat(cur, conncallbacks); + } + } + } + + return conncallbacks; +} + + +/** + * virCloseCallbacksDomainRun + * @vm: domain object + * @conn: pointer to connection being closed + * + * Fetches and sequentially calls all connection close callbacks for @conn from + * @vm. This function must be called with lock on @vm held. + */ +static void +virCloseCallbacksDomainRun(virDomainObj *vm, + virConnectPtr conn) +{ + g_autolist(virCloseCallbacksDomainData) callbacks = NULL; + GList *n; + + callbacks = virCloseCallbacksDomainFetchForConn(vm, conn); + + for (n = callbacks; n; n = n->next) { + virCloseCallbacksDomainData *data = n->data; + + VIR_DEBUG("vm='%s' cb='%p'", vm->def->name, data->cb); + + (data->cb)(vm, conn); + } +} + + +/** + * virCloseCallbacksDomainHasCallbackForConn: + * @vm: domain object + * @conn: connection being closed + * + * Returns true if @vm has a callback registered for the @conn connection. This + * function doesn't require a lock being held on @vm. + */ +static bool +virCloseCallbacksDomainHasCallbackForConn(virDomainObj *vm, + virConnectPtr conn) +{ + /* we can access vm->closecallbacks as it's a immutable pointer */ + virCloseCallbacksDomainList *cc = (virCloseCallbacksDomainList *) vm->closecallbacks; + + if (!cc) + return false; + + VIR_WITH_OBJECT_LOCK_GUARD(cc) { + GList *n; + + for (n = cc->callbacks; n; n = n->next) { + virCloseCallbacksDomainData *data = n->data; + + if (data->conn == conn) + return true; + } + } + + return false; +} + + +/** + * virCloseCallbacksDomainRunForConn: + * @domains: domain list object + * @conn: connection being closed + * + * Finds all domains in @domains which registered one or more connection close + * callbacks for @conn and calls the callbacks. This function is designed to + * be called from virDrvConnectClose function of individual drivers. + * + * To minimize lock contention the function first fetches a list of all domain + * objects, then checks whether a connect close callback is actually registered + * for the domain object and just then acquires the lock on the VM object. + */ +void +virCloseCallbacksDomainRunForConn(virDomainObjList *domains, + virConnectPtr conn) +{ + virDomainObj **vms = NULL; + size_t nvms; + size_t i; + + VIR_DEBUG("conn=%p", conn); + + virDomainObjListCollectAll(domains, &vms, &nvms); + + for (i = 0; i < nvms; i++) { + virDomainObj *vm = vms[i]; + + if (!virCloseCallbacksDomainHasCallbackForConn(vm, conn)) + continue; + + VIR_WITH_OBJECT_LOCK_GUARD(vm) { + /* VIR_WITH_OBJECT_LOCK_GUARD is a for loop, so this break applies to that */ + if (vm->removing) + break; + + virCloseCallbacksDomainRun(vm, conn); + } + } + + virObjectListFreeCount(vms, nvms); +} diff --git a/src/hypervisor/virclosecallbacks.h b/src/hypervisor/virclosecallbacks.h index 7afb0e5640..b471f6b160 100644 --- a/src/hypervisor/virclosecallbacks.h +++ b/src/hypervisor/virclosecallbacks.h @@ -49,3 +49,27 @@ void virCloseCallbacksRun(virCloseCallbacks *closeCallbacks, virConnectPtr conn, virDomainObjList *domains); + +/* ---- */ + +virObject * +virCloseCallbacksDomainAlloc(void); + +void +virCloseCallbacksDomainAdd(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb); + +void +virCloseCallbacksDomainRemove(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb); + +bool +virCloseCallbacksDomainIsRegistered(virDomainObj *vm, + virConnectPtr conn, + virCloseCallback cb); + +void +virCloseCallbacksDomainRunForConn(virDomainObjList *domains, + virConnectPtr conn); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8f50f9fa1e..b1fa23729a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1607,6 +1607,11 @@ virDomainDriverSetupPersistentDefBlkioParams;
# hypervisor/virclosecallbacks.h +virCloseCallbacksDomainAdd; +virCloseCallbacksDomainAlloc; +virCloseCallbacksDomainIsRegistered; +virCloseCallbacksDomainRemove; +virCloseCallbacksDomainRunForConn; virCloseCallbacksGet; virCloseCallbacksNew; virCloseCallbacksRun;

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:29:57PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:29:58PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:29:59PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:00PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:01PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:02PM +0100, Peter Krempa wrote:
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@redhat.com> --- po/POTFILES | 1 - src/hypervisor/virclosecallbacks.c | 282 ----------------------------- src/hypervisor/virclosecallbacks.h | 27 --- src/libvirt_private.syms | 5 - 4 files changed, 315 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Require check of return value of the ACL checking functions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:03PM +0100, Peter Krempa wrote:
Require check of return value of the ACL checking functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/gendispatch.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:04PM +0100, Peter Krempa wrote:
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@redhat.com> --- src/remote/remote_driver.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:05PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_driver.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Convert to a switch instead of a bunch of 'if (type == ...). Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:06PM +0100, Peter Krempa wrote:
Convert to a switch instead of a bunch of 'if (type == ...).
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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)) {
The typecast is probably not necessary as it already returns correct type but it doesn't hurt as well. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:07PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

The test is superseded by 'disk-backing-chains-(no)index' cases. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../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

On Thu, Jan 05, 2023 at 05:30:08PM +0100, Peter Krempa wrote:
The test is superseded by 'disk-backing-chains-(no)index' cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../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
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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:
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@redhat.com> --- 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); +
This file uses only single line spacing. I would probably go with virDomainAssociateFD() as it reads slightly better and we have a lot of APIs with that order, one example dealing with FDs is virDomainOpenGraphicsFD(). Since there is no rule for API naming I'm OK with the one you used, just wanted to mention it in case you will find the other name better. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

On Thu, Jan 05, 2023 at 05:30:09PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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;
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

On Fri, Jan 06, 2023 at 13:40:03 +0100, Pavel Hrdina wrote:
On Thu, Jan 05, 2023 at 05:30:09PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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;
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?
No, VIR_DOMAIN_FD_ASSOCIATE_SECLABEL_RESTORE_REQUIRE can be actually deleted for now and re-introduced once it's implemented.

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:10PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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")
s/Try/try/ to make it consistent with the remaining help strings
+ }, + {.name = "seclabel-restore-require", + .type = VSH_OT_BOOL, + .help = N_("require that security label is restored after use") + }, + {.name = NULL} +};
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:11PM +0100, Peter Krempa wrote:
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@redhat.com> --- src/conf/storage_source_conf.c | 41 ++++++++++++++++++++++++++++++++++ src/conf/storage_source_conf.h | 17 ++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 59 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:12PM +0100, Peter Krempa wrote:
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@redhat.com> --- src/qemu/qemu_domain.c | 2 ++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:13PM +0100, Peter Krempa wrote:
Introduce a new argument type for testQemuInfoSetArgs named ARG_FD_GROUP which allows users to instantiate tests with populated FD passing hash table.
I don't like when we need to alter the code in order to be able to test it. There should be another better way of ensuring we have stable FDs for tests. Not sure how difficult it would be to change it so we need to go with this approach for now but would be nice to rework this by follow up patches.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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

On Fri, Jan 06, 2023 at 14:06:50 +0100, Pavel Hrdina wrote:
On Thu, Jan 05, 2023 at 05:30:13PM +0100, Peter Krempa wrote:
Introduce a new argument type for testQemuInfoSetArgs named ARG_FD_GROUP which allows users to instantiate tests with populated FD passing hash table.
I don't like when we need to alter the code in order to be able to test it. There should be another better way of ensuring we have stable FDs for tests. Not sure how difficult it would be to change it so we need to go with this approach for now but would be nice to rework this by follow up patches.
All the other code uses invalid FD numbers. That doesn't work very well when you need to duplicate them though. When we'd mock 'dup' then it becomes order dependant which is very hard to debug especially when combined with LD_PRELOAD-ed libraries we use for mocking. The advantage here is that we don't have fake FDs, thus dup works and it's the output doesn't change based on the system. Obviously for real usage we don't care about the specific numbers.

On Fri, Jan 06, 2023 at 02:28:39PM +0100, Peter Krempa wrote:
On Fri, Jan 06, 2023 at 14:06:50 +0100, Pavel Hrdina wrote:
On Thu, Jan 05, 2023 at 05:30:13PM +0100, Peter Krempa wrote:
Introduce a new argument type for testQemuInfoSetArgs named ARG_FD_GROUP which allows users to instantiate tests with populated FD passing hash table.
I don't like when we need to alter the code in order to be able to test it. There should be another better way of ensuring we have stable FDs for tests. Not sure how difficult it would be to change it so we need to go with this approach for now but would be nice to rework this by follow up patches.
All the other code uses invalid FD numbers. That doesn't work very well when you need to duplicate them though.
When we'd mock 'dup' then it becomes order dependant which is very hard to debug especially when combined with LD_PRELOAD-ed libraries we use for mocking.
The advantage here is that we don't have fake FDs, thus dup works and it's the output doesn't change based on the system.
Obviously for real usage we don't care about the specific numbers.
Yeah I figured there might be some catch that I did not realize. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:14PM +0100, Peter Krempa wrote:
The 'fdgroup' will allow users to specify a passed FD (via the 'virDomainFDAssociate()' API) use instead of opening a path. This is
/ use/to be used/ most likely or something similar as the current sentence doesn't make sense
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@redhat.com> --- 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
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:15PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:16PM +0100, Peter Krempa wrote:
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@redhat.com> --- src/conf/storage_source_conf.c | 7 +++++++ src/conf/storage_source_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 11 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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@redhat.com> --- 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

On Thu, Jan 05, 2023 at 05:30:17PM +0100, Peter Krempa wrote:
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@redhat.com> --- 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(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Prepare the internal data for passing FDs instead of having qemu open the file internally. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 Thu, Jan 05, 2023 at 05:30:18PM +0100, Peter Krempa wrote:
Prepare the internal data for passing FDs instead of having qemu open the file internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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. */
What happens if users passes multiple FDs but none of them are read-only? Otherwise looks good so if this is not an issue Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

On Fri, Jan 06, 2023 at 14:33:51 +0100, Pavel Hrdina wrote:
On Thu, Jan 05, 2023 at 05:30:18PM +0100, Peter Krempa wrote:
Prepare the internal data for passing FDs instead of having qemu open the file internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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. */
What happens if users passes multiple FDs but none of them are read-only? Otherwise looks good so if this is not an issue
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.

DAC security label is irrelevant once you have the FD. Disable all labelling for such images. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_dac.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 917fcf76a3..4036a2c27a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -881,6 +881,10 @@ virSecurityDACSetImageLabelInternal(virSecurityManager *mgr, if (!priv->dynamicOwnership) return 0; + /* Images passed via FD don't need DAC seclabel change */ + if (virStorageSourceIsFD(src)) + return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && !secdef->relabel) return 0; @@ -992,6 +996,10 @@ virSecurityDACRestoreImageLabelSingle(virSecurityManager *mgr, if (src->readonly || src->shared) return 0; + /* Images passed via FD don't need DAC seclabel change */ + if (virStorageSourceIsFD(src)) + return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && !secdef->relabel) return 0; @@ -1112,10 +1120,14 @@ virSecurityDACMoveImageMetadata(virSecurityManager *mgr, if (!priv->dynamicOwnership) return 0; - if (src && virStorageSourceIsLocalStorage(src)) + if (src && + virStorageSourceIsLocalStorage(src) && + !virStorageSourceIsFD(src)) data.src = src->path; - if (dst && virStorageSourceIsLocalStorage(dst)) + if (dst && + virStorageSourceIsLocalStorage(dst) && + !virStorageSourceIsFD(dst)) data.dst = dst->path; if (!data.src) -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:19PM +0100, Peter Krempa wrote:
DAC security label is irrelevant once you have the FD. Disable all labelling for such images.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_dac.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Unfortunately unlike with DAC we can't simply ignore labelling for the FD and it also influences the on-disk state. Thus we need to relabel the FD and we also store the existing label in cases when the user will request best-effort label replacement. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 1 + src/conf/storage_source_conf.h | 3 +++ src/security/security_selinux.c | 32 +++++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index c16016aabc..c647fc3c2f 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -1399,6 +1399,7 @@ virStorageSourceFDTupleFinalize(GObject *object) g_free(fdt->fds); g_free(fdt->testfds); + g_free(fdt->selinuxLabel); 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 f981261ff4..14a6825d54 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -269,6 +269,9 @@ struct _virStorageSourceFDTuple { /* connection this FD tuple is associated with for auto-closing */ virConnect *conn; + + /* original selinux label when we relabel the image */ + char *selinuxLabel; }; G_DECLARE_FINAL_TYPE(virStorageSourceFDTuple, vir_storage_source_fd_tuple, VIR, STORAGE_SOURCE_FD_TUPLE, GObject); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 93cc12407a..a42d86216a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1741,6 +1741,19 @@ virSecuritySELinuxRestoreImageLabelSingle(virSecurityManager *mgr, if (src->readonly || src->shared) return 0; + if (virStorageSourceIsFD(src)) { + if (migrated) + return 0; + + if (!src->fdtuple || + !src->fdtuple->selinuxLabel || + src->fdtuple->nfds == 0) + return 0; + + ignore_value(virSecuritySELinuxFSetFilecon(src->fdtuple->fds[0], + src->fdtuple->selinuxLabel)); + return 0; + } /* If we have a shared FS and are doing migration, we must not change * ownership, because that kills access on the destination host which is @@ -1888,7 +1901,24 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, path = vfioGroupDev; } - ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember); + if (virStorageSourceIsFD(src)) { + /* We can only really do labelling when we have the FD as the path + * may not be accessible for us */ + if (!src->fdtuple || src->fdtuple->nfds == 0) + return 0; + + /* force a writable label for the image if requested */ + if (src->fdtuple->writable && secdef->imagelabel) + use_label = secdef->imagelabel; + + /* store the existing selinux label for the image */ + if (!src->fdtuple->selinuxLabel) + fgetfilecon_raw(src->fdtuple->fds[0], &src->fdtuple->selinuxLabel); + + ret = virSecuritySELinuxFSetFilecon(src->fdtuple->fds[0], use_label); + } else { + ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember); + } if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:20PM +0100, Peter Krempa wrote:
Unfortunately unlike with DAC we can't simply ignore labelling for the FD and it also influences the on-disk state.
Thus we need to relabel the FD and we also store the existing label in cases when the user will request best-effort label replacement.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 1 + src/conf/storage_source_conf.h | 3 +++ src/security/security_selinux.c | 32 +++++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

We assume that FD passed images already exist so all existance checks are skipped. For the case that a FD-passed image is passed without a terminated backing chain (thus forcing us to detect) we attempt to read the header from the FD. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++--------- src/storage_file/storage_source.c | 14 ++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7dc4ef4ddb..38883a57d8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7679,16 +7679,20 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, disksrc->format > VIR_STORAGE_FILE_NONE && disksrc->format < VIR_STORAGE_FILE_BACKING) { + /* terminate the chain for such images as the code below would do */ + if (!disksrc->backingStore) + disksrc->backingStore = virStorageSourceNew(); + + /* we assume that FD-passed disks always exist */ + if (virStorageSourceIsFD(disksrc)) + return 0; + if (!virFileExists(disksrc->path)) { virStorageSourceReportBrokenChain(errno, disksrc, disksrc); return -1; } - /* terminate the chain for such images as the code below would do */ - if (!disksrc->backingStore) - disksrc->backingStore = virStorageSourceNew(); - /* host cdrom requires special treatment in qemu, so we need to check * whether a block device is a cdrom */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && @@ -7700,12 +7704,14 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, return 0; } - src = disksrc; /* skip to the end of the chain if there is any */ - while (virStorageSourceHasBacking(src)) { - int rv = virStorageSourceSupportsAccess(src); + for (src = disksrc; virStorageSourceHasBacking(src); src = src->backingStore) { + int rv; + + if (virStorageSourceIsFD(src)) + continue; - if (rv < 0) + if ((rv = virStorageSourceSupportsAccess(src)) < 0) return -1; if (rv > 0) { @@ -7720,7 +7726,6 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, virStorageSourceDeinit(src); } - src = src->backingStore; } /* We skipped to the end of the chain. Skip detection if there's the diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index ab0cdf2b12..00b28f73a6 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -1264,6 +1264,20 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSource *src, int ret = -1; ssize_t len; + if (virStorageSourceIsFD(src)) { + if (!src->fdtuple) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("fd passed image source not initialized")); + return -1; + } + + return virFileReadHeaderFD(src->fdtuple->fds[0], VIR_STORAGE_MAX_HEADER, + buf); + } + + if (src->fdgroup) + return 0; + if (virStorageSourceInitAs(src, uid, gid) < 0) return -1; -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:21PM +0100, Peter Krempa wrote:
We assume that FD passed images already exist so all existance checks are skipped.
For the case that a FD-passed image is passed without a terminated backing chain (thus forcing us to detect) we attempt to read the header from the FD.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++--------- src/storage_file/storage_source.c | 14 ++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7dc4ef4ddb..38883a57d8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7679,16 +7679,20 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, disksrc->format > VIR_STORAGE_FILE_NONE && disksrc->format < VIR_STORAGE_FILE_BACKING) {
+ /* terminate the chain for such images as the code below would do */ + if (!disksrc->backingStore) + disksrc->backingStore = virStorageSourceNew(); + + /* we assume that FD-passed disks always exist */ + if (virStorageSourceIsFD(disksrc)) + return 0; + if (!virFileExists(disksrc->path)) { virStorageSourceReportBrokenChain(errno, disksrc, disksrc);
return -1; }
- /* terminate the chain for such images as the code below would do */ - if (!disksrc->backingStore) - disksrc->backingStore = virStorageSourceNew(); - /* host cdrom requires special treatment in qemu, so we need to check * whether a block device is a cdrom */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && @@ -7700,12 +7704,14 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, return 0; }
- src = disksrc; /* skip to the end of the chain if there is any */ - while (virStorageSourceHasBacking(src)) { - int rv = virStorageSourceSupportsAccess(src); + for (src = disksrc; virStorageSourceHasBacking(src); src = src->backingStore) { + int rv; + + if (virStorageSourceIsFD(src)) + continue;
- if (rv < 0) + if ((rv = virStorageSourceSupportsAccess(src)) < 0) return -1;
if (rv > 0) { @@ -7720,7 +7726,6 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver,
virStorageSourceDeinit(src); } - src = src->backingStore; }
/* We skipped to the end of the chain. Skip detection if there's the diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index ab0cdf2b12..00b28f73a6 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -1264,6 +1264,20 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSource *src, int ret = -1; ssize_t len;
+ if (virStorageSourceIsFD(src)) { + if (!src->fdtuple) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("fd passed image source not initialized")); + return -1; + } + + return virFileReadHeaderFD(src->fdtuple->fds[0], VIR_STORAGE_MAX_HEADER, + buf); + }
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.
+ + if (src->fdgroup) + return 0;
This seems like no-op as virStorageSourceIsFD() does the same. Pavel
if (virStorageSourceInitAs(src, uid, gid) < 0) return -1;
-- 2.38.1

Probing stats and block copy to a FD passed image is not yet supported. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c086d4069b..4df7f42ae4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10499,6 +10499,13 @@ qemuDomainBlockPeek(virDomainPtr dom, goto cleanup; } + if (virStorageSourceIsFD(disk->src)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("peeking is not supported for FD-passed images")); + goto cleanup; + + } + if (qemuDomainStorageFileInit(driver, vm, disk->src, NULL) < 0) goto cleanup; @@ -10858,6 +10865,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto endjob; } + if (virStorageSourceIsFD(disk->src)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("block info is not supported for fd-passed disk image")); + goto endjob; + } + /* for inactive domains we have to peek into the files */ if (!virDomainObjIsActive(vm)) { if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src, false)) < 0) @@ -14689,6 +14702,12 @@ qemuDomainBlockCopyCommon(virDomainObj *vm, if (!qemuDomainDiskBlockJobIsSupported(disk)) goto endjob; + if (virStorageSourceIsFD(mirror)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("copy to a FD passed disk source is not yet supported")); + goto endjob; + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN && virDomainDiskDefSourceLUNValidate(mirror) < 0) goto endjob; @@ -17942,6 +17961,9 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriver *driver, if (virStorageSourceIsEmpty(src)) return 0; + if (virStorageSourceIsFD(src)) + return 0; + if (qemuStorageLimitsRefresh(driver, cfg, dom, src, true) <= 0) { virResetLastError(); return 0; -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:22PM +0100, Peter Krempa wrote:
Probing stats and block copy to a FD passed image is not yet supported.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c086d4069b..4df7f42ae4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10499,6 +10499,13 @@ qemuDomainBlockPeek(virDomainPtr dom, goto cleanup; }
+ if (virStorageSourceIsFD(disk->src)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("peeking is not supported for FD-passed images"));
s/FD-passed/FD passed/
+ goto cleanup; + + } + if (qemuDomainStorageFileInit(driver, vm, disk->src, NULL) < 0) goto cleanup;
@@ -10858,6 +10865,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto endjob; }
+ if (virStorageSourceIsFD(disk->src)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("block info is not supported for fd-passed disk image"));
s/fd-passed/FD passed/
+ goto endjob; + } + /* for inactive domains we have to peek into the files */ if (!virDomainObjIsActive(vm)) { if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src, false)) < 0) @@ -14689,6 +14702,12 @@ qemuDomainBlockCopyCommon(virDomainObj *vm, if (!qemuDomainDiskBlockJobIsSupported(disk)) goto endjob;
+ if (virStorageSourceIsFD(mirror)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("copy to a FD passed disk source is not yet supported")); + goto endjob; + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN && virDomainDiskDefSourceLUNValidate(mirror) < 0) goto endjob; @@ -17942,6 +17961,9 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriver *driver, if (virStorageSourceIsEmpty(src)) return 0;
+ if (virStorageSourceIsFD(src)) + return 0; + if (qemuStorageLimitsRefresh(driver, cfg, dom, src, true) <= 0) { virResetLastError(); return 0;
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_cgroup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 9cf2d6474a..aac7c70054 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -206,7 +206,9 @@ qemuSetupImageCgroupInternal(virDomainObj *vm, if (qemuSetupImagePathCgroup(vm, QEMU_DEV_VFIO, false) < 0) return -1; } else { - if (!src->path || !virStorageSourceIsLocalStorage(src)) { + if (!src->path || + !virStorageSourceIsLocalStorage(src) || + virStorageSourceIsFD(src)) { VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", NULLSTR(src->path), virStorageTypeToString(src->type)); return 0; -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:23PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_cgroup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Assert support for VIR_DOMAIN_DEF_FEATURE_DISK_FD in the qemu driver now that all code paths are adapted. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 38883a57d8..b341f43b56 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6002,7 +6002,8 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS | VIR_DOMAIN_DEF_FEATURE_USER_ALIAS | VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT | - VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING, + VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING | + VIR_DOMAIN_DEF_FEATURE_DISK_FD, }; -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:24PM +0100, Peter Krempa wrote:
Assert support for VIR_DOMAIN_DEF_FEATURE_DISK_FD in the qemu driver now that all code paths are adapted.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Enable the qemuxml2xml variant and add output data for qemuxml2argvtest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-source-fd.x86_64-latest.args | 49 +++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ .../disk-source-fd.x86_64-latest.xml | 52 +++++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 4 files changed, 107 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args create mode 100644 tests/qemuxml2xmloutdata/disk-source-fd.x86_64-latest.xml diff --git a/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args b/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args new file mode 100644 index 0000000000..b4a81acfc7 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args @@ -0,0 +1,49 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-accel tcg \ +-cpu qemu64 \ +-m 214 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-add-fd set=2,fd=700,opaque=libvirt-4-storage0 \ +-add-fd set=2,fd=705,opaque=libvirt-4-storage1 \ +-blockdev '{"driver":"file","filename":"/dev/fdset/2","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-4-format","read-only":false,"driver":"qcow2","file":"libvirt-4-storage"}' \ +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-4-format","id":"virtio-disk4","bootindex":1}' \ +-add-fd set=0,fd=704,opaque=libvirt-1-storage0 \ +-add-fd set=1,fd=777,opaque=libvirt-2-storage0 \ +-add-fd set=1,fd=778,opaque=libvirt-2-storage1 \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/rhel7.1484071876","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-3-format","read-only":true,"driver":"qcow2","file":"libvirt-3-storage","backing":null}' \ +-blockdev '{"driver":"file","filename":"/dev/fdset/1","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2","file":"libvirt-2-storage","backing":"libvirt-3-format"}' \ +-blockdev '{"driver":"file","filename":"/dev/fdset/0","node-name":"libvirt-1-storage","read-only":false,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage","backing":"libvirt-2-format"}' \ +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-1-format","id":"virtio-disk5"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x4"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b4b60a0130..6e027cf0bb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1344,6 +1344,10 @@ mymain(void) DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-incompatible-address"); DO_TEST_CAPS_LATEST("disk-backing-chains-index"); DO_TEST_CAPS_LATEST("disk-backing-chains-noindex"); + DO_TEST_CAPS_ARCH_LATEST_FULL("disk-source-fd", "x86_64", + ARG_FD_GROUP, "testgroup2", 2, 700, 705, + ARG_FD_GROUP, "testgroup5", 1, 704, + ARG_FD_GROUP, "testgroup6", 2, 777, 778); DO_TEST_CAPS_LATEST("disk-slices"); DO_TEST_CAPS_LATEST("disk-rotation"); diff --git a/tests/qemuxml2xmloutdata/disk-source-fd.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-source-fd.x86_64-latest.xml new file mode 100644 index 0000000000..9ab5e9443f --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-source-fd.x86_64-latest.xml @@ -0,0 +1,52 @@ +<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> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <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'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </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'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <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='0x04' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8d885d6d9f..5ef3645255 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -909,6 +909,8 @@ mymain(void) DO_TEST_NOCAPS("disk-backing-chains-index"); DO_TEST_NOCAPS("disk-backing-chains-noindex"); + DO_TEST_CAPS_LATEST("disk-source-fd"); + DO_TEST_CAPS_LATEST("disk-network-http"); DO_TEST("chardev-label", -- 2.38.1

On Thu, Jan 05, 2023 at 05:30:25PM +0100, Peter Krempa wrote:
Enable the qemuxml2xml variant and add output data for qemuxml2argvtest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-source-fd.x86_64-latest.args | 49 +++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ .../disk-source-fd.x86_64-latest.xml | 52 +++++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 4 files changed, 107 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-source-fd.x86_64-latest.args create mode 100644 tests/qemuxml2xmloutdata/disk-source-fd.x86_64-latest.xml
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (3)
-
Jonathon Jongsma
-
Pavel Hrdina
-
Peter Krempa