Re: [Qemu-devel] [PATCH V2 2/4] qemu-nbd: support internal snapshot export

[Wenchao Xia]

于 2013/9/23 18:25, Paolo Bonzini 写道:
> Il 22/09/2013 11:39, Wenchao Xia ha scritto:
> > Now it is possible to directly export an internal snapshot, which
> > can be used to probe the snapshot's contents without qemu-img
> > convert.
> >
> > Signed-off-by: Wenchao Xia<address@hidden>
> > ---
> >   qemu-nbd.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 files changed, 53 insertions(+), 1 deletions(-)
> >
> > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > index c26c98e..e450d04 100644
> > --- a/qemu-nbd.c
> > +++ b/qemu-nbd.c
> > @@ -20,6 +20,7 @@
> >   #include "block/block.h"
> >   #include "block/nbd.h"
> >   #include "qemu/main-loop.h"
> > +#include "block/snapshot.h"
> >
> >   #include<stdarg.h>
> >   #include<stdio.h>
> > @@ -304,6 +305,23 @@ static void nbd_accept(void *opaque)
> >       }
> >   }
> >
> > +#define SNAPSHOT_OPT_ID         "id"
> > +#define SNAPSHOT_OPT_NAME       "name"
> > +
> > +static QEMUOptionParameter snapshot_options[] = {
> > +    {
> > +        .name = SNAPSHOT_OPT_ID,
> > +        .type = OPT_STRING,
> > +        .help = "snapshot id"
> > +    },
> > +    {
> > +        .name = SNAPSHOT_OPT_NAME,
> > +        .type = OPT_STRING,
> > +        .help = "snapshot name"
> > +    },
> > +    { NULL }
> > +};
> I think whatever mechanism you use here to pick a snapshot id or name
> should be implemented in qemu-img too.
qemu-img already pick up snapshot by mixed id and name, do you like to 
add a new
interface like the above(Keep old interface untouched for compatiablity)?

> Also, I think QEMUOptionParameter is being phased out.
Is QemuOptsList the recommanded method?

> >   int main(int argc, char **argv)
> >   {
> >       BlockDriverState *bs;
> > @@ -315,7 +333,10 @@ int main(int argc, char **argv)
> >       char *device = NULL;
> >       int port = NBD_DEFAULT_PORT;
> >       off_t fd_size;
> > -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:t";
> > +    QEMUOptionParameter *sn_param = NULL;
> > +    const QEMUOptionParameter *sn_param_id, *sn_param_name;
> > +    const char *sn_id = NULL, *sn_name = NULL;
> > +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
> >       struct option lopt[] = {
> >           { "help", 0, NULL, 'h' },
> >           { "version", 0, NULL, 'V' },
> > @@ -328,6 +349,7 @@ int main(int argc, char **argv)
> >           { "connect", 1, NULL, 'c' },
> >           { "disconnect", 0, NULL, 'd' },
> >           { "snapshot", 0, NULL, 's' },
> > +        { "snapshot-load", 1, NULL, 'l' },
> Please call this option "load-snapshot".
OK.


> Paolo
>
> >           { "nocache", 0, NULL, 'n' },
> >           { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
> >   #ifdef CONFIG_LINUX_AIO
> > @@ -428,6 +450,14 @@ int main(int argc, char **argv)
> >                   errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
> >               }
> >               break;
> > +        case 'l':
> > +            sn_param = parse_option_parameters(optarg,
> > +                                               snapshot_options, sn_param);
> > +            if (!sn_param) {
> > +                errx(EXIT_FAILURE,
> > +                     "Invalid snapshot-load options '%s'", optarg);
> > +            }
> > +            /* fall through */
> >           case 'r':
> >               nbdflags |= NBD_FLAG_READ_ONLY;
> >               flags&= ~BDRV_O_RDWR;
> > @@ -581,6 +611,24 @@ int main(int argc, char **argv)
> >               error_get_pretty(local_err));
> >       }
> >
> > +    if (sn_param) {
> > +        sn_param_id = get_option_parameter(sn_param, SNAPSHOT_OPT_ID);
> > +        sn_param_name = get_option_parameter(sn_param, SNAPSHOT_OPT_NAME);
> > +        if (sn_param_id) {
> > +            sn_id = sn_param_id->value.s;
> > +        }
> > +        if (sn_param_name) {
> > +            sn_name = sn_param_name->value.s;
> > +        }
> > +        ret = bdrv_snapshot_load_tmp(bs, sn_id, sn_name,&local_err);
> > +        if (ret<  0) {
> > +            errno = -ret;
> > +            err(EXIT_FAILURE,
> > +                "Failed to load snapshot, reason:\n%s",
> > +                error_get_pretty(local_err));
> > +        }
> > +    }
> > +
> >       fd_size = bdrv_getlength(bs);
> >
> >       if (partition != -1) {
> > @@ -641,6 +689,10 @@ int main(int argc, char **argv)
> >           unlink(sockpath);
> >       }
> >
> > +    if (sn_param) {
> > +        free_option_parameters(sn_param);
> > +    }
> > +
> >       if (device) {
> >           void *ret;
> >           pthread_join(client_thread,&ret);