[RFC REPOST 0/7] introduce support for live appid updates
Rebased on top of current master. I'm posting this as an RFC mainly because I'm not sure how to model the new API. This patches introduce a new naive API that will change only the APPID and nothing else. Currently there are no other known features related to Fibre Channel resources so this non-extendable API will be sufficient, however the appid lives in <resource> element in the XML where we currently have root cgroup partition. Even though changing the partition will not be supported and we don't know about anything else that could be placed here it doesn't mean it will not happen in the future. In that case we would have to add new API as well. So I'm wondering if we should create a more generic API that would take typed parameters as arguments: int virDomainSetResource(virDomainPtr domain, virTypedParameterPtr params, int nparams, unsigned int flags) Any ideas? Pavel Hrdina (7): conf: extract appid validation to virDomainDefResourceAppidValidate cgroup: extract setting fibre channel appid into virCgroupSetFCAppid virCgroupSetFCAppid: properly handle when appid is NULL src: introduce virDomainSetFibreChannelAppid API remote: add RPC support for the virDomainSetFibreChannelAppid API qemu: implement virDomainSetFibreChannelAppid API tools: introduce virsh setappid command docs/manpages/virsh.rst | 14 ++++++ include/libvirt/libvirt-domain.h | 4 ++ src/conf/domain_validate.c | 42 ++++++++++-------- src/conf/domain_validate.h | 2 + src/driver-hypervisor.h | 6 +++ src/libvirt-domain.c | 44 +++++++++++++++++++ src/libvirt_private.syms | 2 + src/libvirt_public.syms | 1 + src/qemu/qemu_cgroup.c | 17 +------- src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++++- src/remote_protocol-structs | 6 +++ src/util/vircgroup.c | 24 ++++++++++ src/util/vircgroup.h | 3 ++ tools/virsh-domain.c | 65 +++++++++++++++++++++++++++ 16 files changed, 286 insertions(+), 34 deletions(-) -- 2.31.1
This will be needed by future patches adding appid API to allow changing it for running VMs. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_validate.c | 42 +++++++++++++++++++++++--------------- src/conf/domain_validate.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 1bc62c364d..3ab864bbeb 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -58,29 +58,37 @@ virDomainDefBootValidate(const virDomainDef *def) #define APPID_LEN_MIN 1 #define APPID_LEN_MAX 128 +int +virDomainDefResourceAppidValidate(const char *appid) +{ + int len; + + if (!virStringIsPrintable(appid)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Fibre Channel 'appid' is not a printable string")); + return -1; + } + + len = strlen(appid); + if (len < APPID_LEN_MIN || len > APPID_LEN_MAX) { + virReportError(VIR_ERR_INVALID_ARG, + _("Fibre Channel 'appid' string length must be between [%d, %d]"), + APPID_LEN_MIN, APPID_LEN_MAX); + return -1; + } + + return 0; +} + + static int virDomainDefResourceValidate(const virDomainDef *def) { if (!def->resource) return 0; - if (def->resource->appid) { - int len; - - if (!virStringIsPrintable(def->resource->appid)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Fibre Channel 'appid' is not a printable string")); - return -1; - } - - len = strlen(def->resource->appid); - if (len < APPID_LEN_MIN || len > APPID_LEN_MAX) { - virReportError(VIR_ERR_INVALID_ARG, - _("Fibre Channel 'appid' string length must be between [%d, %d]"), - APPID_LEN_MIN, APPID_LEN_MAX); - return -1; - } - } + if (def->resource->appid) + return virDomainDefResourceAppidValidate(def->resource->appid); return 0; } diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h index 430d61fd3c..02fc16b17d 100644 --- a/src/conf/domain_validate.h +++ b/src/conf/domain_validate.h @@ -42,3 +42,5 @@ int virDomainDeviceDefValidate(const virDomainDeviceDef *dev, int virDomainDiskDefValidateSource(const virStorageSource *src); int virDomainDiskDefSourceLUNValidate(const virStorageSource *src); + +int virDomainDefResourceAppidValidate(const char *appid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2778fe7f8f..d1f5dc2080 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -765,6 +765,7 @@ virDomainConfVMNWFilterTeardown; # conf/domain_validate.h virDomainActualNetDefValidate; +virDomainDefResourceAppidValidate; virDomainDefValidate; virDomainDeviceValidateAliasForHotplug; virDomainDiskDefSourceLUNValidate; -- 2.31.1
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
This will be needed by future patches adding appid API to allow changing it for running VMs.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_validate.c | 42 +++++++++++++++++++++++--------------- src/conf/domain_validate.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 1bc62c364d..3ab864bbeb 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c
+ if (def->resource->appid) + return virDomainDefResourceAppidValidate(def->resource->appid);
I'd write this as: if (def->resource->appid && virDomainDefResourceAppidValidate() < 0) return -1; so that this line doesn't have to be changed when something new is added to this function. Moreover, you can have virDomainDefResourceAppidValidate() to be NOP if appid == NULL and write this check as: if (virDomainDefResourceAppidValidate() < 0) return -1; Michal
On Fri, Sep 10, 2021 at 01:49:24PM +0200, Michal Prívozník wrote:
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
This will be needed by future patches adding appid API to allow changing it for running VMs.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_validate.c | 42 +++++++++++++++++++++++--------------- src/conf/domain_validate.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 1bc62c364d..3ab864bbeb 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c
+ if (def->resource->appid) + return virDomainDefResourceAppidValidate(def->resource->appid);
I'd write this as:
if (def->resource->appid && virDomainDefResourceAppidValidate() < 0) return -1;
so that this line doesn't have to be changed when something new is added to this function. Moreover, you can have virDomainDefResourceAppidValidate() to be NOP if appid == NULL and write this check as:
if (virDomainDefResourceAppidValidate() < 0) return -1;
True, I'll fix that. Thanks. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 17 +---------------- src/util/vircgroup.c | 24 ++++++++++++++++++++++++ src/util/vircgroup.h | 3 +++ 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1f5dc2080..8bb349e7da 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1957,6 +1957,7 @@ virCgroupSetCpusetCpus; virCgroupSetCpusetMemoryMigrate; virCgroupSetCpusetMems; virCgroupSetCpuShares; +virCgroupSetFCAppid; virCgroupSetFreezerState; virCgroupSetMemory; virCgroupSetMemoryHardLimit; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 6d4a82b3cd..9eec9db65c 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -908,27 +908,12 @@ static int qemuSetupCgroupAppid(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; - int inode = -1; - const char *path = "/sys/class/fc/fc_udev_device/appid_store"; - g_autofree char *appid = NULL; virDomainResourceDef *resource = vm->def->resource; if (!resource || !resource->appid) return 0; - inode = virCgroupGetInode(priv->cgroup); - if (inode < 0) - return -1; - - appid = g_strdup_printf("%X:%s", inode, resource->appid); - - if (virFileWriteStr(path, appid, 0) < 0) { - virReportSystemError(errno, - _("Unable to write '%s' to '%s'"), appid, path); - return -1; - } - - return 0; + return virCgroupSetFCAppid(priv->cgroup, resource->appid); } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 37b63a2e2d..ad0ee20862 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4012,3 +4012,27 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period, return 0; } + + +int +virCgroupSetFCAppid(virCgroup *group, + const char *appid) +{ + int inode = -1; + const char *path = "/sys/class/fc/fc_udev_device/appid_store"; + g_autofree char *vmid = NULL; + + inode = virCgroupGetInode(group); + if (inode < 0) + return -1; + + vmid = g_strdup_printf("%X:%s", inode, appid); + + if (virFileWriteStr(path, vmid, 0) < 0) { + virReportSystemError(errno, + _("Unable to write '%s' to '%s'"), vmid, path); + return -1; + } + + return 0; +} diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 690f09465c..a69b435b71 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -285,3 +285,6 @@ int virCgroupHasEmptyTasks(virCgroup *cgroup, int controller); bool virCgroupControllerAvailable(int controller); int virCgroupGetInode(virCgroup *cgroup); + +int virCgroupSetFCAppid(virCgroup *group, + const char *appid); -- 2.31.1
With introduction of live changes of appid we should also support removal of the appid from VM. This is done by writing empty appid part to appid_store file. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index ad0ee20862..9470ab061d 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4026,7 +4026,7 @@ virCgroupSetFCAppid(virCgroup *group, if (inode < 0) return -1; - vmid = g_strdup_printf("%X:%s", inode, appid); + vmid = g_strdup_printf("%X:%s", inode, appid ? appid : ""); if (virFileWriteStr(path, vmid, 0) < 0) { virReportSystemError(errno, -- 2.31.1
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
With introduction of live changes of appid we should also support removal of the appid from VM. This is done by writing empty appid part to appid_store file.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index ad0ee20862..9470ab061d 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4026,7 +4026,7 @@ virCgroupSetFCAppid(virCgroup *group, if (inode < 0) return -1;
- vmid = g_strdup_printf("%X:%s", inode, appid); + vmid = g_strdup_printf("%X:%s", inode, appid ? appid : "");
Or NULLSTR_EMPTY(appid) instead of ternary operator. Michal
On Fri, Sep 10, 2021 at 01:49:22PM +0200, Michal Prívozník wrote:
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
With introduction of live changes of appid we should also support removal of the appid from VM. This is done by writing empty appid part to appid_store file.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index ad0ee20862..9470ab061d 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4026,7 +4026,7 @@ virCgroupSetFCAppid(virCgroup *group, if (inode < 0) return -1;
- vmid = g_strdup_printf("%X:%s", inode, appid); + vmid = g_strdup_printf("%X:%s", inode, appid ? appid : "");
Or NULLSTR_EMPTY(appid) instead of ternary operator.
Nice :) didn't know we have a helper for that. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- include/libvirt/libvirt-domain.h | 4 +++ src/driver-hypervisor.h | 6 +++++ src/libvirt-domain.c | 44 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 55 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 7ef8ac51e5..4f7b88ef61 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5153,4 +5153,8 @@ int virDomainStartDirtyRateCalc(virDomainPtr domain, int seconds, unsigned int flags); +int virDomainSetFibreChannelAppid(virDomainPtr domain, + const char *appid, + unsigned int flags); + #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index d642af8a37..e6b1ceb3ce 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1412,6 +1412,11 @@ typedef int typedef struct _virHypervisorDriver virHypervisorDriver; +typedef int +(*virDrvDomainSetFibreChannelAppid)(virDomainPtr domain, + const char *appid, + unsigned int flags); + /** * _virHypervisorDriver: * @@ -1676,4 +1681,5 @@ struct _virHypervisorDriver { virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet; virDrvDomainGetMessages domainGetMessages; virDrvDomainStartDirtyRateCalc domainStartDirtyRateCalc; + virDrvDomainSetFibreChannelAppid domainSetFibreChannelAppid; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a8a386e839..f3e6854a39 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13229,3 +13229,47 @@ virDomainStartDirtyRateCalc(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainSetFibreChannelAppid: + * @domain: a domain object + * @appid: user provided appid string + * @flags: extra flags + * + * Set the Fibre Channel APPID. Accepts only printable characters + * and maximal length is 128 characters. To remove the APPID use + * NULL as @appid value. + * + * Returns 0 in case of success, -1 otherwise. + */ +int +virDomainSetFibreChannelAppid(virDomainPtr domain, + const char *appid, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "appid=%s, flags=0x%x", appid, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainSetFibreChannelAppid) { + int ret; + ret = conn->driver->domainSetFibreChannelAppid(domain, appid, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 3a5fa7cb09..912b421632 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -900,6 +900,7 @@ LIBVIRT_7.7.0 { global: virNWFilterDefineXMLFlags; virNetworkDefineXMLFlags; + virDomainSetFibreChannelAppid; } LIBVIRT_7.3.0; # .... define new API here using predicted next version number .... -- 2.31.1
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- include/libvirt/libvirt-domain.h | 4 +++ src/driver-hypervisor.h | 6 +++++ src/libvirt-domain.c | 44 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 55 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a8a386e839..f3e6854a39 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13229,3 +13229,47 @@ virDomainStartDirtyRateCalc(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainSetFibreChannelAppid: + * @domain: a domain object + * @appid: user provided appid string + * @flags: extra flags
I like the following more: * @flags: extra flags; not used yet, so callers should always pass 0
+ * + * Set the Fibre Channel APPID. Accepts only printable characters + * and maximal length is 128 characters. To remove the APPID use + * NULL as @appid value. + * + * Returns 0 in case of success, -1 otherwise. + */
Michal
On Fri, Sep 10, 2021 at 01:49:20PM +0200, Michal Prívozník wrote:
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- include/libvirt/libvirt-domain.h | 4 +++ src/driver-hypervisor.h | 6 +++++ src/libvirt-domain.c | 44 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 55 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a8a386e839..f3e6854a39 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13229,3 +13229,47 @@ virDomainStartDirtyRateCalc(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainSetFibreChannelAppid: + * @domain: a domain object + * @appid: user provided appid string + * @flags: extra flags
I like the following more:
* @flags: extra flags; not used yet, so callers should always pass 0
Not true, @flags are used to control if we are modifying live or config XML. Pavel
+ * + * Set the Fibre Channel APPID. Accepts only printable characters + * and maximal length is 128 characters. To remove the APPID use + * NULL as @appid value. + * + * Returns 0 in case of success, -1 otherwise. + */
Michal
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b64a86af63..55a0b79f95 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8539,6 +8539,7 @@ static virHypervisorDriver hypervisor_driver = { .domainAuthorizedSSHKeysSet = remoteDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = remoteDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = remoteDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainSetFibreChannelAppid = remoteDomainSetFibreChannelAppid, /* 7.7.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index df1b126b0c..0a64504d45 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3854,6 +3854,12 @@ struct remote_domain_start_dirty_rate_calc_args { unsigned int flags; }; +struct remote_domain_set_fibre_channel_appid_args { + remote_nonnull_domain dom; + remote_string appid; + unsigned int flags; +}; + /*----- Protocol. -----*/ @@ -6818,5 +6824,11 @@ enum remote_procedure { * @acl: network:write * @acl: network:save */ - REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432 + REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432, + + /** + * @generate: both + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_SET_FIBRE_CHANNEL_APPID = 433 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index dad83361fa..731cad112c 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3206,6 +3206,11 @@ struct remote_domain_start_dirty_rate_calc_args { int seconds; u_int flags; }; +struct remote_domain_set_fibre_channel_appid_args { + remote_nonnull_domain dom; + remote_string appid; + u_int flags; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3639,4 +3644,5 @@ enum remote_procedure { REMOTE_PROC_NODE_DEVICE_CREATE = 430, REMOTE_PROC_NWFILTER_DEFINE_XML_FLAGS = 431, REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432, + REMOTE_PROC_DOMAIN_SET_FIBRE_CHANNEL_APPID = 433, }; -- 2.31.1
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index df1b126b0c..0a64504d45 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3854,6 +3854,12 @@ struct remote_domain_start_dirty_rate_calc_args { unsigned int flags; };
+struct remote_domain_set_fibre_channel_appid_args { + remote_nonnull_domain dom; + remote_string appid; + unsigned int flags; +}; +
/*----- Protocol. -----*/
@@ -6818,5 +6824,11 @@ enum remote_procedure { * @acl: network:write * @acl: network:save */ - REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432 + REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432, + + /** + * @generate: both + * @acl: domain:write
Since this is not talking to QEMU on monitor it can't block really and thus it's safe to be marked with @priority: high.
+ */ + REMOTE_PROC_DOMAIN_SET_FIBRE_CHANNEL_APPID = 433 };
Michal
On Fri, Sep 10, 2021 at 01:49:27PM +0200, Michal Prívozník wrote:
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index df1b126b0c..0a64504d45 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3854,6 +3854,12 @@ struct remote_domain_start_dirty_rate_calc_args { unsigned int flags; };
+struct remote_domain_set_fibre_channel_appid_args { + remote_nonnull_domain dom; + remote_string appid; + unsigned int flags; +}; +
/*----- Protocol. -----*/
@@ -6818,5 +6824,11 @@ enum remote_procedure { * @acl: network:write * @acl: network:save */ - REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432 + REMOTE_PROC_NETWORK_DEFINE_XML_FLAGS = 432, + + /** + * @generate: both + * @acl: domain:write
Since this is not talking to QEMU on monitor it can't block really and thus it's safe to be marked with @priority: high.
Right, I missed that in the protocol documentation. Will fix. Thanks, Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bd41ddbc3c..872dedeafe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20428,6 +20428,80 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom, } +static void +qemuDomainUpdateFibreChannelAppid(virDomainDef *def, + const char *appid) +{ + if (!def->resource) { + def->resource = g_new0(virDomainResourceDef, 1); + } else { + g_free(def->resource->appid); + } + + def->resource->appid = g_strdup(appid); +} + + +static int +qemuDomainSetFibreChannelAppid(virDomainPtr dom, + const char *appid, + unsigned int flags) +{ + virQEMUDriver *driver = dom->conn->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainObj *vm = NULL; + virDomainDef *def = NULL; + virDomainDef *persistentDef = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (appid && virDomainDefResourceAppidValidate(appid) < 0) + return -1; + + if (!(vm = qemuDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainSetFibreChannelAppidEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob; + + if (def) { + qemuDomainObjPrivate *priv = vm->privateData; + + if (virCgroupSetFCAppid(priv->cgroup, appid) < 0) + goto endjob; + + qemuDomainUpdateFibreChannelAppid(def, appid); + + if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) + goto endjob; + } + + if (persistentDef) { + qemuDomainUpdateFibreChannelAppid(persistentDef, appid); + + if (virDomainDefSave(persistentDef, driver->xmlopt, cfg->configDir) < 0) + goto endjob; + } + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -20671,6 +20745,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainSetFibreChannelAppid = qemuDomainSetFibreChannelAppid, /* 7.7.0 */ }; -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/manpages/virsh.rst | 14 +++++++++ tools/virsh-domain.c | 65 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9561b3f59d..36fc94808d 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4515,6 +4515,20 @@ Output the IP address and port number for the VNC display. If the information is not available the processes will provide an exit code of 1. +setappid +-------- + +**Syntax:** + +:: + + setappid domain appid + +Set the Fibre Channel appid string for domain that is used to create VMID to +tag the traffic. Accepts only printable characters and maximal length is 128 +characters. To remove the appid from VM don't pass any appid. + + DEVICE COMMANDS =============== diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e5bd1fdd75..07d9c7f954 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14070,6 +14070,65 @@ cmdDomDirtyRateCalc(vshControl *ctl, const vshCmd *cmd) } +/* + * "setappid" command + */ +static const vshCmdInfo info_setappid[] = { + {.name = "help", + .data = N_("Set domain Fibre Channel appid") + }, + {.name = "desc", + .data = N_("Set the Fibre Channel appid string for domain that is " + "used to create VMID to tag the traffic. Accepts only " + "printable characters and maximal length is 128 characters. " + "To remove the appid from VM don't pass any appid.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_setappid[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = "appid", + .type = VSH_OT_STRING, + .help = N_("User provided appid string"), + }, + {.name = NULL} +}; + +static bool +cmdSetAppid(vshControl *ctl, const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + const char *appid = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "appid", &appid) < 0) + return false; + + if (virDomainSetFibreChannelAppid(dom, (char *)appid, flags) < 0) + return false; + + return true; +} + + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -14715,5 +14774,11 @@ const vshCmdDef domManagementCmds[] = { .info = info_domdirtyrate_calc, .flags = 0 }, + {.name = "setappid", + .handler = cmdSetAppid, + .opts = opts_setappid, + .info = info_setappid, + .flags = 0 + }, {.name = NULL} }; -- 2.31.1
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/manpages/virsh.rst | 14 +++++++++ tools/virsh-domain.c | 65 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9561b3f59d..36fc94808d 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4515,6 +4515,20 @@ Output the IP address and port number for the VNC display. If the information is not available the processes will provide an exit code of 1.
+setappid +-------- + +**Syntax:** + +:: + + setappid domain appid + +Set the Fibre Channel appid string for domain that is used to create VMID to +tag the traffic. Accepts only printable characters and maximal length is 128 +characters. To remove the appid from VM don't pass any appid. + + DEVICE COMMANDS ===============
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e5bd1fdd75..07d9c7f954 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14070,6 +14070,65 @@ cmdDomDirtyRateCalc(vshControl *ctl, const vshCmd *cmd) }
+/* + * "setappid" command + */ +static const vshCmdInfo info_setappid[] = { + {.name = "help", + .data = N_("Set domain Fibre Channel appid") + }, + {.name = "desc", + .data = N_("Set the Fibre Channel appid string for domain that is " + "used to create VMID to tag the traffic. Accepts only " + "printable characters and maximal length is 128 characters. " + "To remove the appid from VM don't pass any appid.")
I wonder whether removal semantic with an extra flag, e.g. 'virsh setappid --clear' wouldn't be better. If we ever add new option to this function users will be forced to pass current APPID when using the new option so that the APPID doesn't get cleared. For instance: setappid --appid APPID --magic vs setappid --magic
+ }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_setappid[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = "appid", + .type = VSH_OT_STRING, + .help = N_("User provided appid string"), + }, + {.name = NULL} +}; + +static bool +cmdSetAppid(vshControl *ctl, const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + const char *appid = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "appid", &appid) < 0) + return false; + + if (virDomainSetFibreChannelAppid(dom, (char *)appid, flags) < 0)
Probably a leftover? This typecast shouldn't be needed.
+ return false; + + return true; +}
Michal
On Fri, Sep 10, 2021 at 01:49:18PM +0200, Michal Prívozník wrote:
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/manpages/virsh.rst | 14 +++++++++ tools/virsh-domain.c | 65 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9561b3f59d..36fc94808d 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4515,6 +4515,20 @@ Output the IP address and port number for the VNC display. If the information is not available the processes will provide an exit code of 1.
+setappid +-------- + +**Syntax:** + +:: + + setappid domain appid + +Set the Fibre Channel appid string for domain that is used to create VMID to +tag the traffic. Accepts only printable characters and maximal length is 128 +characters. To remove the appid from VM don't pass any appid. + + DEVICE COMMANDS ===============
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e5bd1fdd75..07d9c7f954 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14070,6 +14070,65 @@ cmdDomDirtyRateCalc(vshControl *ctl, const vshCmd *cmd) }
+/* + * "setappid" command + */ +static const vshCmdInfo info_setappid[] = { + {.name = "help", + .data = N_("Set domain Fibre Channel appid") + }, + {.name = "desc", + .data = N_("Set the Fibre Channel appid string for domain that is " + "used to create VMID to tag the traffic. Accepts only " + "printable characters and maximal length is 128 characters. " + "To remove the appid from VM don't pass any appid.")
I wonder whether removal semantic with an extra flag, e.g. 'virsh setappid --clear' wouldn't be better. If we ever add new option to this function users will be forced to pass current APPID when using the new option so that the APPID doesn't get cleared. For instance:
setappid --appid APPID --magic vs setappid --magic
Good point, thanks, using --clear will be probably better and easier for users. Pavel
+ }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_setappid[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = "appid", + .type = VSH_OT_STRING, + .help = N_("User provided appid string"), + }, + {.name = NULL} +}; + +static bool +cmdSetAppid(vshControl *ctl, const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + const char *appid = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "appid", &appid) < 0) + return false; + + if (virDomainSetFibreChannelAppid(dom, (char *)appid, flags) < 0)
Probably a leftover? This typecast shouldn't be needed.
+ return false; + + return true; +}
Michal
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Rebased on top of current master.
I'm posting this as an RFC mainly because I'm not sure how to model the new API. This patches introduce a new naive API that will change only the APPID and nothing else.
Currently there are no other known features related to Fibre Channel resources so this non-extendable API will be sufficient, however the appid lives in <resource> element in the XML where we currently have root cgroup partition. Even though changing the partition will not be supported and we don't know about anything else that could be placed here it doesn't mean it will not happen in the future. In that case we would have to add new API as well.
So I'm wondering if we should create a more generic API that would take typed parameters as arguments:
int virDomainSetResource(virDomainPtr domain, virTypedParameterPtr params, int nparams, unsigned int flags)
I think having the spcific API as you implemented is fine for now. We did not have much in <resource/> until now and I don't think we will in foreseeable future.
Any ideas?
Pavel Hrdina (7): conf: extract appid validation to virDomainDefResourceAppidValidate cgroup: extract setting fibre channel appid into virCgroupSetFCAppid virCgroupSetFCAppid: properly handle when appid is NULL src: introduce virDomainSetFibreChannelAppid API remote: add RPC support for the virDomainSetFibreChannelAppid API qemu: implement virDomainSetFibreChannelAppid API tools: introduce virsh setappid command
docs/manpages/virsh.rst | 14 ++++++ include/libvirt/libvirt-domain.h | 4 ++ src/conf/domain_validate.c | 42 ++++++++++-------- src/conf/domain_validate.h | 2 + src/driver-hypervisor.h | 6 +++ src/libvirt-domain.c | 44 +++++++++++++++++++ src/libvirt_private.syms | 2 + src/libvirt_public.syms | 1 + src/qemu/qemu_cgroup.c | 17 +------- src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++++- src/remote_protocol-structs | 6 +++ src/util/vircgroup.c | 24 ++++++++++ src/util/vircgroup.h | 3 ++ tools/virsh-domain.c | 65 +++++++++++++++++++++++++++ 16 files changed, 286 insertions(+), 34 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Thu, Sep 09, 2021 at 06:13:16PM +0200, Pavel Hrdina wrote:
Rebased on top of current master.
I'm posting this as an RFC mainly because I'm not sure how to model the new API. This patches introduce a new naive API that will change only the APPID and nothing else.
Currently there are no other known features related to Fibre Channel resources so this non-extendable API will be sufficient, however the appid lives in <resource> element in the XML where we currently have root cgroup partition. Even though changing the partition will not be supported and we don't know about anything else that could be placed here it doesn't mean it will not happen in the future. In that case we would have to add new API as well.
I'm curious why we need to even support updates for the APPID in the first place. I understood it to be a unique identifier for the VM from POV of the storage, and unique identifier are not something you would typically change on the fly. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Sep 10, 2021 at 12:57:30PM +0100, Daniel P. Berrangé wrote:
On Thu, Sep 09, 2021 at 06:13:16PM +0200, Pavel Hrdina wrote:
Rebased on top of current master.
I'm posting this as an RFC mainly because I'm not sure how to model the new API. This patches introduce a new naive API that will change only the APPID and nothing else.
Currently there are no other known features related to Fibre Channel resources so this non-extendable API will be sufficient, however the appid lives in <resource> element in the XML where we currently have root cgroup partition. Even though changing the partition will not be supported and we don't know about anything else that could be placed here it doesn't mean it will not happen in the future. In that case we would have to add new API as well.
I'm curious why we need to even support updates for the APPID in the first place. I understood it to be a unique identifier for the VM from POV of the storage, and unique identifier are not something you would typically change on the fly.
Good point, when implementing this feature I also thought that there is no need to have live changes, but to be sure I asked in the BZ (no public) and got an answer that they would like to have the live changes as well. It's possible though that they just asked to have it without any planned usage for it. Pavel
On Fri, Sep 10, 2021 at 03:26:20PM +0200, Pavel Hrdina wrote:
On Fri, Sep 10, 2021 at 12:57:30PM +0100, Daniel P. Berrangé wrote:
On Thu, Sep 09, 2021 at 06:13:16PM +0200, Pavel Hrdina wrote:
Rebased on top of current master.
I'm posting this as an RFC mainly because I'm not sure how to model the new API. This patches introduce a new naive API that will change only the APPID and nothing else.
Currently there are no other known features related to Fibre Channel resources so this non-extendable API will be sufficient, however the appid lives in <resource> element in the XML where we currently have root cgroup partition. Even though changing the partition will not be supported and we don't know about anything else that could be placed here it doesn't mean it will not happen in the future. In that case we would have to add new API as well.
I'm curious why we need to even support updates for the APPID in the first place. I understood it to be a unique identifier for the VM from POV of the storage, and unique identifier are not something you would typically change on the fly.
Good point, when implementing this feature I also thought that there is no need to have live changes, but to be sure I asked in the BZ (no public) and got an answer that they would like to have the live changes as well.
Reading the bug report I'm not convinced they are asking for live changes. They are asking how to set the value using virsh, but that doesn't imply live change. It is entirely possible, maybe even likely, they just want to use 'virsh edit' to set the value in an existing guest before booting it. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Sep 10, 2021 at 02:37:49PM +0100, Daniel P. Berrangé wrote:
On Fri, Sep 10, 2021 at 03:26:20PM +0200, Pavel Hrdina wrote:
On Fri, Sep 10, 2021 at 12:57:30PM +0100, Daniel P. Berrangé wrote:
On Thu, Sep 09, 2021 at 06:13:16PM +0200, Pavel Hrdina wrote:
Rebased on top of current master.
I'm posting this as an RFC mainly because I'm not sure how to model the new API. This patches introduce a new naive API that will change only the APPID and nothing else.
Currently there are no other known features related to Fibre Channel resources so this non-extendable API will be sufficient, however the appid lives in <resource> element in the XML where we currently have root cgroup partition. Even though changing the partition will not be supported and we don't know about anything else that could be placed here it doesn't mean it will not happen in the future. In that case we would have to add new API as well.
I'm curious why we need to even support updates for the APPID in the first place. I understood it to be a unique identifier for the VM from POV of the storage, and unique identifier are not something you would typically change on the fly.
Good point, when implementing this feature I also thought that there is no need to have live changes, but to be sure I asked in the BZ (no public) and got an answer that they would like to have the live changes as well.
Reading the bug report I'm not convinced they are asking for live changes. They are asking how to set the value using virsh, but that doesn't imply live change. It is entirely possible, maybe even likely, they just want to use 'virsh edit' to set the value in an existing guest before booting it.
So we had a meeting today and yes there was some noise in the communication. There is no need to change the VMID while the VM is running so this whole patch series can be dropped. They wanted to have a virsh command to edit it but I explained that it can be already done by virsh edit and that we will add support to virt-install/virt-xml as well to make it easier. Thanks Michal for the review but I'm not going to push this series. Pavel
participants (3)
-
Daniel P. Berrangé -
Michal Prívozník -
Pavel Hrdina