Patchwork [V3,11/11] hmp: show snapshot on single block device

login
register
mail settings
Submitter Wayne Xia
Date Jan. 14, 2013, 7:09 a.m.
Message ID <1358147387-8221-12-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/211716/
State New
Headers show

Comments

Wayne Xia - Jan. 14, 2013, 7:09 a.m.
This patch use block layer API to qmp snapshot info on a block
device, then use the same code dumping vm snapshot info, to print
in monitor.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
Note:
  This patch need previous hmp extention patch which enable
info sub command take qdict * as paramter.

---
 monitor.c |    6 +++---
 savevm.c  |   43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 4 deletions(-)
Pavel Hrdina - Jan. 14, 2013, 11:15 a.m.
On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>   This patch use block layer API to qmp snapshot info on a block
> device, then use the same code dumping vm snapshot info, to print
> in monitor.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> Note:
>   This patch need previous hmp extention patch which enable
> info sub command take qdict * as paramter.

> diff --git a/savevm.c b/savevm.c
> index cabdcb6..cd474e9 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>      return;
>  }
>  
> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> +{
> +    Error *err = NULL;
> +    SnapshotInfoList *list;
> +    BlockDriverState *bs;
> +
> +    /* find the target bs */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> +        return ;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> +        return ;
> +    }
> +
> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> +    if (error_is_set(&err)) {
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }
> +
> +    if (list == NULL) {
> +        monitor_printf(mon, "There is no snapshot available.\n");
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Device '%s':\n", device);
> +    monitor_dump_snapshotinfolist(mon, list);
> +    qapi_free_SnapshotInfoList(list);
> +    return;
> +}
> +
>  void do_info_snapshots(Monitor *mon, const QDict *qdict)
>  {
> -    do_info_snapshots_vm(mon);
> +    const char *device = qdict_get_try_str(qdict, "device");
> +    if (!device) {
> +        do_info_snapshots_vm(mon);
> +    } else {
> +        do_info_snapshots_blk(mon, device);
> +    }
> +    return;
>  }
>  

I think that you should move these functions into hmp.c file. This also
applies to previous patch and according to this changes you don't have
to export hmp_handle_error() function which should be used only in hmp.c

Pavel
Luiz Capitulino - Jan. 14, 2013, 6:56 p.m.
On Mon, 14 Jan 2013 15:09:47 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This patch use block layer API to qmp snapshot info on a block
> device, then use the same code dumping vm snapshot info, to print
> in monitor.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> Note:
>   This patch need previous hmp extention patch which enable
> info sub command take qdict * as paramter.
> 
> ---
>  monitor.c |    6 +++---
>  savevm.c  |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d186532..cba75a2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2583,9 +2583,9 @@ static mon_cmd_t info_cmds[] = {
>      },
>      {
>          .name       = "snapshots",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show the currently saved VM snapshots",
> +        .args_type  = "device:B?",
> +        .params     = "[device]",
> +        .help       = "show snapshots of whole vm or a single device",
>          .mhandler.cmd = do_info_snapshots,
>      },
>      {
> diff --git a/savevm.c b/savevm.c
> index cabdcb6..cd474e9 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>      return;
>  }
>  
> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> +{
> +    Error *err = NULL;
> +    SnapshotInfoList *list;
> +    BlockDriverState *bs;
> +
> +    /* find the target bs */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> +        return ;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> +        return ;
> +    }
> +
> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> +    if (error_is_set(&err)) {
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }

We try to restrict hmp.c to use the QMP API only (see Pavel's comment on
moving this to hmp.c).

If this can't be implemented using qmp_query_snapshots(), then I'd suggest
to add this capability to qmp_query_snapshots() or add a new QMP command.

> +
> +    if (list == NULL) {
> +        monitor_printf(mon, "There is no snapshot available.\n");
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Device '%s':\n", device);
> +    monitor_dump_snapshotinfolist(mon, list);
> +    qapi_free_SnapshotInfoList(list);
> +    return;
> +}
> +
>  void do_info_snapshots(Monitor *mon, const QDict *qdict)
>  {
> -    do_info_snapshots_vm(mon);
> +    const char *device = qdict_get_try_str(qdict, "device");
> +    if (!device) {
> +        do_info_snapshots_vm(mon);
> +    } else {
> +        do_info_snapshots_blk(mon, device);
> +    }
> +    return;
>  }
>  
>  void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
Luiz Capitulino - Jan. 14, 2013, 6:56 p.m.
On Mon, 14 Jan 2013 12:15:37 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:

> On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
> >   This patch use block layer API to qmp snapshot info on a block
> > device, then use the same code dumping vm snapshot info, to print
> > in monitor.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > ---
> > Note:
> >   This patch need previous hmp extention patch which enable
> > info sub command take qdict * as paramter.
> 
> > diff --git a/savevm.c b/savevm.c
> > index cabdcb6..cd474e9 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
> >      return;
> >  }
> >  
> > +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> > +{
> > +    Error *err = NULL;
> > +    SnapshotInfoList *list;
> > +    BlockDriverState *bs;
> > +
> > +    /* find the target bs */
> > +    bs = bdrv_find(device);
> > +    if (!bs) {
> > +        monitor_printf(mon, "Device '%s' not found.\n", device);
> > +        return ;
> > +    }
> > +
> > +    if (!bdrv_can_snapshot(bs)) {
> > +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> > +        return ;
> > +    }
> > +
> > +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> > +    if (error_is_set(&err)) {
> > +        hmp_handle_error(mon, &err);
> > +        return;
> > +    }
> > +
> > +    if (list == NULL) {
> > +        monitor_printf(mon, "There is no snapshot available.\n");
> > +        return;
> > +    }
> > +
> > +    monitor_printf(mon, "Device '%s':\n", device);
> > +    monitor_dump_snapshotinfolist(mon, list);
> > +    qapi_free_SnapshotInfoList(list);
> > +    return;
> > +}
> > +
> >  void do_info_snapshots(Monitor *mon, const QDict *qdict)
> >  {
> > -    do_info_snapshots_vm(mon);
> > +    const char *device = qdict_get_try_str(qdict, "device");
> > +    if (!device) {
> > +        do_info_snapshots_vm(mon);
> > +    } else {
> > +        do_info_snapshots_blk(mon, device);
> > +    }
> > +    return;
> >  }
> >  
> 
> I think that you should move these functions into hmp.c file. This also
> applies to previous patch and according to this changes you don't have
> to export hmp_handle_error() function which should be used only in hmp.c

Exactly, I was going to say the same thing. Also note that you'll need
better names for do_info_snapshots_vm() and do_info_snapshots_blk() if
you make them public (or you could move them to hmp.c as well).
Wayne Xia - Jan. 15, 2013, 2:36 a.m.
> On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>>    This patch use block layer API to qmp snapshot info on a block
>> device, then use the same code dumping vm snapshot info, to print
>> in monitor.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> Note:
>>    This patch need previous hmp extention patch which enable
>> info sub command take qdict * as paramter.
>
>> diff --git a/savevm.c b/savevm.c
>> index cabdcb6..cd474e9 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>>       return;
>>   }
>>
>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>> +{
>> +    Error *err = NULL;
>> +    SnapshotInfoList *list;
>> +    BlockDriverState *bs;
>> +
>> +    /* find the target bs */
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>> +        return ;
>> +    }
>> +
>> +    if (!bdrv_can_snapshot(bs)) {
>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>> +        return ;
>> +    }
>> +
>> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
>> +    if (error_is_set(&err)) {
>> +        hmp_handle_error(mon, &err);
>> +        return;
>> +    }
>> +
>> +    if (list == NULL) {
>> +        monitor_printf(mon, "There is no snapshot available.\n");
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "Device '%s':\n", device);
>> +    monitor_dump_snapshotinfolist(mon, list);
>> +    qapi_free_SnapshotInfoList(list);
>> +    return;
>> +}
>> +
>>   void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>   {
>> -    do_info_snapshots_vm(mon);
>> +    const char *device = qdict_get_try_str(qdict, "device");
>> +    if (!device) {
>> +        do_info_snapshots_vm(mon);
>> +    } else {
>> +        do_info_snapshots_blk(mon, device);
>> +    }
>> +    return;
>>   }
>>
>
> I think that you should move these functions into hmp.c file. This also
> applies to previous patch and according to this changes you don't have
> to export hmp_handle_error() function which should be used only in hmp.c
>
> Pavel
>
   It seems there are other "do_info_**" function in other .c files,
so I suggest a different serial later which move those functions, if
necessary.
Luiz Capitulino - Jan. 15, 2013, 11:05 a.m.
On Tue, 15 Jan 2013 10:36:22 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>  > On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
> >>    This patch use block layer API to qmp snapshot info on a block
> >> device, then use the same code dumping vm snapshot info, to print
> >> in monitor.
> >>
> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >> ---
> >> Note:
> >>    This patch need previous hmp extention patch which enable
> >> info sub command take qdict * as paramter.
> >
> >> diff --git a/savevm.c b/savevm.c
> >> index cabdcb6..cd474e9 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
> >>       return;
> >>   }
> >>
> >> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> >> +{
> >> +    Error *err = NULL;
> >> +    SnapshotInfoList *list;
> >> +    BlockDriverState *bs;
> >> +
> >> +    /* find the target bs */
> >> +    bs = bdrv_find(device);
> >> +    if (!bs) {
> >> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> >> +        return ;
> >> +    }
> >> +
> >> +    if (!bdrv_can_snapshot(bs)) {
> >> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> >> +        return ;
> >> +    }
> >> +
> >> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> >> +    if (error_is_set(&err)) {
> >> +        hmp_handle_error(mon, &err);
> >> +        return;
> >> +    }
> >> +
> >> +    if (list == NULL) {
> >> +        monitor_printf(mon, "There is no snapshot available.\n");
> >> +        return;
> >> +    }
> >> +
> >> +    monitor_printf(mon, "Device '%s':\n", device);
> >> +    monitor_dump_snapshotinfolist(mon, list);
> >> +    qapi_free_SnapshotInfoList(list);
> >> +    return;
> >> +}
> >> +
> >>   void do_info_snapshots(Monitor *mon, const QDict *qdict)
> >>   {
> >> -    do_info_snapshots_vm(mon);
> >> +    const char *device = qdict_get_try_str(qdict, "device");
> >> +    if (!device) {
> >> +        do_info_snapshots_vm(mon);
> >> +    } else {
> >> +        do_info_snapshots_blk(mon, device);
> >> +    }
> >> +    return;
> >>   }
> >>
> >
> > I think that you should move these functions into hmp.c file. This also
> > applies to previous patch and according to this changes you don't have
> > to export hmp_handle_error() function which should be used only in hmp.c
> >
> > Pavel
> >
>    It seems there are other "do_info_**" function in other .c files,
> so I suggest a different serial later which move those functions, if
> necessary.

The other functions are probably old hmp code. That is, code that is not
converted to make QMP calls. Generally, new hmp code goes into hmp.c unless
there's a good reason to put them in a different file.

So, please move it to hmp.c.
Wayne Xia - Jan. 16, 2013, 2:51 a.m.
于 2013-1-15 19:05, Luiz Capitulino 写道:
> On Tue, 15 Jan 2013 10:36:22 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>   > On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>>>>     This patch use block layer API to qmp snapshot info on a block
>>>> device, then use the same code dumping vm snapshot info, to print
>>>> in monitor.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>> Note:
>>>>     This patch need previous hmp extention patch which enable
>>>> info sub command take qdict * as paramter.
>>>
>>>> diff --git a/savevm.c b/savevm.c
>>>> index cabdcb6..cd474e9 100644
>>>> --- a/savevm.c
>>>> +++ b/savevm.c
>>>> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>>>>        return;
>>>>    }
>>>>
>>>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>>>> +{
>>>> +    Error *err = NULL;
>>>> +    SnapshotInfoList *list;
>>>> +    BlockDriverState *bs;
>>>> +
>>>> +    /* find the target bs */
>>>> +    bs = bdrv_find(device);
>>>> +    if (!bs) {
>>>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>>>> +        return ;
>>>> +    }
>>>> +
>>>> +    if (!bdrv_can_snapshot(bs)) {
>>>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>>>> +        return ;
>>>> +    }
>>>> +
>>>> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
>>>> +    if (error_is_set(&err)) {
>>>> +        hmp_handle_error(mon, &err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (list == NULL) {
>>>> +        monitor_printf(mon, "There is no snapshot available.\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    monitor_printf(mon, "Device '%s':\n", device);
>>>> +    monitor_dump_snapshotinfolist(mon, list);
>>>> +    qapi_free_SnapshotInfoList(list);
>>>> +    return;
>>>> +}
>>>> +
>>>>    void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>>>    {
>>>> -    do_info_snapshots_vm(mon);
>>>> +    const char *device = qdict_get_try_str(qdict, "device");
>>>> +    if (!device) {
>>>> +        do_info_snapshots_vm(mon);
>>>> +    } else {
>>>> +        do_info_snapshots_blk(mon, device);
>>>> +    }
>>>> +    return;
>>>>    }
>>>>
>>>
>>> I think that you should move these functions into hmp.c file. This also
>>> applies to previous patch and according to this changes you don't have
>>> to export hmp_handle_error() function which should be used only in hmp.c
>>>
>>> Pavel
>>>
>>     It seems there are other "do_info_**" function in other .c files,
>> so I suggest a different serial later which move those functions, if
>> necessary.
>
> The other functions are probably old hmp code. That is, code that is not
> converted to make QMP calls. Generally, new hmp code goes into hmp.c unless
> there's a good reason to put them in a different file.
>
> So, please move it to hmp.c.
>

OK, new hmp function will goto hmp.c

Patch

diff --git a/monitor.c b/monitor.c
index d186532..cba75a2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2583,9 +2583,9 @@  static mon_cmd_t info_cmds[] = {
     },
     {
         .name       = "snapshots",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show the currently saved VM snapshots",
+        .args_type  = "device:B?",
+        .params     = "[device]",
+        .help       = "show snapshots of whole vm or a single device",
         .mhandler.cmd = do_info_snapshots,
     },
     {
diff --git a/savevm.c b/savevm.c
index cabdcb6..cd474e9 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2354,9 +2354,50 @@  static void do_info_snapshots_vm(Monitor *mon)
     return;
 }
 
+static void do_info_snapshots_blk(Monitor *mon, const char *device)
+{
+    Error *err = NULL;
+    SnapshotInfoList *list;
+    BlockDriverState *bs;
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        monitor_printf(mon, "Device '%s' not found.\n", device);
+        return ;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
+        return ;
+    }
+
+    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
+    if (error_is_set(&err)) {
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    if (list == NULL) {
+        monitor_printf(mon, "There is no snapshot available.\n");
+        return;
+    }
+
+    monitor_printf(mon, "Device '%s':\n", device);
+    monitor_dump_snapshotinfolist(mon, list);
+    qapi_free_SnapshotInfoList(list);
+    return;
+}
+
 void do_info_snapshots(Monitor *mon, const QDict *qdict)
 {
-    do_info_snapshots_vm(mon);
+    const char *device = qdict_get_try_str(qdict, "device");
+    if (!device) {
+        do_info_snapshots_vm(mon);
+    } else {
+        do_info_snapshots_blk(mon, device);
+    }
+    return;
 }
 
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)