9:29 a.m.
Pavel Hrdina (3):
vircgroup: introduce virCgroupGetInode function
conf: introduce support for Fibre Channel VMID
qemu: implement support for Fibre Channel VMID
docs/formatdomain.rst | 13 ++++++++++
docs/schemas/domaincommon.rng | 11 +++++++++
src/conf/domain_conf.c | 5 ++++
src/conf/domain_conf.h | 2 ++
src/conf/domain_validate.c | 19 +++++++++++++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 32 +++++++++++++++++++++++++
src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++
src/util/vircgroup.h | 2 ++
tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++
tests/genericxml2xmltest.c | 2 ++
11 files changed, 143 insertions(+)
create mode 100644 tests/genericxml2xmlindata/vmid.xml
--
2.31.1
9:29 a.m.
New subject: [libvirt PATCH 1/3] vircgroup: introduce virCgroupGetInode function
For new feature Fibre Channel VMID we will need to get inode of the
VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++
src/util/vircgroup.h | 2 ++
3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6961cdb137..d8451fcfff 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1920,6 +1920,7 @@ virCgroupGetCpuShares;
virCgroupGetDevicePermsString;
virCgroupGetDomainTotalCpuStats;
virCgroupGetFreezerState;
+virCgroupGetInode;
virCgroupGetMemoryHardLimit;
virCgroupGetMemorySoftLimit;
virCgroupGetMemoryStat;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 1b3b28342e..ea702e7b63 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long
*period,
return 0;
}
+
+
+/**
+ * virCgroupGetInode:
+ *
+ * @cgroup: the cgroup to get inode for
+ *
+ * Get the @cgroup inode and return its value to the caller.
+ *
+ * Returns inode on success, -1 on error with error message reported.
+ */
+int
+virCgroupGetInode(virCgroup *cgroup)
+{
+ VIR_AUTOCLOSE fd = 0;
+ struct stat st;
+ int controller = virCgroupGetAnyController(cgroup);
+ g_autofree char *path = NULL;
+
+ if (controller < 0)
+ return -1;
+
+ if (virCgroupPathOfController(cgroup, controller, "", &path) < 0)
+ return -1;
+
+ if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) {
+ virReportSystemError(errno, _("failed to open cgroup '%s'"),
path);
+ return -1;
+ }
+
+ if (fstat(fd, &st) < 0) {
+ virReportSystemError(errno, _("failed to get fstat for '%s'"),
path);
+ return -1;
+ }
+
+ return st.st_ino;
+}
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 8002cac570..690f09465c 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -283,3 +283,5 @@ int virCgroupSetOwner(virCgroup *cgroup,
int virCgroupHasEmptyTasks(virCgroup *cgroup, int controller);
bool virCgroupControllerAvailable(int controller);
+
+int virCgroupGetInode(virCgroup *cgroup);
--
2.31.1
3:06 a.m.
New subject: [libvirt PATCH 1/3] vircgroup: introduce virCgroupGetInode function
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
For new feature Fibre Channel VMID we will need to get inode of the
VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++
src/util/vircgroup.h | 2 ++
3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6961cdb137..d8451fcfff 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1920,6 +1920,7 @@ virCgroupGetCpuShares;
virCgroupGetDevicePermsString;
virCgroupGetDomainTotalCpuStats;
virCgroupGetFreezerState;
+virCgroupGetInode;
virCgroupGetMemoryHardLimit;
virCgroupGetMemorySoftLimit;
virCgroupGetMemoryStat;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 1b3b28342e..ea702e7b63 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long
*period,
return 0;
}
+
+
+/**
+ * virCgroupGetInode:
+ *
+ * @cgroup: the cgroup to get inode for
+ *
+ * Get the @cgroup inode and return its value to the caller.
+ *
+ * Returns inode on success, -1 on error with error message reported.
+ */
+int
+virCgroupGetInode(virCgroup *cgroup)
+{
+ VIR_AUTOCLOSE fd = 0;
+ struct stat st;
+ int controller = virCgroupGetAnyController(cgroup);
+ g_autofree char *path = NULL;
+
+ if (controller < 0)
+ return -1;
+
+ if (virCgroupPathOfController(cgroup, controller, "", &path) < 0)
+ return -1;
+
+ if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) {
+ virReportSystemError(errno, _("failed to open cgroup '%s'"),
path);
+ return -1;
+ }
Is the open() necessary? Why isn't plain stat() enough?
Michal
3:22 a.m.
New subject: [libvirt PATCH 1/3] vircgroup: introduce virCgroupGetInode function
On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote:
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
> For new feature Fibre Channel VMID we will need to get inode of the
> VM root cgroup as it is used in the new kernel API together with VMID.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++
> src/util/vircgroup.h | 2 ++
> 3 files changed, 40 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6961cdb137..d8451fcfff 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares;
> virCgroupGetDevicePermsString;
> virCgroupGetDomainTotalCpuStats;
> virCgroupGetFreezerState;
> +virCgroupGetInode;
> virCgroupGetMemoryHardLimit;
> virCgroupGetMemorySoftLimit;
> virCgroupGetMemoryStat;
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 1b3b28342e..ea702e7b63 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long
long *period,
>
> return 0;
> }
> +
> +
> +/**
> + * virCgroupGetInode:
> + *
> + * @cgroup: the cgroup to get inode for
> + *
> + * Get the @cgroup inode and return its value to the caller.
> + *
> + * Returns inode on success, -1 on error with error message reported.
> + */
> +int
> +virCgroupGetInode(virCgroup *cgroup)
> +{
> + VIR_AUTOCLOSE fd = 0;
> + struct stat st;
> + int controller = virCgroupGetAnyController(cgroup);
> + g_autofree char *path = NULL;
> +
> + if (controller < 0)
> + return -1;
> +
> + if (virCgroupPathOfController(cgroup, controller, "", &path) <
0)
> + return -1;
> +
> + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) {
> + virReportSystemError(errno, _("failed to open cgroup
'%s'"), path);
> + return -1;
> + }
Is the open() necessary? Why isn't plain stat() enough?
Good question :) I was lazy so I looked for first similar code in
libvirt codebase and copied it. You are right, stat should be enough
as according to man page stat and fstat are identical except or the
fd vs path parameter. I'll fix it before pushing or should I send a v2?
Pavel
3:26 a.m.
New subject: [libvirt PATCH 1/3] vircgroup: introduce virCgroupGetInode function
On 8/4/21 10:22 AM, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote:
> On 8/3/21 4:29 PM, Pavel Hrdina wrote:
>> For new feature Fibre Channel VMID we will need to get inode of the
>> VM root cgroup as it is used in the new kernel API together with VMID.
>>
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++
>> src/util/vircgroup.h | 2 ++
>> 3 files changed, 40 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 6961cdb137..d8451fcfff 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares;
>> virCgroupGetDevicePermsString;
>> virCgroupGetDomainTotalCpuStats;
>> virCgroupGetFreezerState;
>> +virCgroupGetInode;
>> virCgroupGetMemoryHardLimit;
>> virCgroupGetMemorySoftLimit;
>> virCgroupGetMemoryStat;
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>> index 1b3b28342e..ea702e7b63 100644
>> --- a/src/util/vircgroup.c
>> +++ b/src/util/vircgroup.c
>> @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned
long long *period,
>>
>> return 0;
>> }
>> +
>> +
>> +/**
>> + * virCgroupGetInode:
>> + *
>> + * @cgroup: the cgroup to get inode for
>> + *
>> + * Get the @cgroup inode and return its value to the caller.
>> + *
>> + * Returns inode on success, -1 on error with error message reported.
>> + */
>> +int
>> +virCgroupGetInode(virCgroup *cgroup)
>> +{
>> + VIR_AUTOCLOSE fd = 0;
>> + struct stat st;
>> + int controller = virCgroupGetAnyController(cgroup);
>> + g_autofree char *path = NULL;
>> +
>> + if (controller < 0)
>> + return -1;
>> +
>> + if (virCgroupPathOfController(cgroup, controller, "", &path)
< 0)
>> + return -1;
>> +
>> + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) {
>> + virReportSystemError(errno, _("failed to open cgroup
'%s'"), path);
>> + return -1;
>> + }
>
> Is the open() necessary? Why isn't plain stat() enough?
Good question :) I was lazy so I looked for first similar code in
libvirt codebase and copied it. You are right, stat should be enough
as according to man page stat and fstat are identical except or the
fd vs path parameter. I'll fix it before pushing or should I send a v2?
No need, just drop open() and switch to stat() before pushing. My
Reviewed-by still applies.
Michal
3:32 a.m.
New subject: [libvirt PATCH 1/3] vircgroup: introduce virCgroupGetInode function
On Wed, Aug 04, 2021 at 10:26:41AM +0200, Michal Prívozník wrote:
On 8/4/21 10:22 AM, Pavel Hrdina wrote:
> On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote:
>> On 8/3/21 4:29 PM, Pavel Hrdina wrote:
>>> For new feature Fibre Channel VMID we will need to get inode of the
>>> VM root cgroup as it is used in the new kernel API together with VMID.
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>> ---
>>> src/libvirt_private.syms | 1 +
>>> src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++
>>> src/util/vircgroup.h | 2 ++
>>> 3 files changed, 40 insertions(+)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 6961cdb137..d8451fcfff 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares;
>>> virCgroupGetDevicePermsString;
>>> virCgroupGetDomainTotalCpuStats;
>>> virCgroupGetFreezerState;
>>> +virCgroupGetInode;
>>> virCgroupGetMemoryHardLimit;
>>> virCgroupGetMemorySoftLimit;
>>> virCgroupGetMemoryStat;
>>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>>> index 1b3b28342e..ea702e7b63 100644
>>> --- a/src/util/vircgroup.c
>>> +++ b/src/util/vircgroup.c
>>> @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup,
unsigned long long *period,
>>>
>>> return 0;
>>> }
>>> +
>>> +
>>> +/**
>>> + * virCgroupGetInode:
>>> + *
>>> + * @cgroup: the cgroup to get inode for
>>> + *
>>> + * Get the @cgroup inode and return its value to the caller.
>>> + *
>>> + * Returns inode on success, -1 on error with error message reported.
>>> + */
>>> +int
>>> +virCgroupGetInode(virCgroup *cgroup)
>>> +{
>>> + VIR_AUTOCLOSE fd = 0;
>>> + struct stat st;
>>> + int controller = virCgroupGetAnyController(cgroup);
>>> + g_autofree char *path = NULL;
>>> +
>>> + if (controller < 0)
>>> + return -1;
>>> +
>>> + if (virCgroupPathOfController(cgroup, controller, "",
&path) < 0)
>>> + return -1;
>>> +
>>> + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) {
>>> + virReportSystemError(errno, _("failed to open cgroup
'%s'"), path);
>>> + return -1;
>>> + }
>>
>> Is the open() necessary? Why isn't plain stat() enough?
>
> Good question :) I was lazy so I looked for first similar code in
> libvirt codebase and copied it. You are right, stat should be enough
> as according to man page stat and fstat are identical except or the
> fd vs path parameter. I'll fix it before pushing or should I send a v2?
No need, just drop open() and switch to stat() before pushing. My
Reviewed-by still applies.
OK, will do. Thanks for review!
Pavel
9:29 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
docs/formatdomain.rst | 13 +++++++++++++
docs/schemas/domaincommon.rng | 11 +++++++++++
src/conf/domain_conf.c | 5 +++++
src/conf/domain_conf.h | 2 ++
src/conf/domain_validate.c | 19 +++++++++++++++++++
tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
tests/genericxml2xmltest.c | 2 ++
7 files changed, 71 insertions(+)
create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 61ccd8895a..f6a1885acf 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2232,6 +2232,19 @@ event name Description
``emulation_faults`` the count of emulation faults, that is when the kernel traps
on unimplemented instrucions and emulates them for user space, by applications running on
the platform ``perf.emulation_faults``
===========================
=======================================================================================================================================================================================
================================
+Fibre Channel VMID
+-------------------
+
+The FC SAN can provide various QOS levels, access control depending on the
+VMID. Also it can collect telemetry data at per-VM level which can be used
+to enhance the IO performance of the VM. This can be configured by using
+the ``vmid`` element. The element contains single string (max 128 bytes).
+
+Using this feature requires Fibre Channel capable HW, kernel compiled with
+option ``CONFIG_BLK_CGROUP_FC_APPID`` and ``nvme_fc`` kernel module loaded.
+:since:`Since 7.7.0`
+
+
:anchor:`<a id="elementsDevices"/>`
Devices
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2442078969..d2e6039c76 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -62,6 +62,9 @@
<optional>
<ref name="perf"/>
</optional>
+ <optional>
+ <ref name="vmid"/>
+ </optional>
<optional>
<ref name="idmap"/>
</optional>
@@ -576,6 +579,14 @@
</element>
</define>
+ <define name="vmid">
+ <element name="vmid">
+ <data type="string">
+ <param name="pattern">.{1,128}</param>
+ </data>
+ </element>
+ </define>
+
<!--
The Identifiers can be:
- an optional id attribute with a number on the domain element
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 09da4ab952..426ea03049 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19646,6 +19646,8 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virDomainDefParseBootOptions(def, ctxt) < 0)
goto error;
+ def->vmid = virXPathString("string(./vmid[1])", ctxt);
+
/* analysis of the disk devices */
if ((n = virXPathNodeSet("./devices/disk", ctxt, &nodes)) < 0)
goto error;
@@ -28161,6 +28163,9 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
virBufferAddLit(buf, "</clock>\n");
}
+ if (def->vmid)
+ virBufferAsprintf(buf, "<vmid>%s</vmid>\n", def->vmid);
+
if (virDomainEventActionDefFormat(buf, def->onPoweroff,
"on_poweroff",
virDomainLifecycleActionTypeToString) < 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9f32bcf9cf..4e5e2259c0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2803,6 +2803,8 @@ struct _virDomainDef {
virDomainClockDef clock;
+ char *vmid;
+
size_t ngraphics;
virDomainGraphicsDef **graphics;
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index a9e4519b1a..9d599e4bd6 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -55,6 +55,22 @@ virDomainDefBootValidate(const virDomainDef *def)
}
+static int
+virDomainDefVMIDValidate(const virDomainDef *def)
+{
+ if (!def->vmid)
+ return 0;
+
+ if (strlen(def->vmid) > 128) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("VMID cannot be longer then 128 characters"));
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
virDomainDefVideoValidate(const virDomainDef *def)
{
@@ -1538,6 +1554,9 @@ static int
virDomainDefValidateInternal(const virDomainDef *def,
virDomainXMLOption *xmlopt)
{
+ if (virDomainDefVMIDValidate(def) < 0)
+ return -1;
+
if (virDomainDefDuplicateDiskInfoValidate(def) < 0)
return -1;
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml
new file mode 100644
index 0000000000..df984df3f5
--- /dev/null
+++ b/tests/genericxml2xmlindata/vmid.xml
@@ -0,0 +1,19 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>4</vcpu>
+ <os>
+ <type arch='x86_64' machine='q35'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index ef51ed91a6..e77f4ff386 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -225,6 +225,8 @@ mymain(void)
DO_TEST_DIFFERENT("cputune");
DO_TEST("device-backenddomain");
+ DO_TEST("vmid");
+
#define DO_TEST_BACKUP_FULL(name, intrnl) \
do { \
const struct testCompareBackupXMLData data = { .testname = name, \
--
2.31.1
4:09 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
docs/formatdomain.rst | 13 +++++++++++++
docs/schemas/domaincommon.rng | 11 +++++++++++
src/conf/domain_conf.c | 5 +++++
src/conf/domain_conf.h | 2 ++
src/conf/domain_validate.c | 19 +++++++++++++++++++
tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
tests/genericxml2xmltest.c | 2 ++
7 files changed, 71 insertions(+)
create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml
b/tests/genericxml2xmlindata/vmid.xml
new file mode 100644
index 0000000000..df984df3f5
--- /dev/null
+++ b/tests/genericxml2xmlindata/vmid.xml
@@ -0,0 +1,19 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>4</vcpu>
+ <os>
+ <type arch='x86_64' machine='q35'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion
as to what it is used for.
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 :|
4:19 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> docs/formatdomain.rst | 13 +++++++++++++
> docs/schemas/domaincommon.rng | 11 +++++++++++
> src/conf/domain_conf.c | 5 +++++
> src/conf/domain_conf.h | 2 ++
> src/conf/domain_validate.c | 19 +++++++++++++++++++
> tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
> tests/genericxml2xmltest.c | 2 ++
> 7 files changed, 71 insertions(+)
> create mode 100644 tests/genericxml2xmlindata/vmid.xml
> diff --git a/tests/genericxml2xmlindata/vmid.xml
b/tests/genericxml2xmlindata/vmid.xml
> new file mode 100644
> index 0000000000..df984df3f5
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/vmid.xml
> @@ -0,0 +1,19 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>4</vcpu>
> + <os>
> + <type arch='x86_64' machine='q35'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion
as to what it is used for.
I was struggling with that as well. To be honest I don't know where to
put it. In downstream BZ there was suggestion to place it under
<blkiotune> but this is not tunning any IO so I don't thing it should
belong there. We can move it closer to UUID and GENID and all this ID
elements. Any idea is welcome :).
Pavel
4:32 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> > ---
> > docs/formatdomain.rst | 13 +++++++++++++
> > docs/schemas/domaincommon.rng | 11 +++++++++++
> > src/conf/domain_conf.c | 5 +++++
> > src/conf/domain_conf.h | 2 ++
> > src/conf/domain_validate.c | 19 +++++++++++++++++++
> > tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
> > tests/genericxml2xmltest.c | 2 ++
> > 7 files changed, 71 insertions(+)
> > create mode 100644 tests/genericxml2xmlindata/vmid.xml
>
> > diff --git a/tests/genericxml2xmlindata/vmid.xml
b/tests/genericxml2xmlindata/vmid.xml
> > new file mode 100644
> > index 0000000000..df984df3f5
> > --- /dev/null
> > +++ b/tests/genericxml2xmlindata/vmid.xml
> > @@ -0,0 +1,19 @@
> > +<domain type='qemu'>
> > + <name>QEMUGuest1</name>
> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > + <memory unit='KiB'>219100</memory>
> > + <currentMemory unit='KiB'>219100</currentMemory>
> > + <vcpu placement='static'>4</vcpu>
> > + <os>
> > + <type arch='x86_64' machine='q35'>hvm</type>
> > + <boot dev='hd'/>
> > + </os>
> > + <clock offset='utc'/>
> > + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
>
> I'm not convinced by adding this element here with this name.
>
> It is rather too generic and doesn't really give any suggestion
> as to what it is used for.
I was struggling with that as well. To be honest I don't know where to
put it. In downstream BZ there was suggestion to place it under
<blkiotune> but this is not tunning any IO so I don't thing it should
belong there. We can move it closer to UUID and GENID and all this ID
elements. Any idea is welcome :).
I'm struggling to find info on what this attribute actally does in
practice, and/or what format it is required to be in.
I was wondering what would happen if we simply wrote the VM UUID
into this attribute unconditionally, and thus avoid need for an
XML element ?
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 :|
4:54 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
On Wed, Aug 04, 2021 at 10:32:00AM +0100, Daniel P. Berrangé wrote:
On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
> On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
> > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> > > ---
> > > docs/formatdomain.rst | 13 +++++++++++++
> > > docs/schemas/domaincommon.rng | 11 +++++++++++
> > > src/conf/domain_conf.c | 5 +++++
> > > src/conf/domain_conf.h | 2 ++
> > > src/conf/domain_validate.c | 19 +++++++++++++++++++
> > > tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
> > > tests/genericxml2xmltest.c | 2 ++
> > > 7 files changed, 71 insertions(+)
> > > create mode 100644 tests/genericxml2xmlindata/vmid.xml
> >
> > > diff --git a/tests/genericxml2xmlindata/vmid.xml
b/tests/genericxml2xmlindata/vmid.xml
> > > new file mode 100644
> > > index 0000000000..df984df3f5
> > > --- /dev/null
> > > +++ b/tests/genericxml2xmlindata/vmid.xml
> > > @@ -0,0 +1,19 @@
> > > +<domain type='qemu'>
> > > + <name>QEMUGuest1</name>
> > > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > > + <memory unit='KiB'>219100</memory>
> > > + <currentMemory unit='KiB'>219100</currentMemory>
> > > + <vcpu placement='static'>4</vcpu>
> > > + <os>
> > > + <type arch='x86_64'
machine='q35'>hvm</type>
> > > + <boot dev='hd'/>
> > > + </os>
> > > + <clock offset='utc'/>
> > > + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
> >
> > I'm not convinced by adding this element here with this name.
> >
> > It is rather too generic and doesn't really give any suggestion
> > as to what it is used for.
>
> I was struggling with that as well. To be honest I don't know where to
> put it. In downstream BZ there was suggestion to place it under
> <blkiotune> but this is not tunning any IO so I don't thing it should
> belong there. We can move it closer to UUID and GENID and all this ID
> elements. Any idea is welcome :).
I'm struggling to find info on what this attribute actally does in
practice, and/or what format it is required to be in.
I was wondering what would happen if we simply wrote the VM UUID
into this attribute unconditionally, and thus avoid need for an
XML element ?
Right, should have provided some links probably, always forget to do
that. We need to have it in XML as users will provide their own VMID
and it can be anything, ti doesn't have to be VM UUID but it can be
part of it. I was not able to find any specification, only some mentions
from HW vendors:
[1] "Virtual-machine awareness: The MDS 9148T provides visibility into
all virtual machines that are accessing the storage LUNs in the fabric.
This feature is available through HBAs capable of priority tagging the
Virtual Machine Identifier (VMID) on every Fibre Channel frame.
Virtual-machine awareness can be extended to intelligent fabric services
such as analytics1 to visualize performance of every flow originating
from each virtual machine in the fabric."
[2] "Provides visibility into each virtual machine logged into the
fabric through HBAs capable of priority tagging the Virtual Machine
Identifier (VMID) on every FC frame."
Fibre Channel VMID is can be included in every FC frame which allows
monitoring or QoS and other things based on the VMID provided by users.
The VMID can contain anything so a group of VMs can share the same VMID,
for example "database". Or to allow per application and VM tracking
another example is "database:$UUID".
Originally before I started working on this feature I thought that the
VM UUID will be added automatically to the user provided string but I
realized that assumption was incorrect as the user doesn't necessarily
want to or need to have the UUID as part of the FC VMID.
I was just straggling how to model this in the XML.
Pavel
[1]
<https://www.cisco.com/c/en/us/products/collateral/storage-networking/mds-...
[2]
<https://buy.hpe.com/za/en/storage/storage-networking/c-series-switches/c-...
9:32 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
On Wed, Aug 04, 2021 at 11:54:07AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:32:00AM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
> > On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> > > > ---
> > > > docs/formatdomain.rst | 13 +++++++++++++
> > > > docs/schemas/domaincommon.rng | 11 +++++++++++
> > > > src/conf/domain_conf.c | 5 +++++
> > > > src/conf/domain_conf.h | 2 ++
> > > > src/conf/domain_validate.c | 19 +++++++++++++++++++
> > > > tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
> > > > tests/genericxml2xmltest.c | 2 ++
> > > > 7 files changed, 71 insertions(+)
> > > > create mode 100644 tests/genericxml2xmlindata/vmid.xml
> > >
> > > > diff --git a/tests/genericxml2xmlindata/vmid.xml
b/tests/genericxml2xmlindata/vmid.xml
> > > > new file mode 100644
> > > > index 0000000000..df984df3f5
> > > > --- /dev/null
> > > > +++ b/tests/genericxml2xmlindata/vmid.xml
> > > > @@ -0,0 +1,19 @@
> > > > +<domain type='qemu'>
> > > > + <name>QEMUGuest1</name>
> > > > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > > > + <memory unit='KiB'>219100</memory>
> > > > + <currentMemory
unit='KiB'>219100</currentMemory>
> > > > + <vcpu placement='static'>4</vcpu>
> > > > + <os>
> > > > + <type arch='x86_64'
machine='q35'>hvm</type>
> > > > + <boot dev='hd'/>
> > > > + </os>
> > > > + <clock offset='utc'/>
> > > > +
<vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
> > >
> > > I'm not convinced by adding this element here with this name.
> > >
> > > It is rather too generic and doesn't really give any suggestion
> > > as to what it is used for.
> >
> > I was struggling with that as well. To be honest I don't know where to
> > put it. In downstream BZ there was suggestion to place it under
> > <blkiotune> but this is not tunning any IO so I don't thing it
should
> > belong there. We can move it closer to UUID and GENID and all this ID
> > elements. Any idea is welcome :).
>
> I'm struggling to find info on what this attribute actally does in
> practice, and/or what format it is required to be in.
>
> I was wondering what would happen if we simply wrote the VM UUID
> into this attribute unconditionally, and thus avoid need for an
> XML element ?
Right, should have provided some links probably, always forget to do
that. We need to have it in XML as users will provide their own VMID
and it can be anything, ti doesn't have to be VM UUID but it can be
part of it. I was not able to find any specification, only some mentions
from HW vendors:
[1] "Virtual-machine awareness: The MDS 9148T provides visibility into
all virtual machines that are accessing the storage LUNs in the fabric.
This feature is available through HBAs capable of priority tagging the
Virtual Machine Identifier (VMID) on every Fibre Channel frame.
Virtual-machine awareness can be extended to intelligent fabric services
such as analytics1 to visualize performance of every flow originating
from each virtual machine in the fabric."
[2] "Provides visibility into each virtual machine logged into the
fabric through HBAs capable of priority tagging the Virtual Machine
Identifier (VMID) on every FC frame."
IIUC, the implication is that the VMID associated with QEMU gets
associated with I/O issued from any <disk> devices that happen to
be backed by the fibre channel HBA consuming it.
Fibre Channel VMID is can be included in every FC frame which allows
monitoring or QoS and other things based on the VMID provided by users.
The VMID can contain anything so a group of VMs can share the same VMID,
for example "database". Or to allow per application and VM tracking
another example is "database:$UUID".
Ok, so the "group of VMs can share the VMID" eliminates the option
of unconditionally using the VM UUID.
I'm inclined to suggest that we put it inside the <resource> element,
since it is essentially a tunable for FC resource control.
eg something like
<resource>
<fibrechannel appid="...."/>
</resouce>
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 :|
10:05 a.m.
New subject: [libvirt PATCH 2/3] conf: introduce support for Fibre Channel VMID
On Wed, Aug 04, 2021 at 03:32:16PM +0100, Daniel P. Berrangé wrote:
On Wed, Aug 04, 2021 at 11:54:07AM +0200, Pavel Hrdina wrote:
> On Wed, Aug 04, 2021 at 10:32:00AM +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
> > > On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
> > > > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> > > > > ---
> > > > > docs/formatdomain.rst | 13 +++++++++++++
> > > > > docs/schemas/domaincommon.rng | 11 +++++++++++
> > > > > src/conf/domain_conf.c | 5 +++++
> > > > > src/conf/domain_conf.h | 2 ++
> > > > > src/conf/domain_validate.c | 19 +++++++++++++++++++
> > > > > tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++
> > > > > tests/genericxml2xmltest.c | 2 ++
> > > > > 7 files changed, 71 insertions(+)
> > > > > create mode 100644 tests/genericxml2xmlindata/vmid.xml
> > > >
> > > > > diff --git a/tests/genericxml2xmlindata/vmid.xml
b/tests/genericxml2xmlindata/vmid.xml
> > > > > new file mode 100644
> > > > > index 0000000000..df984df3f5
> > > > > --- /dev/null
> > > > > +++ b/tests/genericxml2xmlindata/vmid.xml
> > > > > @@ -0,0 +1,19 @@
> > > > > +<domain type='qemu'>
> > > > > + <name>QEMUGuest1</name>
> > > > > +
<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > > > > + <memory unit='KiB'>219100</memory>
> > > > > + <currentMemory
unit='KiB'>219100</currentMemory>
> > > > > + <vcpu placement='static'>4</vcpu>
> > > > > + <os>
> > > > > + <type arch='x86_64'
machine='q35'>hvm</type>
> > > > > + <boot dev='hd'/>
> > > > > + </os>
> > > > > + <clock offset='utc'/>
> > > > > +
<vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
> > > >
> > > > I'm not convinced by adding this element here with this name.
> > > >
> > > > It is rather too generic and doesn't really give any suggestion
> > > > as to what it is used for.
> > >
> > > I was struggling with that as well. To be honest I don't know where
to
> > > put it. In downstream BZ there was suggestion to place it under
> > > <blkiotune> but this is not tunning any IO so I don't thing it
should
> > > belong there. We can move it closer to UUID and GENID and all this ID
> > > elements. Any idea is welcome :).
> >
> > I'm struggling to find info on what this attribute actally does in
> > practice, and/or what format it is required to be in.
> >
> > I was wondering what would happen if we simply wrote the VM UUID
> > into this attribute unconditionally, and thus avoid need for an
> > XML element ?
>
> Right, should have provided some links probably, always forget to do
> that. We need to have it in XML as users will provide their own VMID
> and it can be anything, ti doesn't have to be VM UUID but it can be
> part of it. I was not able to find any specification, only some mentions
> from HW vendors:
>
> [1] "Virtual-machine awareness: The MDS 9148T provides visibility into
> all virtual machines that are accessing the storage LUNs in the fabric.
> This feature is available through HBAs capable of priority tagging the
> Virtual Machine Identifier (VMID) on every Fibre Channel frame.
> Virtual-machine awareness can be extended to intelligent fabric services
> such as analytics1 to visualize performance of every flow originating
> from each virtual machine in the fabric."
>
> [2] "Provides visibility into each virtual machine logged into the
> fabric through HBAs capable of priority tagging the Virtual Machine
> Identifier (VMID) on every FC frame."
IIUC, the implication is that the VMID associated with QEMU gets
associated with I/O issued from any <disk> devices that happen to
be backed by the fibre channel HBA consuming it.
> Fibre Channel VMID is can be included in every FC frame which allows
> monitoring or QoS and other things based on the VMID provided by users.
> The VMID can contain anything so a group of VMs can share the same VMID,
> for example "database". Or to allow per application and VM tracking
> another example is "database:$UUID".
Ok, so the "group of VMs can share the VMID" eliminates the option
of unconditionally using the VM UUID.
I'm inclined to suggest that we put it inside the <resource> element,
since it is essentially a tunable for FC resource control.
eg something like
<resource>
<fibrechannel appid="...."/>
</resouce>
Sounds reasonable as this is somehow related to cgroups and we already
have <partition> there. Thanks! I'll post v2 as I like this better as
well.
Pavel
9:29 a.m.
New subject: [libvirt PATCH 3/3] qemu: implement support for Fibre Channel VMID
Based on kernel commit messages the interface is
/sys/class/fc/fc_udev_device/appid_store
where we need to write the following string "$INODE:$VMID".
$INODE is the VM root cgroup inode in hexadecimal and $VMID is user
provided string that will be attached to each FC frame for the VM
within the cgroup identified by inode and has limit 128 bytes.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f2d99abcfa..df021b5985 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -904,6 +904,35 @@ qemuSetupCpuCgroup(virDomainObj *vm)
}
+static int
+qemuSetupCgroupVMID(virDomainObj *vm)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ int inode = virCgroupGetInode(priv->cgroup);
+ const char *path = "/sys/class/fc/fc_udev_device/appid_store";
+ g_autofree char *appid = NULL;
+
+ if (!vm->def->vmid)
+ return 0;
+
+ appid = g_strdup_printf("%X:%s", inode, vm->def->vmid);
+
+ if (virFileWriteStr(path, appid, 0) < 0) {
+ if (errno == ENOENT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("FC VMID not available on this host"));
+ return -1;
+ }
+
+ virReportSystemError(errno,
+ _("Unable to write '%s' to '%s'"),
appid, path);
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
qemuInitCgroup(virDomainObj *vm,
size_t nnicindexes,
@@ -1096,6 +1125,9 @@ qemuSetupCgroup(virDomainObj *vm,
if (qemuSetupCpusetCgroup(vm) < 0)
return -1;
+ if (qemuSetupCgroupVMID(vm) < 0)
+ return -1;
+
return 0;
}
--
2.31.1
3:06 a.m.
New subject: [libvirt PATCH 3/3] qemu: implement support for Fibre Channel VMID
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
Based on kernel commit messages the interface is
/sys/class/fc/fc_udev_device/appid_store
where we need to write the following string "$INODE:$VMID".
$INODE is the VM root cgroup inode in hexadecimal and $VMID is user
provided string that will be attached to each FC frame for the VM
within the cgroup identified by inode and has limit 128 bytes.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f2d99abcfa..df021b5985 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -904,6 +904,35 @@ qemuSetupCpuCgroup(virDomainObj *vm)
}
+static int
+qemuSetupCgroupVMID(virDomainObj *vm)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ int inode = virCgroupGetInode(priv->cgroup);
+ const char *path = "/sys/class/fc/fc_udev_device/appid_store";
+ g_autofree char *appid = NULL;
+
+ if (!vm->def->vmid)
+ return 0;
+
+ appid = g_strdup_printf("%X:%s", inode, vm->def->vmid);
+
+ if (virFileWriteStr(path, appid, 0) < 0) {
+ if (errno == ENOENT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("FC VMID not available on this host"));
+ return -1;
+ }
+
+ virReportSystemError(errno,
+ _("Unable to write '%s' to
'%s'"), appid, path);
Alternatively, use the following pattern:
if (errno == ENOENT) {
virReportError();
} else {
virReportSystemError();
}
return -1;
Or use just virReportSystemError(); I believe "Unable to write to
/sys/class/...: No such file or directory" is pretty comprehensive.
Michal
3:29 a.m.
New subject: [libvirt PATCH 3/3] qemu: implement support for Fibre Channel VMID
On Wed, Aug 04, 2021 at 10:06:21AM +0200, Michal Prívozník wrote:
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
> Based on kernel commit messages the interface is
>
> /sys/class/fc/fc_udev_device/appid_store
>
> where we need to write the following string "$INODE:$VMID".
>
> $INODE is the VM root cgroup inode in hexadecimal and $VMID is user
> provided string that will be attached to each FC frame for the VM
> within the cgroup identified by inode and has limit 128 bytes.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/qemu/qemu_cgroup.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index f2d99abcfa..df021b5985 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -904,6 +904,35 @@ qemuSetupCpuCgroup(virDomainObj *vm)
> }
>
>
> +static int
> +qemuSetupCgroupVMID(virDomainObj *vm)
> +{
> + qemuDomainObjPrivate *priv = vm->privateData;
> + int inode = virCgroupGetInode(priv->cgroup);
> + const char *path = "/sys/class/fc/fc_udev_device/appid_store";
> + g_autofree char *appid = NULL;
> +
> + if (!vm->def->vmid)
> + return 0;
> +
> + appid = g_strdup_printf("%X:%s", inode, vm->def->vmid);
> +
> + if (virFileWriteStr(path, appid, 0) < 0) {
> + if (errno == ENOENT) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("FC VMID not available on this host"));
> + return -1;
> + }
> +
> + virReportSystemError(errno,
> + _("Unable to write '%s' to
'%s'"), appid, path);
Alternatively, use the following pattern:
if (errno == ENOENT) {
virReportError();
} else {
virReportSystemError();
}
return -1;
Or use just virReportSystemError(); I believe "Unable to write to
/sys/class/...: No such file or directory" is pretty comprehensive.
I wanted to have nice explanatory error messages where missing presence
of the file means that the kernel is old and doesn't have the feature
implemented at all.
I wanted to do the same for case where the feature is not enabled in
kernel config but that is basically impossible. The file will still be
present but any write to it will return EINVAL. However, the same
happens for actual invalid string format or if invalid inode and so on.
I guess I'll drop this attempt too and use single error message as
you've suggested.
Pavel
3:06 a.m.
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
Pavel Hrdina (3):
vircgroup: introduce virCgroupGetInode function
conf: introduce support for Fibre Channel VMID
qemu: implement support for Fibre Channel VMID
docs/formatdomain.rst | 13 ++++++++++
docs/schemas/domaincommon.rng | 11 +++++++++
src/conf/domain_conf.c | 5 ++++
src/conf/domain_conf.h | 2 ++
src/conf/domain_validate.c | 19 +++++++++++++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 32 +++++++++++++++++++++++++
src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++
src/util/vircgroup.h | 2 ++
tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++
tests/genericxml2xmltest.c | 2 ++
11 files changed, 143 insertions(+)
create mode 100644 tests/genericxml2xmlindata/vmid.xml
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1200
days inactive
1201
days old
16 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Michal Prívozník -
Pavel Hrdina