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