Re: [Qemu-devel] [PATCH v2 1/9] qemu-io: Allow explicitly specifying format

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

On 11/07/2014 12:39 PM, Kevin Wolf wrote:
> This adds a -f option to qemu-io which allows to explicitly specify the
> block driver to use for the given image.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 

>  "\n"
>  "  -c, --cmd STRING     execute command with its arguments\n"
>  "                       from the given string\n"
> +"  -f, --format FMT     specifies the block driver to use\n"
>  "  -r, --read-only      export read-only\n"

Documented in one order...

>  "  -s, --snapshot       use snapshot file\n"
>  "  -n, --nocache        disable host cache\n"
> @@ -364,12 +366,13 @@ int main(int argc, char **argv)
>  {
>      int readonly = 0;
>      int growable = 0;
> -    const char *sopt = "hVc:d:rsnmgkt:T:";
> +    const char *sopt = "hVc:d:f:rsnmgkt:T:";

inserted into the longopt in a different order...

>      const struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
>          { "offset", 1, NULL, 'o' },
>          { "cmd", 1, NULL, 'c' },
> +        { "format", 1, NULL, 'f' },
>          { "read-only", 0, NULL, 'r' },

then spelled out in the first order...


> @@ -407,6 +413,13 @@ int main(int argc, char **argv)
>                  exit(1);
>              }
>              break;
> +        case 'f':
> +            drv = bdrv_find_format(optarg);
> +            if (!drv) {
> +                error_report("Invalid format '%s'", optarg);
> +                exit(EXIT_FAILURE);
> +            }
> +            break;
>          case 'c':

...and finally implemented in yet another order.  While it doesn't
impact code correctness, it does impact maintainability (it would be
nice to use one common order among all four locations, rather than 3
different orders).  Arguably, some of it is pre-existing, so I won't
require a respin, but it is worth considering.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]