On 24.04.2015 14:38, Maros Zatko wrote:
> ---
> docs/schemas/domaincommon.rng | 12 ++++++++++++
> src/conf/domain_conf.c | 19 +++++++++++++++++++
> src/conf/domain_conf.h | 10 ++++++++++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_capabilities.c | 6 +++++-
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 11 +++++++++++
> 7 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 03fd541..5a330d1 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1683,6 +1683,9 @@
> <optional>
> <ref name="driverIOThread"/>
> </optional>
> + <optional>
> + <ref name="detect_zeroes"/>
> + </optional>
> <empty/>
> </element>
> </define>
> @@ -1761,6 +1764,15 @@
> </choice>
> </attribute>
> </define>
> + <define name="detect_zeroes">
> + <attribute name="detect_zeroes">
> + <choice>
> + <value>off</value>
> + <value>on</value>
> + <value>unmap</value>
> + </choice>
> + </attribute>
> + </define>
> <define name="driverIOThread">
> <attribute name='iothread'>
> <ref name="unsignedInt"/>
Adding a new element/attribute to RNG schema must always go with
documentation.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4d7e3c9..e8f45ce 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -761,6 +761,11 @@ VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST,
> VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST,
> "passthrough")
>
> +VIR_ENUM_IMPL(virDomainDiskDetectZeroes, VIR_DOMAIN_DISK_DETECT_ZEROES_LAST,
> + "off",
> + "on",
> + "unmap")
Otherwise I can't tell the different between 'on' and 'unmap'.
> +
> VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST,
> "default",
> "unmap",
> @@ -6079,6 +6084,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> char *mirrorFormat = NULL;
> char *mirrorType = NULL;
> char *domain_name = NULL;
> + char *detect_zeroes = NULL;
> int expected_secret_usage = -1;
> int auth_secret_usage = -1;
> int ret = 0;
> @@ -6215,6 +6221,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> event_idx = virXMLPropString(cur, "event_idx");
> copy_on_read = virXMLPropString(cur, "copy_on_read");
> discard = virXMLPropString(cur, "discard");
> + detect_zeroes = virXMLPropString(cur, "detect_zeroes");
> driverIOThread = virXMLPropString(cur, "iothread");
> } else if (!def->mirror &&
> xmlStrEqual(cur->name, BAD_CAST "mirror") &&
> @@ -6802,6 +6809,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> def->copy_on_read = cor;
> }
>
> + if (detect_zeroes) {
> + if ((def->detect_zeroes = virDomainDiskDetectZeroesTypeFromString(detect_zeroes)) <= 0) {
These two can be joined into one:
if (detect_zeroes &&
virDomainDisk...)
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown disk detect-zeroes mode '%s'"), detect_zeroes);
> + goto error;
> + }
> + }
> +
> if (discard) {
> if ((def->discard = virDomainDiskDiscardTypeFromString(discard)) <= 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -6936,6 +6951,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> VIR_FREE(mirrorType);
> VIR_FREE(mirrorFormat);
> VIR_FREE(domain_name);
> + VIR_FREE(detect_zeroes);
>
> ctxt->node = save_ctxt;
> return def;
> @@ -17865,6 +17881,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
> const char *copy_on_read = virTristateSwitchTypeToString(def->copy_on_read);
> const char *sgio = virDomainDeviceSGIOTypeToString(def->sgio);
> const char *discard = virDomainDiskDiscardTypeToString(def->discard);
> + const char *detect_zeroes = virDomainDiskDetectZeroesTypeToString(def->detect_zeroes);
>
> if (!type || !def->src->type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -17942,6 +17959,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " copy_on_read='%s'", copy_on_read);
> if (def->discard)
> virBufferAsprintf(buf, " discard='%s'", discard);
> + if (def->detect_zeroes)
> + virBufferAsprintf(buf, " detect-zeroes='%s'", detect_zeroes);
> if (def->iothread)
> virBufferAsprintf(buf, " iothread='%u'", def->iothread);
> virBufferAddLit(buf, "/>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index e6fa3c9..24a00eb 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -638,6 +638,14 @@ typedef enum {
> VIR_DOMAIN_DISK_DISCARD_LAST
> } virDomainDiskDiscard;
>
> +typedef enum {
> + VIR_DOMAIN_DISK_DETECT_ZEROES_DEFAULT = 0,
> + VIR_DOMAIN_DISK_DETECT_ZEROES_ON,
> + VIR_DOMAIN_DISK_DETECT_ZEROES_UNMAP,
> +
> + VIR_DOMAIN_DISK_DETECT_ZEROES_LAST
> +} virDomainDiskDetectZeroes;
> +
Interesting, DEFAULT maps to "off". I guess we want DEFAULT mapping to
"default" (and keeping it =0 so it does not format anywhere), and OFF
which would map to "off". Remember, this is generic XML which should
serve other drivers too. So even if qemu currently may not accept "off",
our XML should.
/me goes to check qemu source code.
Oh, so qemu accepts: off, on, unmap. So we should accept those values
too. I know your code does, but what I am suggesting is that if user
types in no attribute, we don't format it back. If he types any of
accepted values, we format it in the XML back too. And probably we can
put 'off' on the cmd line as well.
> typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
> struct _virDomainBlockIoTuneInfo {
> unsigned long long total_bytes_sec;
> @@ -725,6 +733,7 @@ struct _virDomainDiskDef {
> int discard; /* enum virDomainDiskDiscard */
> unsigned int iothread; /* unused = 0, > 0 specific thread # */
> char *domain_name; /* backend domain name */
> + int detect_zeroes; /* enum virDomainDiskDetectZeroes */
> };
>
>
> @@ -2906,6 +2915,7 @@ VIR_ENUM_DECL(virDomainDiskErrorPolicy)
> VIR_ENUM_DECL(virDomainDiskIo)
> VIR_ENUM_DECL(virDomainDeviceSGIO)
> VIR_ENUM_DECL(virDomainDiskTray)
> +VIR_ENUM_DECL(virDomainDiskDetectZeroes)
> VIR_ENUM_DECL(virDomainDiskDiscard)
> VIR_ENUM_DECL(virDomainDiskMirrorState)
> VIR_ENUM_DECL(virDomainController)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7166283..bfed1e3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -246,6 +246,7 @@ virDomainDiskDefForeachPath;
> virDomainDiskDefFree;
> virDomainDiskDefNew;
> virDomainDiskDefSourceParse;
> +virDomainDiskDetectZeroesTypeToString;
And also FromString; It doesn't matter that it is not used now. It's
introduced in this commit and therefore it should be exposed in this
commit too.
> virDomainDiskDeviceTypeToString;
> virDomainDiskDiscardTypeToString;
> virDomainDiskErrorPolicyTypeFromString;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 3b49271..71cce9b 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -279,6 +279,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> "qxl.vgamem_mb",
> "qxl-vga.vgamem_mb",
> "pc-dimm",
> +
> + "detect_zeroes", /* 185 */
> );
>
>
> @@ -1041,7 +1043,6 @@ virQEMUCapsComputeCmdFlags(const char *help,
> {
> const char *p;
> const char *fsdev, *netdev;
> -
No. This space helps to keep the code structured. It's like an empty
line between paragraphs.
> if (strstr(help, "-no-kqemu"))
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_KQEMU);
> if (strstr(help, "-enable-kqemu"))
> @@ -1085,6 +1086,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_DRIVE_COPY_ON_READ);
> if (strstr(help, "bps="))
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE);
> + if (strstr(help, "detect-zeroes="))
> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES);
This should not be needed, since we don't parse 'qemu --help'. Only with
ancient qemus which don't support QMP.
> }
> if ((p = strstr(help, "-vga")) && !strstr(help, "-std-vga")) {
> const char *nl = strstr(p, "\n");
> @@ -2509,6 +2512,7 @@ struct virQEMUCapsCommandLineProps {
> static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
> { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE },
> { "drive", "discard", QEMU_CAPS_DRIVE_DISCARD },
> + { "drive", "detect-zeroes", QEMU_CAPS_DRIVE_DETECT_ZEROES },
This means, new qemu capability is detected whenever drive.detect-zeroes
is accessible. Well it is even in our test suite. If you ran 'make
check' you'd have seen 'qemucapabilitiestest' failing. That test needs
adjustment too.
> { "realtime", "mlock", QEMU_CAPS_MLOCK },
> { "boot-opts", "strict", QEMU_CAPS_BOOT_STRICT },
> { "boot-opts", "reboot-timeout", QEMU_CAPS_REBOOT_TIMEOUT },
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index c7b1ac7..7f41a20 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -224,6 +224,7 @@ typedef enum {
> QEMU_CAPS_QXL_VGAMEM = 182, /* -device qxl.vgamem_mb */
> QEMU_CAPS_QXL_VGA_VGAMEM = 183, /* -device qxl-vga.vgamem_mb */
> QEMU_CAPS_DEVICE_PC_DIMM = 184, /* pc-dimm device */
> + QEMU_CAPS_DRIVE_DETECT_ZEROES = 185, /* -drive detect-zeroes */
>
> QEMU_CAPS_LAST, /* this must always be the last item */
> } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e7e0937..3cef993 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3801,6 +3801,17 @@ qemuBuildDriveStr(virConnectPtr conn,
> }
> }
>
> + if (disk->detect_zeroes) {
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) {
> + virBufferAsprintf(&opt, ",detect-zeroes=%s",
> + virDomainDiskDetectZeroesTypeToString(disk->detect_zeroes));
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("detect-zeroes is not supported by this QEMU binary"));
> + goto error;
> + }
> + }
> +
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MONITOR_JSON)) {
> const char *wpolicy = NULL, *rpolicy = NULL;
>
>
What am I missing here is a qemuxml2argv test. Also, you don't need to
have all of this in a single patch (doesn't hurt much either since this
is a small feature with a few lines changed). You can break it into two
patches, in one you'll extend domain XML, parse & format, document the
feature and introduce an .xml into qemuxml2argv test. Then, in the
second patch you actually implement the feature for qemu driver and
complete the test by adding .args file.
Otherwise the code looks good, but I am afraid I need to see a v2 due to
reasons mentioned above.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list