| Message ID | 1299685061-24234-1-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/09/2011 09:37 AM, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen<Jes.Sorensen@redhat.com> > > Add QMP bits for snapshot_blkdev_sync command. This is the same as > snapshot_blkdev in the human monitor, but added _sync to the name to > make it explicit that the command is synchronous and leave space for a > future async version. > > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> > --- > qmp-commands.hx | 19 +++++++++++++++++++ > 1 files changed, 19 insertions(+), 0 deletions(-) > > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 9d3cc31..e32187e 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -667,6 +667,25 @@ Example: > EQMP > > { > + .name = "snapshot_blkdev_sync", > + .args_type = "device:B,snapshot_file:s?,format:s?", > + .params = "device [new-image-file] [format]", > + .help = "initiates a live snapshot\n\t\t\t" > + "of device. If a new image file is specified, the\n\t\t\t" > + "new image file will become the new root image.\n\t\t\t" > + "If format is specified, the snapshot file will\n\t\t\t" > + "be created in that format. Otherwise the\n\t\t\t" > + "snapshot will be internal! (currently unsupported)", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_snapshot_blkdev, > + }, > + > +SQMP > +Synchronous snapshot of block device, using snapshot file as target > +if provided. Please document the error semantics. The documentation in .help is discarded for QMP. You should put the docs in the SQMP section. Also, QMP should use '-' instead of '_'. We should also try to follow the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync' I'm not sure blkdev is the right prefix. Kevin, what are your thoughts here? Does 'blkdev' make sense for any command operating on a block device (that is, a qdev device that happens to have a block drive, not the same thing as -blockdev that we've discussed in the past). Regards, Anthony Liguori > +EQMP > + > + { > .name = "balloon", > .args_type = "value:M", > .params = "target",
Am 09.03.2011 16:46, schrieb Anthony Liguori: > On 03/09/2011 09:37 AM, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen<Jes.Sorensen@redhat.com> >> >> Add QMP bits for snapshot_blkdev_sync command. This is the same as >> snapshot_blkdev in the human monitor, but added _sync to the name to >> make it explicit that the command is synchronous and leave space for a >> future async version. >> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >> --- >> qmp-commands.hx | 19 +++++++++++++++++++ >> 1 files changed, 19 insertions(+), 0 deletions(-) >> >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 9d3cc31..e32187e 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -667,6 +667,25 @@ Example: >> EQMP >> >> { >> + .name = "snapshot_blkdev_sync", >> + .args_type = "device:B,snapshot_file:s?,format:s?", >> + .params = "device [new-image-file] [format]", >> + .help = "initiates a live snapshot\n\t\t\t" >> + "of device. If a new image file is specified, the\n\t\t\t" >> + "new image file will become the new root image.\n\t\t\t" >> + "If format is specified, the snapshot file will\n\t\t\t" >> + "be created in that format. Otherwise the\n\t\t\t" >> + "snapshot will be internal! (currently unsupported)", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_snapshot_blkdev, >> + }, >> + >> +SQMP >> +Synchronous snapshot of block device, using snapshot file as target >> +if provided. > > Please document the error semantics. > > The documentation in .help is discarded for QMP. You should put the > docs in the SQMP section. > > Also, QMP should use '-' instead of '_'. We should also try to follow > the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync' > > I'm not sure blkdev is the right prefix. Kevin, what are your thoughts > here? Does 'blkdev' make sense for any command operating on a block > device (that is, a qdev device that happens to have a block drive, not > the same thing as -blockdev that we've discussed in the past). Doesn't this command work on a -blockdev style thing, i.e. BlockDriverState or DriveInfo? I don't think we have any commands that refer to qdev devices that happen to be block devices. You could probably argue that some of them should... Kevin
On 03/09/2011 10:01 AM, Kevin Wolf wrote: > Am 09.03.2011 16:46, schrieb Anthony Liguori: >> On 03/09/2011 09:37 AM, Jes.Sorensen@redhat.com wrote: >>> From: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> Add QMP bits for snapshot_blkdev_sync command. This is the same as >>> snapshot_blkdev in the human monitor, but added _sync to the name to >>> make it explicit that the command is synchronous and leave space for a >>> future async version. >>> >>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >>> --- >>> qmp-commands.hx | 19 +++++++++++++++++++ >>> 1 files changed, 19 insertions(+), 0 deletions(-) >>> >>> diff --git a/qmp-commands.hx b/qmp-commands.hx >>> index 9d3cc31..e32187e 100644 >>> --- a/qmp-commands.hx >>> +++ b/qmp-commands.hx >>> @@ -667,6 +667,25 @@ Example: >>> EQMP >>> >>> { >>> + .name = "snapshot_blkdev_sync", >>> + .args_type = "device:B,snapshot_file:s?,format:s?", >>> + .params = "device [new-image-file] [format]", >>> + .help = "initiates a live snapshot\n\t\t\t" >>> + "of device. If a new image file is specified, the\n\t\t\t" >>> + "new image file will become the new root image.\n\t\t\t" >>> + "If format is specified, the snapshot file will\n\t\t\t" >>> + "be created in that format. Otherwise the\n\t\t\t" >>> + "snapshot will be internal! (currently unsupported)", >>> + .user_print = monitor_user_noop, >>> + .mhandler.cmd_new = do_snapshot_blkdev, >>> + }, >>> + >>> +SQMP >>> +Synchronous snapshot of block device, using snapshot file as target >>> +if provided. >> Please document the error semantics. >> >> The documentation in .help is discarded for QMP. You should put the >> docs in the SQMP section. >> >> Also, QMP should use '-' instead of '_'. We should also try to follow >> the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync' >> >> I'm not sure blkdev is the right prefix. Kevin, what are your thoughts >> here? Does 'blkdev' make sense for any command operating on a block >> device (that is, a qdev device that happens to have a block drive, not >> the same thing as -blockdev that we've discussed in the past). > Doesn't this command work on a -blockdev style thing, i.e. > BlockDriverState or DriveInfo? I don't think we have any commands that > refer to qdev devices that happen to be block devices. You could > probably argue that some of them should... 'device' is a device name though, right? Or is it a name associated with a BlockDriverState that currently happens to be a qdev name? Would I be able to eventually pass a qdev path here? If the answer is that this is a bdrv_name, then should we at least use blockdev instead of blkdev? Regards, Anthony Liguori > Kevin >
On 03/09/11 18:03, Anthony Liguori wrote: > On 03/09/2011 10:01 AM, Kevin Wolf wrote: >>> I'm not sure blkdev is the right prefix. Kevin, what are your thoughts >>> here? Does 'blkdev' make sense for any command operating on a block >>> device (that is, a qdev device that happens to have a block drive, not >>> the same thing as -blockdev that we've discussed in the past). >> Doesn't this command work on a -blockdev style thing, i.e. >> BlockDriverState or DriveInfo? I don't think we have any commands that >> refer to qdev devices that happen to be block devices. You could >> probably argue that some of them should... > > 'device' is a device name though, right? Or is it a name associated > with a BlockDriverState that currently happens to be a qdev name? It is a device in the sense of what you get from 'info block' in the human monitor. > Would I be able to eventually pass a qdev path here? > > If the answer is that this is a bdrv_name, then should we at least use > blockdev instead of blkdev? I used blkdev in the human monitor command to reduce the length of the command name. Anyway, if you can decide what you want to call the command, I'll rename it accordingly. Cheers, Jes
Am 09.03.2011 18:03, schrieb Anthony Liguori: > On 03/09/2011 10:01 AM, Kevin Wolf wrote: >> Am 09.03.2011 16:46, schrieb Anthony Liguori: >>> On 03/09/2011 09:37 AM, Jes.Sorensen@redhat.com wrote: >>>> From: Jes Sorensen<Jes.Sorensen@redhat.com> >>>> >>>> Add QMP bits for snapshot_blkdev_sync command. This is the same as >>>> snapshot_blkdev in the human monitor, but added _sync to the name to >>>> make it explicit that the command is synchronous and leave space for a >>>> future async version. >>>> >>>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >>>> --- >>>> qmp-commands.hx | 19 +++++++++++++++++++ >>>> 1 files changed, 19 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/qmp-commands.hx b/qmp-commands.hx >>>> index 9d3cc31..e32187e 100644 >>>> --- a/qmp-commands.hx >>>> +++ b/qmp-commands.hx >>>> @@ -667,6 +667,25 @@ Example: >>>> EQMP >>>> >>>> { >>>> + .name = "snapshot_blkdev_sync", >>>> + .args_type = "device:B,snapshot_file:s?,format:s?", >>>> + .params = "device [new-image-file] [format]", >>>> + .help = "initiates a live snapshot\n\t\t\t" >>>> + "of device. If a new image file is specified, the\n\t\t\t" >>>> + "new image file will become the new root image.\n\t\t\t" >>>> + "If format is specified, the snapshot file will\n\t\t\t" >>>> + "be created in that format. Otherwise the\n\t\t\t" >>>> + "snapshot will be internal! (currently unsupported)", >>>> + .user_print = monitor_user_noop, >>>> + .mhandler.cmd_new = do_snapshot_blkdev, >>>> + }, >>>> + >>>> +SQMP >>>> +Synchronous snapshot of block device, using snapshot file as target >>>> +if provided. >>> Please document the error semantics. >>> >>> The documentation in .help is discarded for QMP. You should put the >>> docs in the SQMP section. >>> >>> Also, QMP should use '-' instead of '_'. We should also try to follow >>> the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync' >>> >>> I'm not sure blkdev is the right prefix. Kevin, what are your thoughts >>> here? Does 'blkdev' make sense for any command operating on a block >>> device (that is, a qdev device that happens to have a block drive, not >>> the same thing as -blockdev that we've discussed in the past). >> Doesn't this command work on a -blockdev style thing, i.e. >> BlockDriverState or DriveInfo? I don't think we have any commands that >> refer to qdev devices that happen to be block devices. You could >> probably argue that some of them should... > > 'device' is a device name though, right? Or is it a name associated > with a BlockDriverState that currently happens to be a qdev name? > > Would I be able to eventually pass a qdev path here? > > If the answer is that this is a bdrv_name, then should we at least use > blockdev instead of blkdev? I think it's a drive name, like 'ide-hd0' or what they were called. I'm not completely sure if this is the same namespace as blockdev_add would use, but at first sight it would make some sense. Markus? Kevin
diff --git a/qmp-commands.hx b/qmp-commands.hx index 9d3cc31..e32187e 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -667,6 +667,25 @@ Example: EQMP { + .name = "snapshot_blkdev_sync", + .args_type = "device:B,snapshot_file:s?,format:s?", + .params = "device [new-image-file] [format]", + .help = "initiates a live snapshot\n\t\t\t" + "of device. If a new image file is specified, the\n\t\t\t" + "new image file will become the new root image.\n\t\t\t" + "If format is specified, the snapshot file will\n\t\t\t" + "be created in that format. Otherwise the\n\t\t\t" + "snapshot will be internal! (currently unsupported)", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_snapshot_blkdev, + }, + +SQMP +Synchronous snapshot of block device, using snapshot file as target +if provided. +EQMP + + { .name = "balloon", .args_type = "value:M", .params = "target",