[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]