Re: [libvirt] [PATCH 2/4] conf: Adding XML support for 'xres' and 'yres'
On Mon, Aug 5, 2019 at 9:25 AM wrote:
> From: Julio Faracco
>
> XML need to support both properties together. This commit adds XML
> support for QXL model if they are set. Domain configuration is able to
> parse this properties.
>
> Signed-off-by: Julio Faracco
> ---
> src/conf/domain_conf.c | 26 ++
> src/conf/domain_conf.h | 2 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 441eb1a5a2..120c6ccf5f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15360,6 +15360,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> VIR_AUTOFREE(char *) ram = NULL;
> VIR_AUTOFREE(char *) vgamem = NULL;
> VIR_AUTOFREE(char *) primary = NULL;
> +VIR_AUTOFREE(char *) xres = NULL;
> +VIR_AUTOFREE(char *) yres = NULL;
>
> if (!(def = virDomainVideoDefNew()))
> return NULL;
> @@ -15377,6 +15379,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> vram64 = virXMLPropString(cur, "vram64");
> vgamem = virXMLPropString(cur, "vgamem");
> heads = virXMLPropString(cur, "heads");
> +xres = virXMLPropString(cur, "xres");
> +yres = virXMLPropString(cur, "yres");
>
> if ((primary = virXMLPropString(cur, "primary")) != NULL)
> {
> if (STREQ(primary, "yes"))
> @@ -15459,6 +15463,24 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> }
> }
>
> +if (xres && yres) {
> +if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("xres and yres attribute only supported for
> type of qxl"));
>
The error msg is not right here. Not only qxl type support xres, bus
also VGA, virtio, bochs types.
How about a write a general function for qemu hypervisor driver to verify
if a video type support xres,
and then add xres attribute to all the supported video types?
BTW, there is a bug to track this feature:
https://bugzilla.redhat.com/show_bug.cgi?id=1485793
Please mention the bug in your commit msg. Thank you
> +goto error;
> +}
> +if (virStrToLong_uip(xres, NULL, 10, >xres) < 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("cannot parse video xres '%s'"), xres);
> +goto error;
> +}
> +if (virStrToLong_uip(yres, NULL, 10, >yres) < 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("cannot parse video yres '%s'"), yres);
> +goto error;
> +}
> +}
> +
> if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
> goto error;
>
> @@ -26427,6 +26449,10 @@ virDomainVideoDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " vgamem='%u'", def->vgamem);
> if (def->heads)
> virBufferAsprintf(buf, " heads='%u'", def->heads);
> +if (def->xres)
> +virBufferAsprintf(buf, " xres='%u'", def->xres);
> +if (def->yres)
> +virBufferAsprintf(buf, " yres='%u'", def->yres);
> if (def->primary)
> virBufferAddLit(buf, " primary='yes'");
> if (def->accel) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8a4425df64..bfee86efcf 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1419,6 +1419,8 @@ struct _virDomainVideoDef {
> unsigned int vram64; /* kibibytes (multiples of 1024) */
> unsigned int vgamem; /* kibibytes (multiples of 1024) */
> unsigned int heads;
> +unsigned int xres;
> +unsigned int yres;
> bool primary;
> virDomainVideoAccelDefPtr accel;
> virDomainVideoDriverDefPtr driver;
> --
> 2.20.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Best regards,
---
Han Han
Quality Engineer
Redhat.
Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] Adding resolution properties for QXL device
From: Julio Faracco This serie adds 'xres' and 'yres' properties into XML definition for QXL video device to specify a default resolution. This serie covers a simple test case too. Julio Faracco (4): docs: Adding 'xres' and 'yres' into qxl XML definition conf: Adding XML support for 'xres' and 'yres' qemu: Generate 'xres' and 'yres' for qxl device. tests: Add separate tests for 'xres' and 'yres' docs/schemas/domaincommon.rng | 10 + src/conf/domain_conf.c| 26 src/conf/domain_conf.h| 2 + src/qemu/qemu_command.c | 4 ++ .../video-qxl-resolution.args | 32 +++ .../qemuxml2argvdata/video-qxl-resolution.xml | 40 +++ tests/qemuxml2argvtest.c | 4 ++ .../video-qxl-resolution.xml | 40 +++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 159 insertions(+) create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.args create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.xml create mode 100644 tests/qemuxml2xmloutdata/video-qxl-resolution.xml -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] conf: Adding XML support for 'xres' and 'yres'
From: Julio Faracco
XML need to support both properties together. This commit adds XML
support for QXL model if they are set. Domain configuration is able to
parse this properties.
Signed-off-by: Julio Faracco
---
src/conf/domain_conf.c | 26 ++
src/conf/domain_conf.h | 2 ++
2 files changed, 28 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 441eb1a5a2..120c6ccf5f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15360,6 +15360,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_AUTOFREE(char *) ram = NULL;
VIR_AUTOFREE(char *) vgamem = NULL;
VIR_AUTOFREE(char *) primary = NULL;
+VIR_AUTOFREE(char *) xres = NULL;
+VIR_AUTOFREE(char *) yres = NULL;
if (!(def = virDomainVideoDefNew()))
return NULL;
@@ -15377,6 +15379,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
vram64 = virXMLPropString(cur, "vram64");
vgamem = virXMLPropString(cur, "vgamem");
heads = virXMLPropString(cur, "heads");
+xres = virXMLPropString(cur, "xres");
+yres = virXMLPropString(cur, "yres");
if ((primary = virXMLPropString(cur, "primary")) != NULL) {
if (STREQ(primary, "yes"))
@@ -15459,6 +15463,24 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
}
+if (xres && yres) {
+if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("xres and yres attribute only supported for type
of qxl"));
+goto error;
+}
+if (virStrToLong_uip(xres, NULL, 10, >xres) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("cannot parse video xres '%s'"), xres);
+goto error;
+}
+if (virStrToLong_uip(yres, NULL, 10, >yres) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("cannot parse video yres '%s'"), yres);
+goto error;
+}
+}
+
if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
goto error;
@@ -26427,6 +26449,10 @@ virDomainVideoDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " vgamem='%u'", def->vgamem);
if (def->heads)
virBufferAsprintf(buf, " heads='%u'", def->heads);
+if (def->xres)
+virBufferAsprintf(buf, " xres='%u'", def->xres);
+if (def->yres)
+virBufferAsprintf(buf, " yres='%u'", def->yres);
if (def->primary)
virBufferAddLit(buf, " primary='yes'");
if (def->accel) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8a4425df64..bfee86efcf 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1419,6 +1419,8 @@ struct _virDomainVideoDef {
unsigned int vram64; /* kibibytes (multiples of 1024) */
unsigned int vgamem; /* kibibytes (multiples of 1024) */
unsigned int heads;
+unsigned int xres;
+unsigned int yres;
bool primary;
virDomainVideoAccelDefPtr accel;
virDomainVideoDriverDefPtr driver;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] tests: Add separate tests for 'xres' and 'yres'
From: Julio Faracco
New tests to verify resolution properties of a simple qxl video.
Signed-off-by: Julio Faracco
---
.../video-qxl-resolution.args | 32 +++
.../qemuxml2argvdata/video-qxl-resolution.xml | 40 +++
tests/qemuxml2argvtest.c | 4 ++
.../video-qxl-resolution.xml | 40 +++
tests/qemuxml2xmltest.c | 1 +
5 files changed, 117 insertions(+)
create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.args
create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.xml
create mode 100644 tests/qemuxml2xmloutdata/video-qxl-resolution.xml
diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.args
b/tests/qemuxml2argvdata/video-qxl-resolution.args
new file mode 100644
index 00..71370ff735
--- /dev/null
+++ b/tests/qemuxml2argvdata/video-qxl-resolution.args
@@ -0,0 +1,32 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-realtime mlock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,max_outputs=1,\
+xres=1280,yres=720,bus=pci.0,addr=0x2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.xml
b/tests/qemuxml2argvdata/video-qxl-resolution.xml
new file mode 100644
index 00..c6275c1bc5
--- /dev/null
+++ b/tests/qemuxml2argvdata/video-qxl-resolution.xml
@@ -0,0 +1,40 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219136
+ 219136
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-i686
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c166fd18d6..b67ba8f135 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2003,6 +2003,10 @@ mymain(void)
QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_QXL_MAX_OUTPUTS);
+DO_TEST("video-qxl-resolution",
+QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+QEMU_CAPS_DEVICE_QXL,
+QEMU_CAPS_QXL_MAX_OUTPUTS);
DO_TEST("video-virtio-gpu-device",
QEMU_CAPS_DEVICE_VIRTIO_GPU,
QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
diff --git a/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
new file mode 100644
index 00..c6275c1bc5
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
@@ -0,0 +1,40 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219136
+ 219136
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-i686
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 525eb9a740..62485e89d9 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1189,6 +1189,7 @@ mymain(void)
QEMU_CAPS_DEVICE_QXL);
DO_TEST("video-qxl-heads", NONE);
DO_TEST("video-qxl-noheads", NONE);
+DO_TEST("video-qxl-resolution", NONE);
DO_TEST("video-virtio-gpu-secondary", NONE);
DO_TEST("video-virtio-gpu-ccw",
QEMU_CAPS_CCW,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] qemu: Generate 'xres' and 'yres' for qxl device.
From: Julio Faracco
Now, QEMU command line can define 'xres' and 'yres' if XML contains both
properties from qxl model.
Signed-off-by: Julio Faracco
---
src/qemu/qemu_command.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fee51158a9..82430e7e98 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4713,6 +4713,10 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
if (video->heads)
virBufferAsprintf(, ",max_outputs=%u", video->heads);
}
+
+if (video->xres && video->yres)
+virBufferAsprintf(, ",xres=%u,yres=%u", video->xres,
video->yres);
+
} else if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS)) {
if (video->heads)
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] docs: Adding 'xres' and 'yres' into qxl XML definition
From: Julio Faracco This commit adds 'xres' and 'yres' into qxl XML Domain group definition. Both, properties were added into properties group inside qxl model and they are set as optional. Signed-off-by: Julio Faracco --- docs/schemas/domaincommon.rng | 10 ++ 1 file changed, 10 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a0771da45b..8d95948595 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3606,6 +3606,16 @@ + + + + + + + + + + -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] test_driver: implement virDomainSetBlockIoTune
On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis
> ---
>
> I deliberately omitted the logic from qemuDomainSetBlockIoTuneDefaults
> in order to leave the function simpler. I think that in the case of the
> test driver it is ok.
>
> After all, how we handle the parameters it is supposed to be
> hypervisor-specific.
>
> src/test/test_driver.c | 196 +
> 1 file changed, 196 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 9f4e255e35..3272ecf4b7 100755
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3502,6 +3502,201 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> }
>
>
> +static int
> +testDomainSetBlockIoTune(virDomainPtr dom,
> + const char *path,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +virDomainDefPtr def = NULL;
> +virDomainBlockIoTuneInfo info = {0};
> +virDomainDiskDefPtr conf_disk = NULL;
> +virTypedParameterPtr eventParams = NULL;
> +int eventNparams = 0;
> +int eventMaxparams = 0;
> +size_t i;
> +int ret = -1;
> +
> +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG, -1);
> +
> +if (virTypedParamsValidate(params, nparams,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> + VIR_TYPED_PARAM_STRING,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + NULL) < 0)
> +return -1;
> +
> +if (!(vm = testDomObjFromDomain(dom)))
> +return -1;
> +
> +if (!(def = virDomainObjGetOneDef(vm, flags)))
> +goto cleanup;
> +
> +if (!(conf_disk = virDomainDiskByName(def, path, true))) {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("missing persistent configuration for disk '%s'"),
> + path);
> +goto cleanup;
> +}
> +
> +info = conf_disk->blkdeviotune;
> +if (VIR_STRDUP(info.group_name, conf_disk->blkdeviotune.group_name) < 0)
> +goto cleanup;
> +
> +if (virTypedParamsAddString(, , ,
> +VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
> +goto cleanup;
> +
> +#define
Re: [libvirt] [PATCH 1/2] test_driver: implement virDomainGetBlockIoTune
On Sun, Aug 04, 2019 at 03:46:03PM +0200, Ilias Stamatis wrote:
> On Sun, Aug 4, 2019 at 11:12 AM Erik Skultety wrote:
> >
> > On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis
> > > ---
> > > src/test/test_driver.c | 90 ++
> > > 1 file changed, 90 insertions(+)
> > >
> > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > index ab0f8b06d6..9f4e255e35 100755
> > > --- a/src/test/test_driver.c
> > > +++ b/src/test/test_driver.c
> > > @@ -3500,6 +3500,95 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> > > virDomainObjEndAPI();
> > > return ret;
> > > }
> > > +
> > > +
> > > +static int
> > > +testDomainGetBlockIoTune(virDomainPtr dom,
> > > + const char *path,
> > > + virTypedParameterPtr params,
> > > + int *nparams,
> > > + unsigned int flags)
> > > +{
> > > +virDomainObjPtr vm = NULL;
> > > +virDomainDefPtr def = NULL;
> > > +virDomainDiskDefPtr disk;
> > > +virDomainBlockIoTuneInfo reply = {0};
> > > +int ret = -1;
> > > +
> > > +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > > + VIR_DOMAIN_AFFECT_CONFIG |
> > > + VIR_TYPED_PARAM_STRING_OKAY, -1);
> > > +
> > > +flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> > > +
> > > +if (*nparams == 0) {
> > > +*nparams = 20;
> > > +return 0;
> > > +}
> > > +
> > > +if (!(vm = testDomObjFromDomain(dom)))
> > > +return -1;
> > > +
> > > +if (!(def = virDomainObjGetOneDef(vm, flags)))
> > > +goto cleanup;
> > > +
> > > +if (!(disk = virDomainDiskByName(def, path, true))) {
> > > +virReportError(VIR_ERR_INVALID_ARG,
> > > + _("disk '%s' was not found in the domain config"),
> > > + path);
> > > +goto cleanup;
> > > +}
> > > +
> > > +reply = disk->blkdeviotune;
> > > +if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name) < 0)
> > > +goto cleanup;
> > > +
> > > +#define ULL VIR_TYPED_PARAM_ULLONG
> >
> > I don't see a point in doing ^this just to save a few characters, we're way
> > over the 80 columns already anyway, so wrap the long lines and span the
> > macro
> > call over multiple lines.
>
> That's true that we're already over the 80 columns, but not by that
> much so I thought that by doing it like that it allows us to "save" 13
> new lines.
Well, there isn't strictly a rule of thumb here. It's always a personal
preference, but I look at it whether we gained anything by this "translation",
we saved 13 lines, great. However, if you split the lines properly, are we
really going to lose anything? No, not even in terms of readability.
>
> > Funny that I remember us having a discussion about macros doing string
> > concatenation (which I don't really mind that much) but prevents you from
> > jumping
> > directly to the symbol definition - that's exactly what the QEMU alternative
> > does, I guess just to save some common prefixes :D.
>
> Actually in this case it doesn't really prevent you. Just you need an
> extra jump. One to jump to the definition of the ULL and from there to
> jump to the next definition (even though all the ULL usage fits in a
> single screen).
>
> Instead if there are many (instead of a single one) strings like like
> "TOTAL_BYTES_SEC", "READ_BYTES_SEC" etc that are concatenated/extended
> magically by some macro it makes it trickier to find the original
> definitions. But that's just my opinion.
Ironically, patch 2 does exactly the concatenation magic ;).
>
> For me it's alright to use the QEMU macro as well, but we have already
> the TEST_SET_PARAM which is used everywhere else so it makes more
> sense to use it here too for consistency reasons I believe.
I didn't object to using TEST_SET_PARAM, I merely suggested a minor adjustment
to it with other potentially necessary minor adjustments along the way.
Erik
>
> >
> > Looking at the functions that use the typed parameters, an improvement we
> > could
> > definitely do (in a standalone patch) is to ditch the index from the macro
> > definition since it's quite fragile, by doing:
> >
> > int maxparams = X;
> >
> > if (*nparams == 0) {
> > *nparams = maxparams;
> > return 0;
> > }
> >
> > *nparams = 0;
> >
> > TEST_SET_PARAM([*nparams++],...)
> >
> > > +TEST_SET_PARAM(0, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, ULL,
> > > reply.total_bytes_sec);
> > > +TEST_SET_PARAM(1, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, ULL,
> > > reply.read_bytes_sec);
> > > +TEST_SET_PARAM(2, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, ULL,
> > > reply.write_bytes_sec);
> > > +
> > > +TEST_SET_PARAM(3, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, ULL,
> > > reply.total_iops_sec);
> > > +TEST_SET_PARAM(4, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, ULL,
> > > reply.read_iops_sec);
> >
Re: [libvirt] [PATCH 1/2] test_driver: implement virDomainGetBlockIoTune
On Sun, Aug 4, 2019 at 11:12 AM Erik Skultety wrote:
>
> On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis
> > ---
> > src/test/test_driver.c | 90 ++
> > 1 file changed, 90 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index ab0f8b06d6..9f4e255e35 100755
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3500,6 +3500,95 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> > virDomainObjEndAPI();
> > return ret;
> > }
> > +
> > +
> > +static int
> > +testDomainGetBlockIoTune(virDomainPtr dom,
> > + const char *path,
> > + virTypedParameterPtr params,
> > + int *nparams,
> > + unsigned int flags)
> > +{
> > +virDomainObjPtr vm = NULL;
> > +virDomainDefPtr def = NULL;
> > +virDomainDiskDefPtr disk;
> > +virDomainBlockIoTuneInfo reply = {0};
> > +int ret = -1;
> > +
> > +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > + VIR_DOMAIN_AFFECT_CONFIG |
> > + VIR_TYPED_PARAM_STRING_OKAY, -1);
> > +
> > +flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> > +
> > +if (*nparams == 0) {
> > +*nparams = 20;
> > +return 0;
> > +}
> > +
> > +if (!(vm = testDomObjFromDomain(dom)))
> > +return -1;
> > +
> > +if (!(def = virDomainObjGetOneDef(vm, flags)))
> > +goto cleanup;
> > +
> > +if (!(disk = virDomainDiskByName(def, path, true))) {
> > +virReportError(VIR_ERR_INVALID_ARG,
> > + _("disk '%s' was not found in the domain config"),
> > + path);
> > +goto cleanup;
> > +}
> > +
> > +reply = disk->blkdeviotune;
> > +if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name) < 0)
> > +goto cleanup;
> > +
> > +#define ULL VIR_TYPED_PARAM_ULLONG
>
> I don't see a point in doing ^this just to save a few characters, we're way
> over the 80 columns already anyway, so wrap the long lines and span the macro
> call over multiple lines.
That's true that we're already over the 80 columns, but not by that
much so I thought that by doing it like that it allows us to "save" 13
new lines.
> Funny that I remember us having a discussion about macros doing string
> concatenation (which I don't really mind that much) but prevents you from
> jumping
> directly to the symbol definition - that's exactly what the QEMU alternative
> does, I guess just to save some common prefixes :D.
Actually in this case it doesn't really prevent you. Just you need an
extra jump. One to jump to the definition of the ULL and from there to
jump to the next definition (even though all the ULL usage fits in a
single screen).
Instead if there are many (instead of a single one) strings like like
"TOTAL_BYTES_SEC", "READ_BYTES_SEC" etc that are concatenated/extended
magically by some macro it makes it trickier to find the original
definitions. But that's just my opinion.
For me it's alright to use the QEMU macro as well, but we have already
the TEST_SET_PARAM which is used everywhere else so it makes more
sense to use it here too for consistency reasons I believe.
>
> Looking at the functions that use the typed parameters, an improvement we
> could
> definitely do (in a standalone patch) is to ditch the index from the macro
> definition since it's quite fragile, by doing:
>
> int maxparams = X;
>
> if (*nparams == 0) {
> *nparams = maxparams;
> return 0;
> }
>
> *nparams = 0;
>
> TEST_SET_PARAM([*nparams++],...)
>
> > +TEST_SET_PARAM(0, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, ULL,
> > reply.total_bytes_sec);
> > +TEST_SET_PARAM(1, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, ULL,
> > reply.read_bytes_sec);
> > +TEST_SET_PARAM(2, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, ULL,
> > reply.write_bytes_sec);
> > +
> > +TEST_SET_PARAM(3, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, ULL,
> > reply.total_iops_sec);
> > +TEST_SET_PARAM(4, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, ULL,
> > reply.read_iops_sec);
> > +TEST_SET_PARAM(5, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, ULL,
> > reply.write_iops_sec);
> > +
> > +TEST_SET_PARAM(6, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, ULL,
> > reply.total_bytes_sec_max);
> > +TEST_SET_PARAM(7, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, ULL,
> > reply.read_bytes_sec_max);
> > +TEST_SET_PARAM(8, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, ULL,
> > reply.write_bytes_sec_max);
> > +
> > +TEST_SET_PARAM(9, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, ULL,
> > reply.total_iops_sec_max);
> > +TEST_SET_PARAM(10, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, ULL,
> > reply.read_iops_sec_max);
> > +TEST_SET_PARAM(11, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, ULL,
> > reply.write_iops_sec_max);
> > +
> > +TEST_SET_PARAM(12,
[libvirt] [PATCH v2 4/4] test_driver: implement virDomainFSTrim
Signed-off-by: Ilias Stamatis
---
src/test/test_driver.c | 32
1 file changed, 32 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2a9041d777..cb373f61d3 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4177,6 +4177,37 @@ testDomainFSThaw(virDomainPtr dom,
}
+static int
+testDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+virDomainObjPtr vm;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = testDomObjFromDomain(dom)))
+return -1;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto cleanup;
+
+if (mountPoint && STRNEQ(mountPoint, "/") && STRNEQ(mountPoint, "/boot")) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountPoint);
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+
static int testDomainGetAutostart(virDomainPtr domain,
int *autostart)
{
@@ -8825,6 +8856,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
.domainFSFreeze = testDomainFSFreeze, /* 5.7.0 */
.domainFSThaw = testDomainFSThaw, /* 5.7.0 */
+.domainFSTrim = testDomainFSTrim, /* 5.7.0 */
.domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
.domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
.domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/4] test_driver: implement virDomainFSFreeze
On success update the domain-private data. Consider / and /boot to be
the only mountpoints avaiable in order to be consistent with the other
FS-related calls.
Signed-off-by: Ilias Stamatis
---
src/test/test_driver.c | 72 ++
1 file changed, 72 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 88a1d330c1..a8b75b175a 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -390,6 +390,8 @@ typedef struct _testDomainObjPrivate testDomainObjPrivate;
typedef testDomainObjPrivate *testDomainObjPrivatePtr;
struct _testDomainObjPrivate {
testDriverPtr driver;
+
+bool frozen[2]; /* used by file system related calls */
};
@@ -402,6 +404,7 @@ testDomainObjPrivateAlloc(void *opaque)
return NULL;
priv->driver = opaque;
+priv->frozen[0] = priv->frozen[1] = false;
return priv;
}
@@ -4046,6 +4049,74 @@ static int testDomainUndefine(virDomainPtr domain)
return testDomainUndefineFlags(domain, 0);
}
+
+static int
+testDomainFSFreeze(virDomainPtr dom,
+ const char **mountpoints,
+ unsigned int nmountpoints,
+ unsigned int flags)
+{
+virDomainObjPtr vm;
+testDomainObjPrivatePtr priv;
+size_t i;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = testDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto cleanup;
+
+priv = vm->privateData;
+
+if (nmountpoints == 0) {
+ret = 0;
+if (priv->frozen[0] == false) {
+priv->frozen[0] = true;
+ret++;
+}
+if (priv->frozen[1] == false) {
+priv->frozen[1] = true;
+ret++;
+}
+} else {
+int nfreeze = 0;
+bool freeze[2];
+
+memcpy(, priv->frozen, 2);
+
+for (i = 0; i < nmountpoints; i++) {
+if (STREQ(mountpoints[i], "/")) {
+if (!freeze[0]) {
+freeze[0] = true;
+nfreeze++;
+}
+} else if (STREQ(mountpoints[i], "/boot")) {
+if (!freeze[1]) {
+freeze[1] = true;
+nfreeze++;
+}
+} else {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountpoints[i]);
+goto cleanup;
+}
+}
+
+/* steal the helper copy */
+memcpy(priv->frozen, , 2);
+ret = nfreeze;
+}
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+
static int testDomainGetAutostart(virDomainPtr domain,
int *autostart)
{
@@ -8692,6 +8763,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainDefineXMLFlags = testDomainDefineXMLFlags, /* 1.2.12 */
.domainUndefine = testDomainUndefine, /* 0.1.11 */
.domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
+.domainFSFreeze = testDomainFSFreeze, /* 5.7.0 */
.domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
.domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
.domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/4] test_driver: implement FS-related APIs
Ilias Stamatis (4): test_driver: introduce domain-private data test_driver: implement virDomainFSFreeze test_driver: implement virDomainFSThaw test_driver: implement virDomainFSTrim src/test/test_driver.c | 200 - 1 file changed, 199 insertions(+), 1 deletion(-) -- 2.22.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/4] test_driver: introduce domain-private data
vm-specific data can be used by APIs that need to preserve some state
between calls
Some of them are:
- FS-related APIs for remembering which mountpoints are frozen
- virDomainSetTime / virDomainGetTime for maintaining time information
- virDomainSetIOThreadParams for storing the I/O thread parameters
- virDomainManagedSaveDefineXML for internally storing the VM definition
Signed-off-by: Ilias Stamatis
Reviewed-by: Erik Skultety
---
src/test/test_driver.c | 35 ++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index aae9875194..88a1d330c1 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -386,6 +386,35 @@ testBuildCapabilities(virConnectPtr conn)
}
+typedef struct _testDomainObjPrivate testDomainObjPrivate;
+typedef testDomainObjPrivate *testDomainObjPrivatePtr;
+struct _testDomainObjPrivate {
+testDriverPtr driver;
+};
+
+
+static void *
+testDomainObjPrivateAlloc(void *opaque)
+{
+testDomainObjPrivatePtr priv;
+
+if (VIR_ALLOC(priv) < 0)
+return NULL;
+
+priv->driver = opaque;
+
+return priv;
+}
+
+
+static void
+testDomainObjPrivateFree(void *data)
+{
+testDomainObjPrivatePtr priv = data;
+VIR_FREE(priv);
+}
+
+
static testDriverPtr
testDriverNew(void)
{
@@ -401,6 +430,10 @@ testDriverNew(void)
VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT |
VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING,
};
+virDomainXMLPrivateDataCallbacks privatecb = {
+.alloc = testDomainObjPrivateAlloc,
+.free = testDomainObjPrivateFree,
+};
testDriverPtr ret;
if (testDriverInitialize() < 0)
@@ -409,7 +442,7 @@ testDriverNew(void)
if (!(ret = virObjectLockableNew(testDriverClass)))
return NULL;
-if (!(ret->xmlopt = virDomainXMLOptionNew(, NULL, , NULL, NULL))
||
+if (!(ret->xmlopt = virDomainXMLOptionNew(, , , NULL,
NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
!(ret->ifaces = virInterfaceObjListNew()) ||
!(ret->domains = virDomainObjListNew()) ||
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] test_driver: implement virDomainFSFreeze
On Fri, Aug 2, 2019 at 4:39 PM Erik Skultety wrote:
>
> On Tue, Jul 09, 2019 at 09:23:22PM +0200, Ilias Stamatis wrote:
> > On success update the domain-private data. Consider / and /boot to be
> > the only mountpoints avaiable in order to be consistent with the other
> > FS-related calls.
> >
> > Signed-off-by: Ilias Stamatis
> > ---
> > src/test/test_driver.c | 58 ++
> > 1 file changed, 58 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index af3503c523..8c25c679a5 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -388,6 +388,8 @@ typedef struct _testDomainObjPrivate
> > testDomainObjPrivate;
> > typedef testDomainObjPrivate *testDomainObjPrivatePtr;
> > struct _testDomainObjPrivate {
> > testDriverPtr driver;
> > +
> > +bool frozen[2]; /* used by file system related calls */
> > };
> >
> >
> > @@ -400,6 +402,7 @@ testDomainObjPrivateAlloc(void *opaque)
> > return NULL;
> >
> > priv->driver = opaque;
> > +priv->frozen[0] = priv->frozen[1] = false;
> >
> > return priv;
> > }
> > @@ -3439,6 +3442,60 @@ static int testDomainUndefine(virDomainPtr domain)
> > return testDomainUndefineFlags(domain, 0);
> > }
> >
> > +
> > +static int
> > +testDomainFSFreeze(virDomainPtr dom,
> > + const char **mountpoints,
> > + unsigned int nmountpoints,
> > + unsigned int flags)
> > +{
> > +virDomainObjPtr vm;
> > +testDomainObjPrivatePtr priv;
> > +size_t i;
> > +int slash = 0, slash_boot = 0;
>
> One declaration per line please. Also, we should define these explicitly as
> booleans, the standard states these would return 0 and 1 respectively.
>
I have seen in many places in the codebase many variables declared in
the same line so that's what I thought it was okay when the variables
are somehow coupled or related. But okay, apparently the single
declaration per line rule has been added later.
> > +int ret = -1;
> > +
> > +virCheckFlags(0, -1);
> > +
> > +if (!(vm = testDomObjFromDomain(dom)))
> > +goto cleanup;
> > +
> > +if (virDomainObjCheckActive(vm) < 0)
> > +goto cleanup;
> > +
> > +priv = vm->privateData;
> > +
> > +if (nmountpoints == 0) {
> > +priv->frozen[0] = priv->frozen[1] = true;
> > +ret = 2;
>
> Well, this is not always true, e.g. if '/' was frozen before, ret should be 1.
> Similarly, if both were frozen prior to calling this API, we should return 0.
Right. I don't remember why but I thought in the past that it made
sense to report that we froze a partition even if it was already
frozen from before. Maybe for simplicity? I can't recall my original
thinking right now.
But thinking about it again, sure, let's make it freeze only what's
not frozen already. I'm sending a new patch with implementing your
feedback.
Thanks!
Ilias
>
> > +} else {
> > +for (i = 0; i < nmountpoints; i++) {
> > +if (STREQ(mountpoints[i], "/")) {
> > +slash = 1;
> > +} else if (STREQ(mountpoints[i], "/boot")) {
> > +slash_boot = 1;
>
> ^here too, if the filesystems were already frozen, we should not account for
> them in @ret.
>
> > +} else {
> > +virReportError(VIR_ERR_OPERATION_INVALID,
> > + _("mount point not found: %s"),
> > + mountpoints[i]);
> > +goto cleanup;
> > +}
> > +}
> > +
> > +if (slash)
> > +priv->frozen[0] = true;
> > +if (slash_boot)
> > +priv->frozen[1] = true;
> > +
> > +ret = slash + slash_boot;
> > +}
>
> We could do ^this or alternatively, we could introduce another iterator
> @nfreeze, use it within the loop. We could also declare:
>
> bool freeze[2];
>
> //backup the original values:
> memcpy(, priv->frozen, 2);
>
> for (mountpoints) {
> //set the helper array
> if (mount[i] == / && !freeze[i]) {
> freeze[i] = true;
> nfreeze++;
> }
>
> else if (mount[i] == /boot && !freeze[i]) {
> freeze[i] = true;
> nfreeze++;
> }
> }
> ret = nfreeze;
>
> //steal the helper copy
> memcpy(priv->frozen, , 2);
>
> return ret;
>
> Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] test_driver: implement virDomainGetBlockIoTune
On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis
> ---
> src/test/test_driver.c | 90 ++
> 1 file changed, 90 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index ab0f8b06d6..9f4e255e35 100755
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3500,6 +3500,95 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> virDomainObjEndAPI();
> return ret;
> }
> +
> +
> +static int
> +testDomainGetBlockIoTune(virDomainPtr dom,
> + const char *path,
> + virTypedParameterPtr params,
> + int *nparams,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +virDomainDefPtr def = NULL;
> +virDomainDiskDefPtr disk;
> +virDomainBlockIoTuneInfo reply = {0};
> +int ret = -1;
> +
> +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG |
> + VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> +
> +if (*nparams == 0) {
> +*nparams = 20;
> +return 0;
> +}
> +
> +if (!(vm = testDomObjFromDomain(dom)))
> +return -1;
> +
> +if (!(def = virDomainObjGetOneDef(vm, flags)))
> +goto cleanup;
> +
> +if (!(disk = virDomainDiskByName(def, path, true))) {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("disk '%s' was not found in the domain config"),
> + path);
> +goto cleanup;
> +}
> +
> +reply = disk->blkdeviotune;
> +if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name) < 0)
> +goto cleanup;
> +
> +#define ULL VIR_TYPED_PARAM_ULLONG
I don't see a point in doing ^this just to save a few characters, we're way
over the 80 columns already anyway, so wrap the long lines and span the macro
call over multiple lines.
Funny that I remember us having a discussion about macros doing string
concatenation (which I don't really mind that much) but prevents you from
jumping
directly to the symbol definition - that's exactly what the QEMU alternative
does, I guess just to save some common prefixes :D.
Looking at the functions that use the typed parameters, an improvement we could
definitely do (in a standalone patch) is to ditch the index from the macro
definition since it's quite fragile, by doing:
int maxparams = X;
if (*nparams == 0) {
*nparams = maxparams;
return 0;
}
*nparams = 0;
TEST_SET_PARAM([*nparams++],...)
> +TEST_SET_PARAM(0, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, ULL,
> reply.total_bytes_sec);
> +TEST_SET_PARAM(1, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, ULL,
> reply.read_bytes_sec);
> +TEST_SET_PARAM(2, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, ULL,
> reply.write_bytes_sec);
> +
> +TEST_SET_PARAM(3, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, ULL,
> reply.total_iops_sec);
> +TEST_SET_PARAM(4, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, ULL,
> reply.read_iops_sec);
> +TEST_SET_PARAM(5, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, ULL,
> reply.write_iops_sec);
> +
> +TEST_SET_PARAM(6, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, ULL,
> reply.total_bytes_sec_max);
> +TEST_SET_PARAM(7, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, ULL,
> reply.read_bytes_sec_max);
> +TEST_SET_PARAM(8, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, ULL,
> reply.write_bytes_sec_max);
> +
> +TEST_SET_PARAM(9, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, ULL,
> reply.total_iops_sec_max);
> +TEST_SET_PARAM(10, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, ULL,
> reply.read_iops_sec_max);
> +TEST_SET_PARAM(11, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, ULL,
> reply.write_iops_sec_max);
> +
> +TEST_SET_PARAM(12, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, ULL,
> reply.size_iops_sec);
> +
> +TEST_SET_PARAM(13, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> VIR_TYPED_PARAM_STRING, reply.group_name);
> +reply.group_name = NULL;
> +
> +TEST_SET_PARAM(14, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> ULL,
> + reply.total_bytes_sec_max_length);
> +TEST_SET_PARAM(15, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,
> ULL,
> + reply.read_bytes_sec_max_length);
> +TEST_SET_PARAM(16, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,
> ULL,
> + reply.write_bytes_sec_max_length);
> +
> +TEST_SET_PARAM(17, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,
> ULL,
> + reply.total_iops_sec_max_length);
> +TEST_SET_PARAM(18, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, ULL,
> + reply.read_iops_sec_max_length);
> +TEST_SET_PARAM(19, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,
> ULL,
> + reply.write_iops_sec_max_length);
> +#undef ULL
> +
> +if (*nparams > 20)
> +
