[libvirt] [PATCH 0/4] Cleanup storage volume creation and tests and provide better error messages
This series cleans up a few aspects when creating storage volumes and adds error messages if the metadata preallocation flag is not supported. Peter Krempa (4): storagevolxml2argvtest: Report better error messages on test failure storage: Clean up function header and reflow error message storage: Avoid unnecessary ternary operators and refactor the code storage: Provide better error message if metadata pre-alloc is unsupported src/storage/storage_backend.c | 132 +++++++++++++++++++++++------------------ tests/storagevolxml2argvtest.c | 7 ++- 2 files changed, 77 insertions(+), 62 deletions(-) -- 1.8.2.1
If the creation of the commandline failed, libvirt always reported "out of memory" from the virCommandToString function rather than the proper error that happened in virStorageBackendCreateQemuImgCmd. Error out earlier. --- tests/storagevolxml2argvtest.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index e1507b4..92ab2f2 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -60,9 +60,7 @@ testCompareXMLToArgvFiles(bool shouldFail, cmd = virStorageBackendCreateQemuImgCmd(conn, &poolobj, vol, inputvol, flags, create_tool, imgformat); - - actualCmdline = virCommandToString(cmd); - if (!actualCmdline) { + if (!cmd) { if (shouldFail) { virResetLastError(); ret = 0; @@ -70,6 +68,9 @@ testCompareXMLToArgvFiles(bool shouldFail, goto cleanup; } + if (!(actualCmdline = virCommandToString(cmd))) + goto cleanup; + len = virtTestLoadFile(cmdline, &expectedCmdline); if (len < 0) goto cleanup; -- 1.8.2.1
On 06/05/2013 10:59 AM, Peter Krempa wrote:
If the creation of the commandline failed, libvirt always reported "out of memory" from the virCommandToString function rather than the proper error that happened in virStorageBackendCreateQemuImgCmd. Error out earlier. --- tests/storagevolxml2argvtest.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
ACK (and I can drop it from my qcow2 extensions series) Jan
Comply with the coding standard and save a few lines. --- src/storage/storage_backend.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 66b3ff7..ace9cae 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -388,9 +388,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); if (vol->target.encryption != NULL) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("storage pool does not support encrypted " - "volumes")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("storage pool does not support encrypted volumes")); goto cleanup; } @@ -529,9 +528,11 @@ cleanup: return ret; } -static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virCommandPtr cmd) { +static int +virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virCommandPtr cmd) +{ struct stat st; gid_t gid; uid_t uid; @@ -594,7 +595,8 @@ enum { QEMU_IMG_BACKING_FORMAT_OPTIONS, }; -static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) +static int +virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { char *help = NULL; char *start; @@ -996,9 +998,9 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, inputvol->target.format != VIR_STORAGE_FILE_RAW)) { if ((tool_type = virStorageBackendFindFSImageTool(NULL)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("creation of non-raw file images is " - "not supported without qemu-img.")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("creation of non-raw file images is " + "not supported without qemu-img.")); return NULL; } @@ -1013,7 +1015,8 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, virStorageBackendPtr -virStorageBackendForType(int type) { +virStorageBackendForType(int type) +{ unsigned int i; for (i = 0; backends[i]; i++) if (backends[i]->type == type) -- 1.8.2.1
On 06/05/2013 10:59 AM, Peter Krempa wrote:
Comply with the coding standard and save a few lines. --- src/storage/storage_backend.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
ACK Jan
Setting of local variables in virStorageBackendCreateQemuImgCmd was unnecessarily cluttered with ternary operators and repeated testing of of conditions. This patch refactors the function to use if statements and optimizes the code flow resulting in a line count reduction. --- src/storage/storage_backend.c | 80 +++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 45 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ace9cae..4846210 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -649,53 +649,56 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, virCommandPtr cmd = NULL; bool do_encryption = (vol->target.encryption != NULL); unsigned long long int size_arg; - bool preallocate = false; - - /* Treat output block devices as 'raw' format */ - const char *type = - virStorageFileFormatTypeToString(vol->type == VIR_STORAGE_VOL_BLOCK ? - VIR_STORAGE_FILE_RAW : - vol->target.format); - - const char *backingType = vol->backingStore.path ? - virStorageFileFormatTypeToString(vol->backingStore.format) : NULL; - - const char *inputBackingPath = (inputvol ? inputvol->backingStore.path - : NULL); - const char *inputPath = inputvol ? inputvol->target.path : NULL; - /* Treat input block devices as 'raw' format */ - const char *inputType = inputPath ? - virStorageFileFormatTypeToString(inputvol->type == VIR_STORAGE_VOL_BLOCK ? - VIR_STORAGE_FILE_RAW : - inputvol->target.format) : - NULL; + bool preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA); + const char *type; + const char *backingType = NULL; + const char *inputPath = NULL; + const char *inputType = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); - preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA); + /* Treat output block devices as 'raw' format */ + type = virStorageFileFormatTypeToString(vol->type == VIR_STORAGE_VOL_BLOCK ? + VIR_STORAGE_FILE_RAW : + vol->target.format); - if (type == NULL) { + if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol type %d"), vol->target.format); return NULL; } - if (inputvol && inputType == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol type %d"), - inputvol->target.format); - return NULL; - } - if (preallocate && vol->target.format != VIR_STORAGE_FILE_QCOW2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("metadata preallocation only available with qcow2")); - return NULL; + + if (inputvol) { + inputPath = inputvol->target.path; + inputType = virStorageFileFormatTypeToString(inputvol->type == VIR_STORAGE_VOL_BLOCK ? + VIR_STORAGE_FILE_RAW : + inputvol->target.format); + + if (!inputType) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + inputvol->target.format); + 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 (STRNEQ_NULLABLE(inputvol->backingStore.path, vol->backingStore.path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a different backing store cannot be specified.")); + return NULL; + } } if (vol->backingStore.path) { int accessRetCode = -1; char *absolutePath = NULL; + backingType = virStorageFileFormatTypeToString(vol->backingStore.format); + if (preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation conflicts with backing" @@ -703,19 +706,6 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, 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 && - (!inputBackingPath || - STRNEQ(inputBackingPath, vol->backingStore.path))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("a different backing store cannot " - "be specified.")); - return NULL; - } - if (backingType == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol backing store type %d"), -- 1.8.2.1
On 06/05/2013 10:59 AM, Peter Krempa wrote:
Setting of local variables in virStorageBackendCreateQemuImgCmd was unnecessarily cluttered with ternary operators and repeated testing of of conditions.
This patch refactors the function to use if statements and optimizes the code flow resulting in a line count reduction. --- src/storage/storage_backend.c | 80 +++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 45 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ace9cae..4846210 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -649,53 +649,56 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
...
- if (preallocate && vol->target.format != VIR_STORAGE_FILE_QCOW2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("metadata preallocation only available with qcow2")); - return NULL;
Why did you remove this error? It would be nice if you could add a test for it. :)
+ + if (inputvol) { + inputPath = inputvol->target.path; + inputType = virStorageFileFormatTypeToString(inputvol->type == VIR_STORAGE_VOL_BLOCK ? + VIR_STORAGE_FILE_RAW : + inputvol->target.format);
Before, inputType was only filled if inputvol->target.path was non-NULL. Do we need to check for it explicitly, or is it not possible to call this function with an inputvol that has no target?
+ + if (!inputType) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + inputvol->target.format); + 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 (STRNEQ_NULLABLE(inputvol->backingStore.path, vol->backingStore.path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a different backing store cannot be specified.")); + return NULL; + }
Before, this check was only done when vol->backingStore.path is not NULL. Now, you won't be allowed to clone a volume with a backing store to a volume without it. Jan
Instead of a unknown flag error report that metadata pre-allocation is not supported with the requested volume creation method. --- src/storage/storage_backend.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4846210..6127fd3 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -254,7 +254,14 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, gid_t gid; uid_t uid; - virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); + + if (flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata preallocation is not supported for block " + "volumes")); + goto cleanup; + } if ((fd = open(vol->target.path, O_RDWR)) < 0) { virReportSystemError(errno, @@ -385,7 +392,14 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, int fd = -1; int operation_flags; - virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); + + if (flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata preallocation is not supported for raw " + "volumes")); + goto cleanup; + } if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -887,7 +901,14 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, char *size; virCommandPtr cmd; - virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); + + if (flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata preallocation is not supported with " + "qcow-create")); + return -1; + } if (inputvol) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.8.2.1
On 06/05/2013 10:59 AM, Peter Krempa wrote:
Instead of a unknown flag error report that metadata pre-allocation is not supported with the requested volume creation method. --- src/storage/storage_backend.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
ACK if you include the public BZ link in the commit message. Jan
On 06/05/13 12:09, Ján Tomko wrote:> On 06/05/2013 10:59 AM, Peter Krempa wrote:
Instead of a unknown flag error report that metadata pre-allocation is not supported with the requested volume creation method. --- src/storage/storage_backend.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
ACK if you include the public BZ link in the commit message.
Thanks, I've amended the commit message and pushed patches 1,2 and 4. I'll repost v3 after I fix it. Peter
participants (2)
-
Ján Tomko -
Peter Krempa