6:13 p.m.
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
6:13 p.m.
New subject: [RFC REPOST 1/7] conf: extract appid validation to virDomainDefResourceAppidValidate
This will be needed by future patches adding appid API to allow changing
it for running VMs.
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:49 p.m.
New subject: [RFC REPOST 1/7] conf: extract appid validation to virDomainDefResourceAppidValidate
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(a)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
3:21 p.m.
New subject: [RFC REPOST 1/7] conf: extract appid validation to virDomainDefResourceAppidValidate
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(a)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
6:13 p.m.
New subject: [RFC REPOST 2/7] cgroup: extract setting fibre channel appid into virCgroupSetFCAppid
Signed-off-by: Pavel Hrdina <phrdina(a)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
6:13 p.m.
New subject: [RFC REPOST 3/7] virCgroupSetFCAppid: properly handle when appid is NULL
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(a)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
1:49 p.m.
New subject: [RFC REPOST 3/7] virCgroupSetFCAppid: properly handle when appid is NULL
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(a)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
3:16 p.m.
New subject: [RFC REPOST 3/7] virCgroupSetFCAppid: properly handle when appid is NULL
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(a)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
6:13 p.m.
New subject: [RFC REPOST 4/7] src: introduce virDomainSetFibreChannelAppid API
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:49 p.m.
New subject: [RFC REPOST 4/7] src: introduce virDomainSetFibreChannelAppid API
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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
3:15 p.m.
New subject: [RFC REPOST 4/7] src: introduce virDomainSetFibreChannelAppid API
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(a)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
6:13 p.m.
New subject: [RFC REPOST 5/7] remote: add RPC support for the virDomainSetFibreChannelAppid API
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:49 p.m.
New subject: [RFC REPOST 5/7] remote: add RPC support for the virDomainSetFibreChannelAppid API
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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
3:23 p.m.
New subject: [RFC REPOST 5/7] remote: add RPC support for the virDomainSetFibreChannelAppid API
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(a)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
6:13 p.m.
New subject: [RFC REPOST 6/7] qemu: implement virDomainSetFibreChannelAppid API
Signed-off-by: Pavel Hrdina <phrdina(a)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
6:13 p.m.
New subject: [RFC REPOST 7/7] tools: introduce virsh setappid command
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:49 p.m.
New subject: [RFC REPOST 7/7] tools: introduce virsh setappid command
On 9/9/21 6:13 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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
3:13 p.m.
New subject: [RFC REPOST 7/7] tools: introduce virsh setappid command
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(a)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
1:49 p.m.
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(a)redhat.com>
Michal
1:57 p.m.
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 :|
3:26 p.m.
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
3:37 p.m.
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 :|
5:54 p.m.
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
1099
days inactive
1103
days old
22 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Michal Prívozník -
Pavel Hrdina