3:59 a.m.
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
3:59 a.m.
New subject: [libvirt] [PATCH 1/4] storagevolxml2argvtest: Report better error messages on test failure
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
5:09 a.m.
New subject: [libvirt] [PATCH 1/4] storagevolxml2argvtest: Report better error messages on test failure
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
3:59 a.m.
New subject: [libvirt] [PATCH 2/4] storage: Clean up function header and reflow error message
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
5:09 a.m.
New subject: [libvirt] [PATCH 2/4] storage: Clean up function header and reflow error message
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
3:59 a.m.
New subject: [libvirt] [PATCH 3/4] storage: Avoid unnecessary ternary operators and refactor the code
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
5:09 a.m.
New subject: [libvirt] [PATCH 3/4] storage: Avoid unnecessary ternary operators and refactor the code
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
3:59 a.m.
New subject: [libvirt] [PATCH 4/4] storage: Provide better error message if metadata pre-alloc is unsupported
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
5:09 a.m.
New subject: [libvirt] [PATCH 4/4] storage: Provide better error message if metadata pre-alloc is unsupported
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
8:03 a.m.
New subject: [libvirt] [PATCH 4/4] storage: Provide better error message if metadata pre-alloc is unsupported
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
4188
days inactive
4188
days old
9 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa