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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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<xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   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);
>>>
>>
>
>