| Message ID | 1433759662-25139-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/08/2015 04:34 AM, Fam Zheng wrote: > The logic will be shared with qmp_drive_mirror. s/Extrace/Extract/ in the subject > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 26 ++++++++++++++++++++++++++ > blockdev.c | 14 ++------------ > include/block/block.h | 3 +++ > 3 files changed, 31 insertions(+), 12 deletions(-) > > + > + if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > + !(bdrv_flags & BDRV_O_UNMAP)) { > + error_setg(errp, "setting detect-zeroes to unmap is not allowed " > + "without setting discard operation to unmap"); > + } I think it might be better to have a tri-state enum, than to have two competing bools where only 3 of the 4 states are valid. We haven't yet committed to the 'unmap' bool, so we still have time to get the API right.
On 08/06/2015 16:17, Eric Blake wrote: >> > + >> > + if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && >> > + !(bdrv_flags & BDRV_O_UNMAP)) { >> > + error_setg(errp, "setting detect-zeroes to unmap is not allowed " >> > + "without setting discard operation to unmap"); >> > + } > I think it might be better to have a tri-state enum, than to have two > competing bools where only 3 of the 4 states are valid. Note that this is not a bool. We have one bool and one 3-element enum (off/on/unmap), where only 5 of the 6 states are valid. Also, at least detect-zeroes would go away if we had some kind of blockdev-mirror (where the target is added first with blockdev-add), so I think it's better to leave it separate. Paolo
On Mon, 06/08 16:53, Paolo Bonzini wrote: > > > On 08/06/2015 16:17, Eric Blake wrote: > >> > + > >> > + if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > >> > + !(bdrv_flags & BDRV_O_UNMAP)) { > >> > + error_setg(errp, "setting detect-zeroes to unmap is not allowed " > >> > + "without setting discard operation to unmap"); > >> > + } > > I think it might be better to have a tri-state enum, than to have two > > competing bools where only 3 of the 4 states are valid. > > Note that this is not a bool. We have one bool and one 3-element enum > (off/on/unmap), where only 5 of the 6 states are valid. Also, at least > detect-zeroes would go away if we had some kind of blockdev-mirror > (where the target is added first with blockdev-add), so I think it's > better to leave it separate. Agreed. But by then "drive-mirror derive=... detect-zeroes=unmap unmap=false" will be way too confusing. I say let's save that and just go for blockdev-add :) https://www.mail-archive.com/qemu-devel@nongnu.org/msg301990.html Fam
On Wed, 06/10 17:11, Fam Zheng wrote: > On Mon, 06/08 16:53, Paolo Bonzini wrote: > > > > > > On 08/06/2015 16:17, Eric Blake wrote: > > >> > + > > >> > + if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > > >> > + !(bdrv_flags & BDRV_O_UNMAP)) { > > >> > + error_setg(errp, "setting detect-zeroes to unmap is not allowed " > > >> > + "without setting discard operation to unmap"); > > >> > + } > > > I think it might be better to have a tri-state enum, than to have two > > > competing bools where only 3 of the 4 states are valid. > > > > Note that this is not a bool. We have one bool and one 3-element enum > > (off/on/unmap), where only 5 of the 6 states are valid. Also, at least > > detect-zeroes would go away if we had some kind of blockdev-mirror > > (where the target is added first with blockdev-add), so I think it's > > better to leave it separate. > > Agreed. But by then "drive-mirror derive=... detect-zeroes=unmap unmap=false" s/derive/device/ > will be way too confusing. > > I say let's save that and just go for blockdev-add :) > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg301990.html > > Fam >
diff --git a/block.c b/block.c index 9890707..d9686c5 100644 --- a/block.c +++ b/block.c @@ -33,6 +33,7 @@ #include "qemu/notify.h" #include "block/coroutine.h" #include "block/qapi.h" +#include "qapi/util.h" #include "qmp-commands.h" #include "qemu/timer.h" #include "qapi-event.h" @@ -671,6 +672,31 @@ int bdrv_parse_cache_flags(const char *mode, int *flags) return 0; } +BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes_flags(const char *mode, + int bdrv_flags, + Error **errp) +{ + Error *local_err = NULL; + BlockdevDetectZeroesOptions detect_zeroes; + detect_zeroes = + qapi_enum_parse(BlockdevDetectZeroesOptions_lookup, + mode, + BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX, + BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, + &local_err); + if (local_err) { + error_propagate(errp, local_err); + return detect_zeroes; + } + + if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && + !(bdrv_flags & BDRV_O_UNMAP)) { + error_setg(errp, "setting detect-zeroes to unmap is not allowed " + "without setting discard operation to unmap"); + } + return detect_zeroes; +} + /* * Returns the flags that a temporary snapshot should get, based on the * originally requested flags (the originally requested image will have flags diff --git a/blockdev.c b/blockdev.c index 6a45555..5ad6960 100644 --- a/blockdev.c +++ b/blockdev.c @@ -483,23 +483,13 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, } detect_zeroes = - qapi_enum_parse(BlockdevDetectZeroesOptions_lookup, - qemu_opt_get(opts, "detect-zeroes"), - BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX, - BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, - &error); + bdrv_parse_detect_zeroes_flags(qemu_opt_get(opts, "detect-zeroes"), + bdrv_flags, &error); if (error) { error_propagate(errp, error); goto early_err; } - if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && - !(bdrv_flags & BDRV_O_UNMAP)) { - error_setg(errp, "setting detect-zeroes to unmap is not allowed " - "without setting discard operation to unmap"); - goto early_err; - } - /* init */ if ((!file || !*file) && !has_driver_specific_opts) { blk = blk_new_with_bs(qemu_opts_id(opts), errp); diff --git a/include/block/block.h b/include/block/block.h index 4d9b555..b7905ab 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -194,6 +194,9 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top); int bdrv_parse_cache_flags(const char *mode, int *flags); int bdrv_parse_discard_flags(const char *mode, int *flags); +BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes_flags(const char *mode, + int bdrv_flags, + Error **errp); int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, bool allow_none, Error **errp);
The logic will be shared with qmp_drive_mirror. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 26 ++++++++++++++++++++++++++ blockdev.c | 14 ++------------ include/block/block.h | 3 +++ 3 files changed, 31 insertions(+), 12 deletions(-)