[libvirt] [PATCH 0/7] Cleanup with storageencryption/qemu-img
The first 4 patches were already posted: http://www.redhat.com/archives/libvir-list/2016-May/msg01984.htm http://www.redhat.com/archives/libvir-list/2016-May/msg02147.html There was only one small change in a comment to any of those patches. The last 3 patches are new - just trying to make it easier to use the qemu-img code for creating a luks volume. John Ferlan (7): util: Clean up code formatting in virstorageencryption storage: Split out setting default secret for encryption storage: Split out a helper for encryption checks storage: Adjust qemu-img switches check storage: Create helper to set input for CreateQemuImg code storage: Create helper to set backing for CreateQemuImg code storage: Create helper to set options for CreateQemuImg code src/storage/storage_backend.c | 367 ++++++++++++++++++++++----------------- src/storage/storage_backend_fs.c | 79 +++++---- src/util/virstorageencryption.c | 58 +++---- 3 files changed, 282 insertions(+), 222 deletions(-) -- 2.5.5
Bring style more in line with more recent code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstorageencryption.c | 58 +++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index ec4a8cb..8105158 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -112,8 +112,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, { xmlNodePtr old_node; virStorageEncryptionSecretPtr ret; - char *type_str; - int type; + char *type_str = NULL; char *uuidstr = NULL; if (VIR_ALLOC(ret) < 0) @@ -122,25 +121,21 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, old_node = ctxt->node; ctxt->node = node; - type_str = virXPathString("string(./@type)", ctxt); - if (type_str == NULL) { + if (!(type_str = virXPathString("string(./@type)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown volume encryption secret type")); goto cleanup; } - type = virStorageEncryptionSecretTypeFromString(type_str); - if (type < 0) { + + if ((ret->type = virStorageEncryptionSecretTypeFromString(type_str)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume encryption secret type %s"), type_str); - VIR_FREE(type_str); goto cleanup; } VIR_FREE(type_str); - ret->type = type; - uuidstr = virXPathString("string(./@uuid)", ctxt); - if (uuidstr) { + if ((uuidstr = virXPathString("string(./@uuid)", ctxt))) { if (virUUIDParse(uuidstr, ret->uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("malformed volume encryption uuid '%s'"), @@ -157,6 +152,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, return ret; cleanup: + VIR_FREE(type_str); virStorageEncryptionSecretFree(ret); VIR_FREE(uuidstr); ctxt->node = old_node; @@ -168,46 +164,48 @@ virStorageEncryptionParseXML(xmlXPathContextPtr ctxt) { xmlNodePtr *nodes = NULL; virStorageEncryptionPtr ret; - char *format_str; - int format, n; + char *format_str = NULL; + int n; size_t i; if (VIR_ALLOC(ret) < 0) return NULL; - format_str = virXPathString("string(./@format)", ctxt); - if (format_str == NULL) { + if (!(format_str = virXPathString("string(./@format)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown volume encryption format")); goto cleanup; } - format = virStorageEncryptionFormatTypeFromString(format_str); - if (format < 0) { + + if ((ret->format = + virStorageEncryptionFormatTypeFromString(format_str)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume encryption format type %s"), format_str); - VIR_FREE(format_str); goto cleanup; } VIR_FREE(format_str); - ret->format = format; - n = virXPathNodeSet("./secret", ctxt, &nodes); - if (n < 0) + if ((n = virXPathNodeSet("./secret", ctxt, &nodes)) < 0) goto cleanup; - if (n != 0 && VIR_ALLOC_N(ret->secrets, n) < 0) - goto cleanup; - ret->nsecrets = n; - for (i = 0; i < n; i++) { - ret->secrets[i] = virStorageEncryptionSecretParse(ctxt, nodes[i]); - if (ret->secrets[i] == NULL) + + if (n > 0) { + if (VIR_ALLOC_N(ret->secrets, n) < 0) goto cleanup; + ret->nsecrets = n; + + for (i = 0; i < n; i++) { + if (!(ret->secrets[i] = + virStorageEncryptionSecretParse(ctxt, nodes[i]))) + goto cleanup; + } + VIR_FREE(nodes); } - VIR_FREE(nodes); return ret; cleanup: + VIR_FREE(format_str); VIR_FREE(nodes); virStorageEncryptionFree(ret); return NULL; @@ -248,8 +246,7 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, const char *type; char uuidstr[VIR_UUID_STRING_BUFLEN]; - type = virStorageEncryptionSecretTypeToString(secret->type); - if (!type) { + if (!(type = virStorageEncryptionSecretTypeToString(secret->type))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected volume encryption secret type")); return -1; @@ -268,8 +265,7 @@ virStorageEncryptionFormat(virBufferPtr buf, const char *format; size_t i; - format = virStorageEncryptionFormatTypeToString(enc->format); - if (!format) { + if (!(format = virStorageEncryptionFormatTypeToString(enc->format))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected encryption format")); return -1; -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:06 -0400, John Ferlan wrote:
Bring style more in line with more recent code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstorageencryption.c | 58 +++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 31 deletions(-)
ACK
Split the qcow setting of encryption secrets into a helper Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 79 +++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 45474cb..a11df36 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1280,6 +1280,51 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, } +/* virStorageBackendFileSystemLoadDefaultSecrets: + * @conn: Connection pointer to fetch secret + * @vol: volume being refreshed + * + * If the volume had a QCOW secret generated, we need to regenerate the + * secret + * + * Returns 0 if no secret or secret setup was successful, + * -1 on failures w/ error message set + */ +static int +virStorageBackendFileSystemLoadDefaultSecrets(virConnectPtr conn, + virStorageVolDefPtr vol) +{ + virSecretPtr sec; + virStorageEncryptionSecretPtr encsec = NULL; + + /* Only necessary for qcow format */ + if (!vol->target.encryption || + vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW || + vol->target.encryption->nsecrets != 0) + return 0; + + if (!(sec = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_VOLUME, + vol->target.path))) + return 0; + + if (VIR_ALLOC_N(vol->target.encryption->secrets, 1) < 0 || + VIR_ALLOC(encsec) < 0) { + VIR_FREE(vol->target.encryption->secrets); + virObjectUnref(sec); + return -1; + } + + vol->target.encryption->nsecrets = 1; + vol->target.encryption->secrets[0] = encsec; + + encsec->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE; + virSecretGetUUID(sec, encsec->uuid); + virObjectUnref(sec); + + return 0; +} + + /** * Update info about a volume's capacity/allocation */ @@ -1291,39 +1336,13 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, int ret; /* Refresh allocation / capacity / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, - VIR_STORAGE_VOL_FS_OPEN_FLAGS, 0); - if (ret < 0) + if ((ret = virStorageBackendUpdateVolInfo(vol, false, + VIR_STORAGE_VOL_FS_OPEN_FLAGS, + 0)) < 0) return ret; /* Load any secrets if possible */ - if (vol->target.encryption && - vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW && - vol->target.encryption->nsecrets == 0) { - virSecretPtr sec; - virStorageEncryptionSecretPtr encsec = NULL; - - sec = virSecretLookupByUsage(conn, - VIR_SECRET_USAGE_TYPE_VOLUME, - vol->target.path); - if (sec) { - if (VIR_ALLOC_N(vol->target.encryption->secrets, 1) < 0 || - VIR_ALLOC(encsec) < 0) { - VIR_FREE(vol->target.encryption->secrets); - virObjectUnref(sec); - return -1; - } - - vol->target.encryption->nsecrets = 1; - vol->target.encryption->secrets[0] = encsec; - - encsec->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE; - virSecretGetUUID(sec, encsec->uuid); - virObjectUnref(sec); - } - } - - return 0; + return virStorageBackendFileSystemLoadDefaultSecrets(conn, vol); } static int -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:07 -0400, John Ferlan wrote:
Split the qcow setting of encryption secrets into a helper
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 79 +++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 30 deletions(-)
ACK
Split out a helper from virStorageBackendCreateQemuImgCmdFromVol to check the encryption - soon a new encryption sheriff will be patroling and that'll mean all sorts of new checks. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 79 ++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 28 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 3a23cd7..2076155 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1010,6 +1010,53 @@ virStorageBackendCreateQemuImgOpts(char **opts, return -1; } + +/* virStorageBackendCreateQemuImgCheckEncryption: + * @format: format of file found + * @conn: pointer to connection + * @vol: pointer to volume def + * + * Ensure the proper setup for encryption. + * + * Returns 0 on success, -1 on failure w/ error set + */ +static int +virStorageBackendCreateQemuImgCheckEncryption(int format, + const char *type, + virConnectPtr conn, + virStorageVolDefPtr vol) +{ + virStorageEncryptionPtr enc = vol->target.encryption; + + if (format == VIR_STORAGE_FILE_QCOW || format == VIR_STORAGE_FILE_QCOW2) { + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW && + enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported volume encryption format %d"), + vol->target.encryption->format); + return -1; + } + if (enc->nsecrets > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("too many secrets for qcow encryption")); + return -1; + } + if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT || + enc->nsecrets == 0) { + if (virStorageGenerateQcowEncryption(conn, vol) < 0) + return -1; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("qcow volume encryption unsupported with " + "volume format %s"), type); + return -1; + } + + return 0; +} + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat */ @@ -1133,35 +1180,11 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, } } - if (info.encryption) { - virStorageEncryptionPtr enc; + if (info.encryption && + virStorageBackendCreateQemuImgCheckEncryption(info.format, type, + conn, vol) < 0) + return NULL; - if (info.format != VIR_STORAGE_FILE_QCOW && - info.format != VIR_STORAGE_FILE_QCOW2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("qcow volume encryption unsupported with " - "volume format %s"), type); - return NULL; - } - enc = vol->target.encryption; - if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW && - enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported volume encryption format %d"), - vol->target.encryption->format); - return NULL; - } - if (enc->nsecrets > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("too many secrets for qcow encryption")); - return NULL; - } - if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT || - enc->nsecrets == 0) { - if (virStorageGenerateQcowEncryption(conn, vol) < 0) - return NULL; - } - } /* Size in KB */ info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024); -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:08 -0400, John Ferlan wrote:
Split out a helper from virStorageBackendCreateQemuImgCmdFromVol to check the encryption - soon a new encryption sheriff will be patroling and that'll mean all sorts of new checks.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 79 ++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 28 deletions(-)
ACK
Since we support QEMU 0.12 and later, checking for support of specific flags added prior to that isn't necessary. Thus start with the base of having the "-o options" available for the qemu-img create option and then determine whether we have the compat option for qcow2 files (which would be necessary up through qemu 2.0 where the default changes to compat 0.11). NOTE: Keeping old tests around since it's still possible to create in the old format. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 66 ++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 42 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2076155..4c40e43 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -869,9 +869,19 @@ virStoragePloopResize(virStorageVolDefPtr vol, return ret; } +/* Flag values shared w/ storagevolxml2argvtest.c. + * Since it's still possible to provide the old format args, just + * keep them; however, prefix with an "X_" (similar to qemu_capabilities.c) + * to indicate the are older. + * + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11) + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT + * was made necessary due to 2.0 change to change the default + * qcow2 file format from 0.10 to 1.1. + */ enum { - QEMU_IMG_BACKING_FORMAT_NONE = 0, - QEMU_IMG_BACKING_FORMAT_FLAG, + X_QEMU_IMG_BACKING_FORMAT_NONE = 0, + X_QEMU_IMG_BACKING_FORMAT_FLAG, QEMU_IMG_BACKING_FORMAT_OPTIONS, QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, }; @@ -904,46 +914,18 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { - char *help = NULL; - char *start; - char *end; - char *tmp; - int ret = -1; - int exitstatus; - virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL); - - virCommandAddEnvString(cmd, "LC_ALL=C"); - virCommandSetOutputBuffer(cmd, &help); - virCommandClearCaps(cmd); - - /* qemuimg doesn't return zero exit status on -h, - * therefore we need to provide pointer for storing - * exit status, although we don't parse it any later */ - if (virCommandRun(cmd, &exitstatus) < 0) - goto cleanup; + /* As of QEMU 0.11 the [-o options] support was added via qemu + * commit id '9ea2ea71', so we start with that base and figure + * out what else we have */ + int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS; + + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer + * understands. Since we still support QEMU 0.12 and newer, we need + * to be able to handle the previous format as can be set via a + * compat=0.10 option. */ + if (virStorageBackendQemuImgSupportsCompat(qemuimg)) + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; - if ((start = strstr(help, " create ")) == NULL || - (end = strstr(start, "\n")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to parse qemu-img output '%s'"), - help); - goto cleanup; - } - if (((tmp = strstr(start, "-F fmt")) && tmp < end) || - ((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) { - ret = QEMU_IMG_BACKING_FORMAT_FLAG; - } else if ((tmp = strstr(start, "[-o options]")) && tmp < end) { - if (virStorageBackendQemuImgSupportsCompat(qemuimg)) - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; - else - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS; - } else { - ret = QEMU_IMG_BACKING_FORMAT_NONE; - } - - cleanup: - virCommandFree(cmd); - VIR_FREE(help); return ret; } @@ -1219,7 +1201,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, VIR_FREE(opts); } else { if (info.backingPath) { - if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG) + if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG) virCommandAddArgList(cmd, "-F", backingType, NULL); else VIR_DEBUG("Unable to set backing store format for %s with %s", -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
Since we support QEMU 0.12 and later, checking for support of specific flags added prior to that isn't necessary.
Thus start with the base of having the "-o options" available for the qemu-img create option and then determine whether we have the compat option for qcow2 files (which would be necessary up through qemu 2.0 where the default changes to compat 0.11).
NOTE: Keeping old tests around since it's still possible to create in the old format.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 66 ++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 42 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2076155..4c40e43 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -869,9 +869,19 @@ virStoragePloopResize(virStorageVolDefPtr vol, return ret; }
+/* Flag values shared w/ storagevolxml2argvtest.c. + * Since it's still possible to provide the old format args, just + * keep them; however, prefix with an "X_" (similar to qemu_capabilities.c) + * to indicate the are older.
We use that approach as we can't delete the already existing flags since we wouldn't be able to parse the status XML. For qemu-img we don't store the flags anywhere so the old ones can be deleted.
+ * + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11) + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT + * was made necessary due to 2.0 change to change the default + * qcow2 file format from 0.10 to 1.1. + */ enum { - QEMU_IMG_BACKING_FORMAT_NONE = 0, - QEMU_IMG_BACKING_FORMAT_FLAG, + X_QEMU_IMG_BACKING_FORMAT_NONE = 0,
So this can be dropped entirely after the hunk below.
+ X_QEMU_IMG_BACKING_FORMAT_FLAG,
This will be never set either after the hunk below.
QEMU_IMG_BACKING_FORMAT_OPTIONS, QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, }; @@ -904,46 +914,18 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { - char *help = NULL; - char *start; - char *end; - char *tmp; - int ret = -1; - int exitstatus; - virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL); - - virCommandAddEnvString(cmd, "LC_ALL=C"); - virCommandSetOutputBuffer(cmd, &help); - virCommandClearCaps(cmd); - - /* qemuimg doesn't return zero exit status on -h, - * therefore we need to provide pointer for storing - * exit status, although we don't parse it any later */ - if (virCommandRun(cmd, &exitstatus) < 0) - goto cleanup; + /* As of QEMU 0.11 the [-o options] support was added via qemu + * commit id '9ea2ea71', so we start with that base and figure + * out what else we have */ + int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
So this flag can basically be completely removed as it should be always present in qemu-img 0.12
+ + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer + * understands. Since we still support QEMU 0.12 and newer, we need + * to be able to handle the previous format as can be set via a + * compat=0.10 option. */ + if (virStorageBackendQemuImgSupportsCompat(qemuimg)) + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
Yup, we need this.
- if ((start = strstr(help, " create ")) == NULL || - (end = strstr(start, "\n")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to parse qemu-img output '%s'"), - help); - goto cleanup; - } - if (((tmp = strstr(start, "-F fmt")) && tmp < end) || - ((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) { - ret = QEMU_IMG_BACKING_FORMAT_FLAG; - } else if ((tmp = strstr(start, "[-o options]")) && tmp < end) { - if (virStorageBackendQemuImgSupportsCompat(qemuimg)) - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; - else - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS; - } else { - ret = QEMU_IMG_BACKING_FORMAT_NONE; - } - - cleanup: - virCommandFree(cmd); - VIR_FREE(help); return ret; }
@@ -1219,7 +1201,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, VIR_FREE(opts); } else { if (info.backingPath) { - if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG) + if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG)
So ... if the above is never set, why aren't you dropping this entire case?
virCommandAddArgList(cmd, "-F", backingType, NULL); else VIR_DEBUG("Unable to set backing store format for %s with %s",
On 06/06/2016 09:37 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
Since we support QEMU 0.12 and later, checking for support of specific flags added prior to that isn't necessary.
Thus start with the base of having the "-o options" available for the qemu-img create option and then determine whether we have the compat option for qcow2 files (which would be necessary up through qemu 2.0 where the default changes to compat 0.11).
NOTE: Keeping old tests around since it's still possible to create in the old format.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 66 ++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 42 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2076155..4c40e43 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -869,9 +869,19 @@ virStoragePloopResize(virStorageVolDefPtr vol, return ret; }
+/* Flag values shared w/ storagevolxml2argvtest.c. + * Since it's still possible to provide the old format args, just + * keep them; however, prefix with an "X_" (similar to qemu_capabilities.c) + * to indicate the are older.
We use that approach as we can't delete the already existing flags since we wouldn't be able to parse the status XML. For qemu-img we don't store the flags anywhere so the old ones can be deleted.
+ * + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11) + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT + * was made necessary due to 2.0 change to change the default + * qcow2 file format from 0.10 to 1.1. + */ enum { - QEMU_IMG_BACKING_FORMAT_NONE = 0, - QEMU_IMG_BACKING_FORMAT_FLAG, + X_QEMU_IMG_BACKING_FORMAT_NONE = 0,
So this can be dropped entirely after the hunk below.
+ X_QEMU_IMG_BACKING_FORMAT_FLAG,
This will be never set either after the hunk below.
QEMU_IMG_BACKING_FORMAT_OPTIONS, QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, }; @@ -904,46 +914,18 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { - char *help = NULL; - char *start; - char *end; - char *tmp; - int ret = -1; - int exitstatus; - virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL); - - virCommandAddEnvString(cmd, "LC_ALL=C"); - virCommandSetOutputBuffer(cmd, &help); - virCommandClearCaps(cmd); - - /* qemuimg doesn't return zero exit status on -h, - * therefore we need to provide pointer for storing - * exit status, although we don't parse it any later */ - if (virCommandRun(cmd, &exitstatus) < 0) - goto cleanup; + /* As of QEMU 0.11 the [-o options] support was added via qemu + * commit id '9ea2ea71', so we start with that base and figure + * out what else we have */ + int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
So this flag can basically be completely removed as it should be always present in qemu-img 0.12
+ + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer + * understands. Since we still support QEMU 0.12 and newer, we need + * to be able to handle the previous format as can be set via a + * compat=0.10 option. */ + if (virStorageBackendQemuImgSupportsCompat(qemuimg)) + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
Yup, we need this.
- if ((start = strstr(help, " create ")) == NULL || - (end = strstr(start, "\n")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to parse qemu-img output '%s'"), - help); - goto cleanup; - } - if (((tmp = strstr(start, "-F fmt")) && tmp < end) || - ((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) { - ret = QEMU_IMG_BACKING_FORMAT_FLAG; - } else if ((tmp = strstr(start, "[-o options]")) && tmp < end) { - if (virStorageBackendQemuImgSupportsCompat(qemuimg)) - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; - else - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS; - } else { - ret = QEMU_IMG_BACKING_FORMAT_NONE; - } - - cleanup: - virCommandFree(cmd); - VIR_FREE(help); return ret; }
@@ -1219,7 +1201,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, VIR_FREE(opts); } else { if (info.backingPath) { - if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG) + if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG)
So ... if the above is never set, why aren't you dropping this entire case?
It wasn't clear if tests/storagevolxml2argvtest.c should then be removed, adjusted, or otherwise. It replicated the flags I put the X_ in front of as: enum { FMT_NONE = 0, FMT_FLAG, FMT_OPTIONS, FMT_COMPAT, }; Whether we care or not to support/test all those old options was unclear to me. I would think at some point in time qemu-img would drop those in preference for the new arguments. I you think essentially doing away with all those old options is fine, I can certain do/add that. John
virCommandAddArgList(cmd, "-F", backingType, NULL); else VIR_DEBUG("Unable to set backing store format for %s with %s",
On Mon, Jun 06, 2016 at 09:46:22 -0400, John Ferlan wrote:
On 06/06/2016 09:37 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
Since we support QEMU 0.12 and later, checking for support of specific flags added prior to that isn't necessary.
Thus start with the base of having the "-o options" available for the qemu-img create option and then determine whether we have the compat option for qcow2 files (which would be necessary up through qemu 2.0 where the default changes to compat 0.11).
NOTE: Keeping old tests around since it's still possible to create in the old format.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 66 ++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 42 deletions(-)
[...]
So ... if the above is never set, why aren't you dropping this entire case?
It wasn't clear if tests/storagevolxml2argvtest.c should then be removed, adjusted, or otherwise. It replicated the flags I put the X_ in front of as:
enum { FMT_NONE = 0, FMT_FLAG, FMT_OPTIONS, FMT_COMPAT, };
Whether we care or not to support/test all those old options was unclear to me. I would think at some point in time qemu-img would drop those in
I don't think it makes any sense to test options that we won't ever pass to qemu-img so we can drop it.
Create helper virStorageBackendCreateQemuImgSetInput to set the input Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 50 +++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4c40e43..4a3c41d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -943,6 +943,7 @@ struct _virStorageBackendQemuImgInfo { int backingFormat; const char *inputPath; + const char *inputType; int inputFormat; }; @@ -1039,6 +1040,32 @@ virStorageBackendCreateQemuImgCheckEncryption(int format, } +static int +virStorageBackendCreateQemuImgSetInput(virStorageVolDefPtr inputvol, + struct _virStorageBackendQemuImgInfo *info) +{ + if (!(info->inputPath = inputvol->target.path)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing input volume target path")); + return -1; + } + + info->inputFormat = inputvol->target.format; + if (inputvol->type == VIR_STORAGE_VOL_BLOCK) + info->inputFormat = VIR_STORAGE_FILE_RAW; + if (!(info->inputType = + virStorageFileFormatTypeToString(info->inputFormat))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + info->inputFormat); + return -1; + } + + return 0; +} + + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat */ @@ -1054,7 +1081,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virCommandPtr cmd = NULL; const char *type; const char *backingType = NULL; - const char *inputType = NULL; char *opts = NULL; struct _virStorageBackendQemuImgInfo info = { .format = vol->target.format, @@ -1095,23 +1121,9 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return NULL; } - if (inputvol) { - if (!(info.inputPath = inputvol->target.path)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("missing input volume target path")); - return NULL; - } - - info.inputFormat = inputvol->target.format; - if (inputvol->type == VIR_STORAGE_VOL_BLOCK) - info.inputFormat = VIR_STORAGE_FILE_RAW; - if (!(inputType = virStorageFileFormatTypeToString(info.inputFormat))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol type %d"), - info.inputFormat); - return NULL; - } - } + if (inputvol && + virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0) + return NULL; if (vol->target.backingStore) { int accessRetCode = -1; @@ -1180,7 +1192,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, } if (info.inputPath) - virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + virCommandAddArgList(cmd, "convert", "-f", info.inputType, "-O", type, NULL); else virCommandAddArgList(cmd, "create", "-f", type, NULL); -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:10 -0400, John Ferlan wrote:
Create helper virStorageBackendCreateQemuImgSetInput to set the input
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 50 +++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4c40e43..4a3c41d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -943,6 +943,7 @@ struct _virStorageBackendQemuImgInfo { int backingFormat;
const char *inputPath; + const char *inputType;
This is misleading. That's the string version of 'inputFormat'. I'd call it inputFormatStr if you wan't to store it here.
int inputFormat; };
ACK with the tweak
Create a helper virStorageBackendCreateQemuImgSetBacking to perform the backing store set Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 118 ++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 51 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4a3c41d..624790f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -940,6 +940,7 @@ struct _virStorageBackendQemuImgInfo { bool nocow; const char *backingPath; + const char *backingType; int backingFormat; const char *inputPath; @@ -1065,6 +1066,66 @@ virStorageBackendCreateQemuImgSetInput(virStorageVolDefPtr inputvol, } +static int +virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + struct _virStorageBackendQemuImgInfo *info) +{ + int accessRetCode = -1; + char *absolutePath = NULL; + + info->backingFormat = vol->target.backingStore->format; + info->backingPath = vol->target.backingStore->path; + + if (info->preallocate) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata preallocation conflicts with backing" + " store")); + return -1; + } + + /* XXX: Not strictly required: qemu-img has an option a different + * backing store, not really sure what use it serves though, and it + * may cause issues with lvm. Untested essentially. + */ + if (inputvol && inputvol->target.backingStore && + STRNEQ_NULLABLE(inputvol->target.backingStore->path, + info->backingPath)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a different backing store cannot be specified.")); + return -1; + } + + if (!(info->backingType = + virStorageFileFormatTypeToString(info->backingFormat))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol backing store type %d"), + info->backingFormat); + return -1; + } + + /* Convert relative backing store paths to absolute paths for access + * validation. + */ + if ('/' != *(info->backingPath) && + virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, + info->backingPath) < 0) + return -1; + accessRetCode = access(absolutePath ? absolutePath : + info->backingPath, R_OK); + VIR_FREE(absolutePath); + if (accessRetCode != 0) { + virReportSystemError(errno, + _("inaccessible backing store volume %s"), + info->backingPath); + return -1; + } + + return 0; +} + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat @@ -1080,7 +1141,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, { virCommandPtr cmd = NULL; const char *type; - const char *backingType = NULL; char *opts = NULL; struct _virStorageBackendQemuImgInfo info = { .format = vol->target.format, @@ -1125,54 +1185,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0) return NULL; - if (vol->target.backingStore) { - int accessRetCode = -1; - char *absolutePath = NULL; - - info.backingFormat = vol->target.backingStore->format; - info.backingPath = vol->target.backingStore->path; - - if (info.preallocate) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("metadata preallocation conflicts with backing" - " store")); - return NULL; - } - - /* XXX: Not strictly required: qemu-img has an option a different - * backing store, not really sure what use it serves though, and it - * may cause issues with lvm. Untested essentially. - */ - if (inputvol && inputvol->target.backingStore && - STRNEQ_NULLABLE(inputvol->target.backingStore->path, info.backingPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("a different backing store cannot be specified.")); - return NULL; - } - - if (!(backingType = virStorageFileFormatTypeToString(info.backingFormat))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol backing store type %d"), - info.backingFormat); - return NULL; - } - - /* Convert relative backing store paths to absolute paths for access - * validation. - */ - if ('/' != *(info.backingPath) && - virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - info.backingPath) < 0) - return NULL; - accessRetCode = access(absolutePath ? absolutePath : info.backingPath, R_OK); - VIR_FREE(absolutePath); - if (accessRetCode != 0) { - virReportSystemError(errno, - _("inaccessible backing store volume %s"), - info.backingPath); - return NULL; - } - } + if (vol->target.backingStore && + virStorageBackendCreateQemuImgSetBacking(pool, vol, inputvol, + &info) < 0) + return NULL; if (info.encryption && virStorageBackendCreateQemuImgCheckEncryption(info.format, type, @@ -1188,7 +1204,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, /* ignore the backing volume when we're converting a volume */ if (info.inputPath) { info.backingPath = NULL; - backingType = NULL; + info.backingType = NULL; } if (info.inputPath) @@ -1214,7 +1230,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, } else { if (info.backingPath) { if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG) - virCommandAddArgList(cmd, "-F", backingType, NULL); + virCommandAddArgList(cmd, "-F", info.backingType, NULL); else VIR_DEBUG("Unable to set backing store format for %s with %s", info.path, create_tool); -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:11 -0400, John Ferlan wrote:
Create a helper virStorageBackendCreateQemuImgSetBacking to perform the backing store set
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 118 ++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 51 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4a3c41d..624790f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -940,6 +940,7 @@ struct _virStorageBackendQemuImgInfo { bool nocow;
const char *backingPath; + const char *backingType;
backingFormatStr.
int backingFormat;
const char *inputPath;
ACK with the adjustment
On 06/06/2016 09:49 AM, Peter Krempa wrote:
On Fri, Jun 03, 2016 at 06:42:11 -0400, John Ferlan wrote:
Create a helper virStorageBackendCreateQemuImgSetBacking to perform the backing store set
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 118 ++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 51 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4a3c41d..624790f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -940,6 +940,7 @@ struct _virStorageBackendQemuImgInfo { bool nocow;
const char *backingPath; + const char *backingType;
backingFormatStr.
int backingFormat;
const char *inputPath;
ACK with the adjustment
Given that patch 4 will change to remove the else condition, this patch won't be necessary John
Create a helper virStorageBackendCreateQemuImgSetOptions to set either the qemu-img -o options or the previous mechanism using -F Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 58 ++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 624790f..a12480f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1126,6 +1126,38 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, } +static int +virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, + int imgformat, + struct _virStorageBackendQemuImgInfo info) +{ + char *opts = NULL; + + if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { + if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && + imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) + info.compat = "0.10"; + + if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) + return -1; + if (opts) + virCommandAddArgList(cmd, "-o", opts, NULL); + VIR_FREE(opts); + } else { + if (info.backingPath) { + if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG) + virCommandAddArgList(cmd, "-F", info.backingType, NULL); + else + VIR_DEBUG("Unable to set backing store format for %s", + info.path); + } + if (info.encryption) + virCommandAddArg(cmd, "-e"); + } + + return 0; +} + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat @@ -1141,7 +1173,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, { virCommandPtr cmd = NULL; const char *type; - char *opts = NULL; struct _virStorageBackendQemuImgInfo info = { .format = vol->target.format, .path = vol->target.path, @@ -1215,28 +1246,9 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, if (info.backingPath) virCommandAddArgList(cmd, "-b", info.backingPath, NULL); - if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { - if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && - imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) - info.compat = "0.10"; - - if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) { - virCommandFree(cmd); - return NULL; - } - if (opts) - virCommandAddArgList(cmd, "-o", opts, NULL); - VIR_FREE(opts); - } else { - if (info.backingPath) { - if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG) - virCommandAddArgList(cmd, "-F", info.backingType, NULL); - else - VIR_DEBUG("Unable to set backing store format for %s with %s", - info.path, create_tool); - } - if (info.encryption) - virCommandAddArg(cmd, "-e"); + if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, info) < 0) { + virCommandFree(cmd); + return NULL; } if (info.inputPath) -- 2.5.5
On Fri, Jun 03, 2016 at 06:42:12 -0400, John Ferlan wrote:
Create a helper virStorageBackendCreateQemuImgSetOptions to set either the qemu-img -o options or the previous mechanism using -F
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 58 ++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 23 deletions(-)
This will need changes after updating 4/7 so I'll review the updated version.
On 06/03/2016 06:42 AM, John Ferlan wrote:
The first 4 patches were already posted:
http://www.redhat.com/archives/libvir-list/2016-May/msg01984.htm
http://www.redhat.com/archives/libvir-list/2016-May/msg02147.html
There was only one small change in a comment to any of those patches.
The last 3 patches are new - just trying to make it easier to use the qemu-img code for creating a luks volume.
John Ferlan (7): util: Clean up code formatting in virstorageencryption storage: Split out setting default secret for encryption storage: Split out a helper for encryption checks storage: Adjust qemu-img switches check storage: Create helper to set input for CreateQemuImg code storage: Create helper to set backing for CreateQemuImg code storage: Create helper to set options for CreateQemuImg code
src/storage/storage_backend.c | 367 ++++++++++++++++++++++----------------- src/storage/storage_backend_fs.c | 79 +++++---- src/util/virstorageencryption.c | 58 +++---- 3 files changed, 282 insertions(+), 222 deletions(-)
I have pushed patches 1, 2, 3, and an adjusted 5. I'll respin 4 with test removals and 7 with appropriate adjustments. Tks - John
participants (2)
-
John Ferlan -
Peter Krempa