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

[Wenchao Xia]

于 2013/10/10 14:00, Wenchao Xia 写道:
> 于 2013/10/2 0:08, Paolo Bonzini 写道:
> > Il 26/09/2013 02:16, 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>
> > > ---
> > >   block/snapshot.c         |   18 ++++++++++++++++++
> > >   include/block/snapshot.h |    6 ++++++
> > >   qemu-nbd.c               |   35 ++++++++++++++++++++++++++++++++++-
> > >   3 files changed, 58 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > index 2ae3099..b371c27 100644
> > > --- a/block/snapshot.c
> > > +++ b/block/snapshot.c
> > > @@ -25,6 +25,24 @@
> > >   #include "block/snapshot.h"
> > >   #include "block/block_int.h"
> > >
> > > +QemuOptsList internal_snapshot_opts = {
> > > +    .name = "snapshot",
> > > +    .head = QTAILQ_HEAD_INITIALIZER(internal_snapshot_opts.head),
> > > +    .desc = {
> > > +        {
> > > +            .name = SNAPSHOT_OPT_ID,
> > Why not just use "id" and "name"?
> Later it is used by code:
> qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> The macro is used to avoid type it twice in the codes, shouldn't it be 
> used?
>
> Another reason not using "id" is because string "id" is treated as 
> special case
> in opts_parse() so I choosed string "snapshot.id".
>
> > > +            .type = QEMU_OPT_STRING,
> > > +            .help = "snapshot id"
> > > +        },{
> > > +            .name = SNAPSHOT_OPT_NAME,
> > > +            .type = QEMU_OPT_STRING,
> > > +            .help = "snapshot name"
> > > +        },{
> > > +            /* end of list */
> > > +        }
> > > +    },
> > > +};
> > > +
> > >   int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo 
> > > *sn_info,
> > >                          const char *name)
> > >   {
> > > diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> > > index d05bea7..c524a49 100644
> > > --- a/include/block/snapshot.h
> > > +++ b/include/block/snapshot.h
> > > @@ -27,6 +27,12 @@
> > >
> > >   #include "qemu-common.h"
> > >   #include "qapi/error.h"
> > > +#include "qemu/option.h"
> > > +
> > > +#define SNAPSHOT_OPT_ID         "snapshot.id"
> > > +#define SNAPSHOT_OPT_NAME       "snapshot.name"
> > > +
> > > +extern QemuOptsList internal_snapshot_opts;
> > >
> > >   typedef struct QEMUSnapshotInfo {
> > >       char id_str[128]; /* unique snapshot id */
> > > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > > index c26c98e..6588a1f 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>
> > > @@ -315,7 +316,9 @@ 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";
> > > +    QemuOpts *sn_opts = NULL;
> > > +    const char *sn_id_or_name = NULL;
> > > +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:L:";
> > >       struct option lopt[] = {
> > >           { "help", 0, NULL, 'h' },
> > >           { "version", 0, NULL, 'V' },
> > > @@ -328,6 +331,8 @@ int main(int argc, char **argv)
> > >           { "connect", 1, NULL, 'c' },
> > >           { "disconnect", 0, NULL, 'd' },
> > >           { "snapshot", 0, NULL, 's' },
> > > +        { "load-snapshot", 1, NULL, 'l' },
> > Just omit the long option here...
> >
> > > +        { "load-snapshot1", 1, NULL, 'L' },
> > ... and call this "load-snapshot".
> >
> > Paolo
> OK, I will change as:
>
> { NULL, 1, NULL, 'l' },
>
> { "load-snapshot", 1, NULL, 'L' },
  From Eric's suggestion, I think simply one item:
{ "load-snapshot", 1, NULL, 'l' }
would be engough to handle both cases.

> > >           { "nocache", 0, NULL, 'n' },
> > >           { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
> > >   #ifdef CONFIG_LINUX_AIO
> > > @@ -428,6 +433,14 @@ int main(int argc, char **argv)
> > >                   errx(EXIT_FAILURE, "Offset must be positive `%s'", 
> > > optarg);
> > >               }
> > >               break;
> > > +        case 'l':
> > > +            sn_id_or_name = optarg;
> > > +            nbdflags |= NBD_FLAG_READ_ONLY;
> > > +            flags&= ~BDRV_O_RDWR;
> > > +            break;
> > > +        case 'L':
> > > +            sn_opts = qemu_opts_parse(&internal_snapshot_opts, 
> > > optarg, 0);
> > > +            /* fall through */
> > >           case 'r':
> > >               nbdflags |= NBD_FLAG_READ_ONLY;
> > >               flags&= ~BDRV_O_RDWR;
> > > @@ -581,6 +594,22 @@ int main(int argc, char **argv)
> > >               error_get_pretty(local_err));
> > >       }
> > >
> > > +    if (sn_opts) {
> > > +        ret = bdrv_snapshot_load_tmp(bs,
> > > +                                     qemu_opt_get(sn_opts, 
> > > SNAPSHOT_OPT_ID),
> > > +                                     qemu_opt_get(sn_opts, 
> > > SNAPSHOT_OPT_NAME),
> > > +&local_err);
> > > +    } else if (sn_id_or_name) {
> > > +        ret = bdrv_snapshot_load_tmp_by_id_or_name(bs, sn_id_or_name,
> > > +&local_err);
> > > +    }
> > > +    if (ret<  0) {
> > > +        errno = -ret;
> > > +        err(EXIT_FAILURE,
> > > +            "Failed to load snapshot: %s",
> > > +            error_get_pretty(local_err));
> > > +    }
> > > +
> > >       fd_size = bdrv_getlength(bs);
> > >
> > >       if (partition != -1) {
> > > @@ -641,6 +670,10 @@ int main(int argc, char **argv)
> > >           unlink(sockpath);
> > >       }
> > >
> > > +    if (sn_opts) {
> > > +        qemu_opts_del(sn_opts);
> > > +    }
> > > +
> > >       if (device) {
> > >           void *ret;
> > >           pthread_join(client_thread,&ret);