Re: [Qemu-devel] [PATCH v2 3/7] qemu-img: add more conv= conversions to dd

[Stefan Hajnoczi]

On Fri, Aug 26, 2016 at 11:16:39AM +0200, Reda Sallahi wrote:
> @@ -3968,7 +3963,11 @@ static int img_dd_flag(const char *arg, struct DdIo 
> *io,
>          int j;
>          for (j = 0; flags[j].name != NULL; j++) {
>              if (!strcmp(tok, flags[j].name)) {
> -                io->flags |= flags[j].value;
> +                if (dd) {
> +                    dd->conv |= flags[j].value;
> +                } else {
> +                    io->flags |= flags[j].value;
> +                }
>                  break;
>              }
>          }

Does this function need to know about DdIo and DdInfo?  How about:

static int img_dd_flag(const char *arg, unsigned int *flag_bits,
                       const struct DdSymbols *flags, const char *err_str)

Then function can be called with &dd->flags or &io->flags.

If you do this then please squash the change into earlier patches in
this series using git-rebase(1).  That way the function is created with
its final prototype in the earliest patch that needs it and there is no
code churn.

> @@ -4219,6 +4260,7 @@ static int img_dd(int argc, char **argv)
>              ret = -1;
>              goto out;
>          }
> +        local_err = NULL;

Why set local_err to NULL here and not where it's declared?  If it could
possibly be non-NULL then error_free() must be used to avoid a memory
leak.

> +        if (dd.conv & C_SPARSE) {
> +            if (buffer_is_zero(in.buf, bsz)) {
> +                sparse_count++;
> +                continue;
> +            }
> +            if (sparse_count > 0) {
> +                out_pos += sparse_count * in.bsz;
> +                sparse_count = 0;
> +            }
> +        }

Is sparse_count needed at all?

if ((dd.conv & C_SPARSE) && buffer_is_zero(in.buf, bsz)) {
    out_pos += in.bsz;
    continue;
}

signature.asc
Description: PGP signature