| Message ID | 20180321121940.39426-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD export bitmaps | expand |
On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block.json | 27 +++++++++++++++++++++++++++ > blockdev-nbd.c | 23 +++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/qapi/block.json b/qapi/block.json > index c694524002..4afbbcd7b7 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -269,6 +269,33 @@ > 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} } > > ## > +# @nbd-server-add-bitmap: > +# > +# Export dirty bitmap through selected export. Bitmaps are searched for in > +# device attached to the export and in all its backings. Exported bitmap > +# is locked until NBD export is removed. Expose a dirty bitmap associated with the selected export. The bitmap search starts at the device attached to the export, and includes all backing files. The exported bitmap is then locked until the NBD export is removed. > +# > +# @name: Export name. > +# > +# @bitmap: Bitmap name to search. s/search./search for./ > +# > +# @bitmap-export-name: How the bitmap will be seen by nbd clients > +# (default @bitmap) Maybe mention that the client must use NBD_OPT_SET_META_CONTEXT with a query of "qemu-dirty-bitmap:NAME" (where NAME matches bitmap-export-name) to access the exposed bitmap. (May need to be adjusted by my suggestion to use just the namespace "qemu:") > +# > +# > +# Returns: error on one of the following conditions: > +# - the server is not running > +# - export is not found > +# - bitmap is not found > +# - bitmap is disabled > +# - bitmap is locked Do we really need to list all the error conditions? My worry is that a list this specific might go stale, compared to the obvious default that the command succeeds only if it was able to expose the bitmap and that the error message is specific enough for a human to figure out what to fix if it failed. > +# > +# Since: 2.13 > +## > + { 'command': 'nbd-server-add-bitmap', > + 'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} } > +
21.03.2018 20:33, Eric Blake wrote: > On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block.json | 27 +++++++++++++++++++++++++++ >> blockdev-nbd.c | 23 +++++++++++++++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/qapi/block.json b/qapi/block.json >> index c694524002..4afbbcd7b7 100644 >> --- a/qapi/block.json >> +++ b/qapi/block.json >> @@ -269,6 +269,33 @@ >> 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} } >> ## >> +# @nbd-server-add-bitmap: >> +# >> +# Export dirty bitmap through selected export. Bitmaps are searched >> for in >> +# device attached to the export and in all its backings. Exported >> bitmap >> +# is locked until NBD export is removed. > > Expose a dirty bitmap associated with the selected export. The bitmap > search starts at the device attached to the export, and includes all > backing files. The exported bitmap is then locked until the NBD > export is removed. > >> +# >> +# @name: Export name. >> +# >> +# @bitmap: Bitmap name to search. > > s/search./search for./ > >> +# >> +# @bitmap-export-name: How the bitmap will be seen by nbd clients >> +# (default @bitmap) > > Maybe mention that the client must use NBD_OPT_SET_META_CONTEXT with a > query of "qemu-dirty-bitmap:NAME" (where NAME matches > bitmap-export-name) to access the exposed bitmap. (May need to be > adjusted by my suggestion to use just the namespace "qemu:") ok > >> +# >> +# >> +# Returns: error on one of the following conditions: >> +# - the server is not running >> +# - export is not found >> +# - bitmap is not found >> +# - bitmap is disabled >> +# - bitmap is locked > > Do we really need to list all the error conditions? My worry is that > a list this specific might go stale, compared to the obvious default > that the command succeeds only if it was able to expose the bitmap and > that the error message is specific enough for a human to figure out > what to fix if it failed. Hmm, I've just doing it similar with other commands in the file. Is there any requirements on this part of qapi documentation? I can write only "# Returns: nothing on success" line, is it appropriate? blockdev-mirror do so, but other commands tries to describe errors. Looks like we lack some specified format for return code description like we have for parameters.. > >> +# >> +# Since: 2.13 >> +## >> + { 'command': 'nbd-server-add-bitmap', >> + 'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': >> 'str'} } >> + >
On 03/22/2018 10:43 AM, Vladimir Sementsov-Ogievskiy wrote: >>> +# Returns: error on one of the following conditions: >>> +# - the server is not running >>> +# - export is not found >>> +# - bitmap is not found >>> +# - bitmap is disabled >>> +# - bitmap is locked >> >> Do we really need to list all the error conditions? My worry is that >> a list this specific might go stale, compared to the obvious default >> that the command succeeds only if it was able to expose the bitmap and >> that the error message is specific enough for a human to figure out >> what to fix if it failed. > > > Hmm, I've just doing it similar with other commands in the file. Is > there any requirements on this part of qapi documentation? I can write > only "# Returns: nothing on success" line, is it appropriate? > blockdev-mirror do so, but other commands tries to describe errors. > Looks like we lack some specified format for return code description > like we have for parameters.. Yeah, for returns, it's been very ad hoc. My personal feel (although it's not very well documented and certainly not enforced): all commands can reasonably return errors, presumably for a good reason; but exhaustively auditing WHICH errors is a huge task with little benefits. A few commands return non-generic errors, but if all error paths used error_setg(), there's nothing that a machine can do to tell the difference between the errors, so documenting different error reasons doesn't add much. Thus, if a command returns nothing on success, I'm fine with omitting 'Returns:' entirely, and the doc generator permits that. But if you have bothered to list Returns: for certain errors, I'm not going to blindly throw away the documentation work, even though the list may become incomplete over time.
22.03.2018 19:19, Eric Blake wrote: > On 03/22/2018 10:43 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> +# Returns: error on one of the following conditions: >>>> +# - the server is not running >>>> +# - export is not found >>>> +# - bitmap is not found >>>> +# - bitmap is disabled >>>> +# - bitmap is locked >>> >>> Do we really need to list all the error conditions? My worry is >>> that a list this specific might go stale, compared to the obvious >>> default that the command succeeds only if it was able to expose the >>> bitmap and that the error message is specific enough for a human to >>> figure out what to fix if it failed. >> >> >> Hmm, I've just doing it similar with other commands in the file. Is >> there any requirements on this part of qapi documentation? I can >> write only "# Returns: nothing on success" line, is it appropriate? >> blockdev-mirror do so, but other commands tries to describe errors. >> Looks like we lack some specified format for return code description >> like we have for parameters.. > > Yeah, for returns, it's been very ad hoc. My personal feel (although > it's not very well documented and certainly not enforced): all > commands can reasonably return errors, presumably for a good reason; > but exhaustively auditing WHICH errors is a huge task with little > benefits. A few commands return non-generic errors, but if all error > paths used error_setg(), there's nothing that a machine can do to tell > the difference between the errors, so documenting different error > reasons doesn't add much. > > Thus, if a command returns nothing on success, I'm fine with omitting > 'Returns:' entirely, and the doc generator permits that. But if you > have bothered to list Returns: for certain errors, I'm not going to > blindly throw away the documentation work, even though the list may > become incomplete over time. > So, only something interesting worth documenting. Hmm, interesting: consider for example bloc-dirty-bitmap-remove. It says: # Returns: nothing on success # If @node is not a valid block device or node, DeviceNotFound But the code uses bdrv_lookup_bs, which uses simple error_setg, so it will return GenericError, isn't it? And, it's not the only place..
diff --git a/qapi/block.json b/qapi/block.json index c694524002..4afbbcd7b7 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -269,6 +269,33 @@ 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} } ## +# @nbd-server-add-bitmap: +# +# Export dirty bitmap through selected export. Bitmaps are searched for in +# device attached to the export and in all its backings. Exported bitmap +# is locked until NBD export is removed. +# +# @name: Export name. +# +# @bitmap: Bitmap name to search. +# +# @bitmap-export-name: How the bitmap will be seen by nbd clients +# (default @bitmap) +# +# +# Returns: error on one of the following conditions: +# - the server is not running +# - export is not found +# - bitmap is not found +# - bitmap is disabled +# - bitmap is locked +# +# Since: 2.13 +## + { 'command': 'nbd-server-add-bitmap', + 'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} } + +## # @nbd-server-stop: # # Stop QEMU's embedded NBD server, and unregister all devices previously diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 65a84739ed..6b0c50732c 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -220,3 +220,26 @@ void qmp_nbd_server_stop(Error **errp) nbd_server_free(nbd_server); nbd_server = NULL; } + +void qmp_nbd_server_add_bitmap(const char *name, const char *bitmap, + bool has_bitmap_export_name, + const char *bitmap_export_name, + Error **errp) +{ + NBDExport *exp; + + if (!nbd_server) { + error_setg(errp, "NBD server not running"); + return; + } + + exp = nbd_export_find(name); + if (exp == NULL) { + error_setg(errp, "Export '%s' is not found", name); + return; + } + + nbd_export_bitmap(exp, bitmap, + has_bitmap_export_name ? bitmap_export_name : bitmap, + errp); +}
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block.json | 27 +++++++++++++++++++++++++++ blockdev-nbd.c | 23 +++++++++++++++++++++++ 2 files changed, 50 insertions(+)