| Message ID | 1314211389-28915-9-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 24 Aug 2011 13:43:03 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > blockdev.c | 22 +++++++++++----------- > blockdev.h | 1 - > hmp-commands.hx | 3 +-- > hmp.c | 14 ++++++++++++++ > hmp.h | 1 + > qapi-schema.json | 25 +++++++++++++++++++++++++ > qmp-commands.hx | 3 +-- > 7 files changed, 53 insertions(+), 16 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index d272659..6b7fc41 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -16,6 +16,7 @@ > #include "sysemu.h" > #include "hw/qdev.h" > #include "block_int.h" > +#include "qmp-commands.h" > > static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); > > @@ -644,32 +645,31 @@ out: > return ret; > } > > -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) > +static int eject_device(BlockDriverState *bs, int force, Error **errp) > { > if (!bdrv_is_removable(bs)) { > - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); > + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); > return -1; > } > if (!force && bdrv_is_locked(bs)) { > - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > return -1; > } > bdrv_close(bs); > return 0; > } > > -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) > +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > { > BlockDriverState *bs; > - int force = qdict_get_try_bool(qdict, "force", 0); > - const char *filename = qdict_get_str(qdict, "device"); > > - bs = bdrv_find(filename); > + bs = bdrv_find(device); > if (!bs) { > - qerror_report(QERR_DEVICE_NOT_FOUND, filename); > - return -1; > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > } > - return eject_device(mon, bs, force); > + > + eject_device(bs, force, errp); > } > > int do_block_set_passwd(Monitor *mon, const QDict *qdict, > @@ -715,7 +715,7 @@ int do_change_block(Monitor *mon, const char *device, > return -1; > } > } > - if (eject_device(mon, bs, 0) < 0) { > + if (eject_device(bs, 0, NULL) < 0) { This will make the change command return an undefined error for errors caught in eject_device(). I believe this is fixed in patch 13/14? If yes, the it's probably a good thing to note it in the commit log. > return -1; > } > bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; > diff --git a/blockdev.h b/blockdev.h > index 3587786..badbf01 100644 > --- a/blockdev.h > +++ b/blockdev.h > @@ -58,7 +58,6 @@ DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi); > DriveInfo *add_init_drive(const char *opts); > > void do_commit(Monitor *mon, const QDict *qdict); > -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data); > int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data); > int do_change_block(Monitor *mon, const char *device, > const char *filename, const char *fmt); > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 0ccfb28..bcb789b 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -76,8 +76,7 @@ ETEXI > .args_type = "force:-f,device:B", > .params = "[-f] device", > .help = "eject a removable medium (use -f to force it)", > - .user_print = monitor_user_noop, > - .mhandler.cmd_new = do_eject, > + .mhandler.cmd = hmp_eject, > }, > > STEXI > diff --git a/hmp.c b/hmp.c > index 47e1ff7..36eb5b9 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -24,3 +24,17 @@ void hmp_info_name(Monitor *mon) > } > qapi_free_NameInfo(info); > } > + > +void hmp_eject(Monitor *mon, const QDict *qdict) > +{ > + int force = qdict_get_try_bool(qdict, "force", 0); > + const char *device = qdict_get_str(qdict, "device"); > + Error *err = NULL; > + > + qmp_eject(device, true, force, &err); > + if (err) { > + monitor_printf(mon, "eject: %s\n", error_get_pretty(err)); > + error_free(err); > + } > +} > + > diff --git a/hmp.h b/hmp.h > index 5fe73f1..6a552c1 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -18,5 +18,6 @@ > #include "qapi-types.h" > > void hmp_info_name(Monitor *mon); > +void hmp_eject(Monitor *mon, const QDict *args); > > #endif > diff --git a/qapi-schema.json b/qapi-schema.json > index 654409b..934ea81 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -24,3 +24,28 @@ > ## > { 'command': 'query-name', 'returns': 'NameInfo' } > > +## > +# @eject: > +# > +# Ejects a device from a removable drive. > +# > +# @device: The name of the device > +# > +# @force: @optional If true, eject regardless of whether the drive is locked. > +# If not specified, the default value is false. > +# > +# Returns: Nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# If @device is not removable and @force is false, DeviceNotRemovable > +# If @force is false and @device is locked, DeviceLocked > +# > +# Notes: If the @force flag is used, the backing file will be closed > +# regardless of whether the device is removable. This may result in > +# a badly broken guest. > +# > +# Ejecting a device with no media results in success > +# > +# Since: 0.14.0 > +## We're duplicating the documentation, as it also exists in qmp-commands.hx. Should we drop it from there? If we do, we'll have to update the script that generates QMP/qmp-commands.txt. > +{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} } > + > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 03f67da..81d1800 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -89,8 +89,7 @@ EQMP > .args_type = "force:-f,device:B", > .params = "[-f] device", > .help = "eject a removable medium (use -f to force it)", > - .user_print = monitor_user_noop, > - .mhandler.cmd_new = do_eject, > + .mhandler.cmd_new = qmp_marshal_input_eject, > }, > > SQMP
Am 24.08.2011 20:43, schrieb Anthony Liguori: > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > blockdev.c | 22 +++++++++++----------- > blockdev.h | 1 - > hmp-commands.hx | 3 +-- > hmp.c | 14 ++++++++++++++ > hmp.h | 1 + > qapi-schema.json | 25 +++++++++++++++++++++++++ > qmp-commands.hx | 3 +-- > 7 files changed, 53 insertions(+), 16 deletions(-) All of the conversion patches I've read so far add more lines than they delete (even when you ignore changes to the schema, which is mostly new documentation), even though I had expected code generation to do the opposite, that is less hand-written code. Is this expected, or are these first examples just exceptions? > diff --git a/blockdev.c b/blockdev.c > index d272659..6b7fc41 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -16,6 +16,7 @@ > #include "sysemu.h" > #include "hw/qdev.h" > #include "block_int.h" > +#include "qmp-commands.h" > > static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); > > @@ -644,32 +645,31 @@ out: > return ret; > } > > -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) > +static int eject_device(BlockDriverState *bs, int force, Error **errp) > { > if (!bdrv_is_removable(bs)) { > - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); > + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); > return -1; > } > if (!force && bdrv_is_locked(bs)) { > - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > return -1; > } > bdrv_close(bs); > return 0; > } > > -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) > +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) Wow, this is ugly. :-) I would suspect that many cases of optional arguments are like this: If it isn't specified, the very first thing the monitor handler does is to assign a default value (false in this case). Can't we include default values in the schema and get the handling outside instead of an additional has_xyz parameter that can easily be ignored by accident, like in the code below? > { > BlockDriverState *bs; > - int force = qdict_get_try_bool(qdict, "force", 0); > - const char *filename = qdict_get_str(qdict, "device"); > > - bs = bdrv_find(filename); > + bs = bdrv_find(device); > if (!bs) { > - qerror_report(QERR_DEVICE_NOT_FOUND, filename); > - return -1; > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > } > - return eject_device(mon, bs, force); > + > + eject_device(bs, force, errp); > } Kevin
On 08/25/2011 07:19 AM, Kevin Wolf wrote: > Am 24.08.2011 20:43, schrieb Anthony Liguori: >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> blockdev.c | 22 +++++++++++----------- >> blockdev.h | 1 - >> hmp-commands.hx | 3 +-- >> hmp.c | 14 ++++++++++++++ >> hmp.h | 1 + >> qapi-schema.json | 25 +++++++++++++++++++++++++ >> qmp-commands.hx | 3 +-- >> 7 files changed, 53 insertions(+), 16 deletions(-) > > All of the conversion patches I've read so far add more lines than they > delete (even when you ignore changes to the schema, which is mostly new > documentation), even though I had expected code generation to do the > opposite, that is less hand-written code. > > Is this expected, or are these first examples just exceptions? Yes. These are extremely simple interfaces so unmarshalling a couple strings by hand really isn't all that hard to do. Plus, this series adds 4 new commands and also adds significantly more documentation than has ever existed before (in fact, that's the largest add in this patch). The real code savings comes in for the commands that return complex data structures like query-vnc. Not only do we save code, but we save a lot of complexity. In the full conversion branch, I think we're generating somewhere around 10k lines of code. So there's a pretty significant savings. > >> diff --git a/blockdev.c b/blockdev.c >> index d272659..6b7fc41 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -16,6 +16,7 @@ >> #include "sysemu.h" >> #include "hw/qdev.h" >> #include "block_int.h" >> +#include "qmp-commands.h" >> >> static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >> >> @@ -644,32 +645,31 @@ out: >> return ret; >> } >> >> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) >> +static int eject_device(BlockDriverState *bs, int force, Error **errp) >> { >> if (!bdrv_is_removable(bs)) { >> - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >> + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >> return -1; >> } >> if (!force&& bdrv_is_locked(bs)) { >> - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >> + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >> return -1; >> } >> bdrv_close(bs); >> return 0; >> } >> >> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) >> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > > Wow, this is ugly. :-) > > I would suspect that many cases of optional arguments are like this: If > it isn't specified, the very first thing the monitor handler does is to > assign a default value (false in this case). Can't we include default > values in the schema and get the handling outside instead of an > additional has_xyz parameter that can easily be ignored by accident, > like in the code below? There are quite a few commands that actually rely on tristate behavior. So they'll do things like: if (has_force) { if (force) { do_A(); } else { do_B(); } } else { do_C(); } It's not pretty, but it lets us preserve compatibility. I think it's also safer for dealing with pointers because otherwise you have a mix of pointers that may be null and may not be null. Having a clear indication of which pointers are nullable makes for safer code. Regards, Anthony Liguori > >> { >> BlockDriverState *bs; >> - int force = qdict_get_try_bool(qdict, "force", 0); >> - const char *filename = qdict_get_str(qdict, "device"); >> >> - bs = bdrv_find(filename); >> + bs = bdrv_find(device); >> if (!bs) { >> - qerror_report(QERR_DEVICE_NOT_FOUND, filename); >> - return -1; >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> } >> - return eject_device(mon, bs, force); >> + >> + eject_device(bs, force, errp); >> } > > Kevin >
Am 25.08.2011 15:40, schrieb Anthony Liguori: > On 08/25/2011 07:19 AM, Kevin Wolf wrote: >> Am 24.08.2011 20:43, schrieb Anthony Liguori: >>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>> --- >>> blockdev.c | 22 +++++++++++----------- >>> blockdev.h | 1 - >>> hmp-commands.hx | 3 +-- >>> hmp.c | 14 ++++++++++++++ >>> hmp.h | 1 + >>> qapi-schema.json | 25 +++++++++++++++++++++++++ >>> qmp-commands.hx | 3 +-- >>> 7 files changed, 53 insertions(+), 16 deletions(-) >> >> All of the conversion patches I've read so far add more lines than they >> delete (even when you ignore changes to the schema, which is mostly new >> documentation), even though I had expected code generation to do the >> opposite, that is less hand-written code. >> >> Is this expected, or are these first examples just exceptions? > > Yes. These are extremely simple interfaces so unmarshalling a couple > strings by hand really isn't all that hard to do. Plus, this series > adds 4 new commands and also adds significantly more documentation than > has ever existed before (in fact, that's the largest add in this patch). > > The real code savings comes in for the commands that return complex data > structures like query-vnc. Not only do we save code, but we save a lot > of complexity. > > In the full conversion branch, I think we're generating somewhere around > 10k lines of code. So there's a pretty significant savings. > >> >>> diff --git a/blockdev.c b/blockdev.c >>> index d272659..6b7fc41 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -16,6 +16,7 @@ >>> #include "sysemu.h" >>> #include "hw/qdev.h" >>> #include "block_int.h" >>> +#include "qmp-commands.h" >>> >>> static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >>> >>> @@ -644,32 +645,31 @@ out: >>> return ret; >>> } >>> >>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) >>> +static int eject_device(BlockDriverState *bs, int force, Error **errp) >>> { >>> if (!bdrv_is_removable(bs)) { >>> - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >>> + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >>> return -1; >>> } >>> if (!force&& bdrv_is_locked(bs)) { >>> - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >>> + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >>> return -1; >>> } >>> bdrv_close(bs); >>> return 0; >>> } >>> >>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) >>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >> >> Wow, this is ugly. :-) >> >> I would suspect that many cases of optional arguments are like this: If >> it isn't specified, the very first thing the monitor handler does is to >> assign a default value (false in this case). Can't we include default >> values in the schema and get the handling outside instead of an >> additional has_xyz parameter that can easily be ignored by accident, >> like in the code below? > > There are quite a few commands that actually rely on tristate behavior. > So they'll do things like: > > if (has_force) { > if (force) { > do_A(); > } else { > do_B(); > } > } else { > do_C(); > } > > It's not pretty, but it lets us preserve compatibility. I think it's > also safer for dealing with pointers because otherwise you have a mix of > pointers that may be null and may not be null. Having a clear > indication of which pointers are nullable makes for safer code. I'm not saying that implementing a default value in generic (or generated) code works for all cases. But if the schema supported default values, we could get rid of the parameter in all simple cases (which I would expect to be the majority); and if there is no default value in the schema, we could still generate the has_* parameters. Kevin
On 08/25/2011 04:52 PM, Kevin Wolf wrote: > > > > It's not pretty, but it lets us preserve compatibility. I think it's > > also safer for dealing with pointers because otherwise you have a mix of > > pointers that may be null and may not be null. Having a clear > > indication of which pointers are nullable makes for safer code. > > I'm not saying that implementing a default value in generic (or > generated) code works for all cases. But if the schema supported default > values, we could get rid of the parameter in all simple cases (which I > would expect to be the majority); and if there is no default value in > the schema, we could still generate the has_* parameters. > An alternative is to pass a struct by value, with a .valid bool followed by the actual value (which should be initialized even if not valid, just to be safe).
On 08/25/2011 08:52 AM, Kevin Wolf wrote: > Am 25.08.2011 15:40, schrieb Anthony Liguori: >> On 08/25/2011 07:19 AM, Kevin Wolf wrote: >>> Am 24.08.2011 20:43, schrieb Anthony Liguori: >>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>>> --- >>>> blockdev.c | 22 +++++++++++----------- >>>> blockdev.h | 1 - >>>> hmp-commands.hx | 3 +-- >>>> hmp.c | 14 ++++++++++++++ >>>> hmp.h | 1 + >>>> qapi-schema.json | 25 +++++++++++++++++++++++++ >>>> qmp-commands.hx | 3 +-- >>>> 7 files changed, 53 insertions(+), 16 deletions(-) >>> >>> All of the conversion patches I've read so far add more lines than they >>> delete (even when you ignore changes to the schema, which is mostly new >>> documentation), even though I had expected code generation to do the >>> opposite, that is less hand-written code. >>> >>> Is this expected, or are these first examples just exceptions? >> >> Yes. These are extremely simple interfaces so unmarshalling a couple >> strings by hand really isn't all that hard to do. Plus, this series >> adds 4 new commands and also adds significantly more documentation than >> has ever existed before (in fact, that's the largest add in this patch). >> >> The real code savings comes in for the commands that return complex data >> structures like query-vnc. Not only do we save code, but we save a lot >> of complexity. >> >> In the full conversion branch, I think we're generating somewhere around >> 10k lines of code. So there's a pretty significant savings. >> >>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index d272659..6b7fc41 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -16,6 +16,7 @@ >>>> #include "sysemu.h" >>>> #include "hw/qdev.h" >>>> #include "block_int.h" >>>> +#include "qmp-commands.h" >>>> >>>> static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >>>> >>>> @@ -644,32 +645,31 @@ out: >>>> return ret; >>>> } >>>> >>>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) >>>> +static int eject_device(BlockDriverState *bs, int force, Error **errp) >>>> { >>>> if (!bdrv_is_removable(bs)) { >>>> - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >>>> + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >>>> return -1; >>>> } >>>> if (!force&& bdrv_is_locked(bs)) { >>>> - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >>>> + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >>>> return -1; >>>> } >>>> bdrv_close(bs); >>>> return 0; >>>> } >>>> >>>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) >>>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >>> >>> Wow, this is ugly. :-) >>> >>> I would suspect that many cases of optional arguments are like this: If >>> it isn't specified, the very first thing the monitor handler does is to >>> assign a default value (false in this case). Can't we include default >>> values in the schema and get the handling outside instead of an >>> additional has_xyz parameter that can easily be ignored by accident, >>> like in the code below? >> >> There are quite a few commands that actually rely on tristate behavior. >> So they'll do things like: >> >> if (has_force) { >> if (force) { >> do_A(); >> } else { >> do_B(); >> } >> } else { >> do_C(); >> } >> >> It's not pretty, but it lets us preserve compatibility. I think it's >> also safer for dealing with pointers because otherwise you have a mix of >> pointers that may be null and may not be null. Having a clear >> indication of which pointers are nullable makes for safer code. > > I'm not saying that implementing a default value in generic (or > generated) code works for all cases. But if the schema supported default > values, we could get rid of the parameter in all simple cases (which I > would expect to be the majority); and if there is no default value in > the schema, we could still generate the has_* parameters. I had thought about this but forgot to respond. The problem is that the schema doesn't help the C API. We're going to use QMP commands internally too. So if the schema defaulted optional arguments, you'd end up having those default values open coded in the C APIs. As ugly as it is, having an explicit not-specified flag allows the C API to have the same semantics as the wire protocol. Honestly, having optional arguments in the methods was a bad move. We shouldn't do that anymore. Optional arguments should always be done via a structure as Avi sort of suggested in another response. Regards, Anthony Liguori > > Kevin >
Am 02.09.2011 18:05, schrieb Anthony Liguori: > On 08/25/2011 08:52 AM, Kevin Wolf wrote: >> Am 25.08.2011 15:40, schrieb Anthony Liguori: >>> On 08/25/2011 07:19 AM, Kevin Wolf wrote: >>>> Am 24.08.2011 20:43, schrieb Anthony Liguori: >>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>>>> --- >>>>> blockdev.c | 22 +++++++++++----------- >>>>> blockdev.h | 1 - >>>>> hmp-commands.hx | 3 +-- >>>>> hmp.c | 14 ++++++++++++++ >>>>> hmp.h | 1 + >>>>> qapi-schema.json | 25 +++++++++++++++++++++++++ >>>>> qmp-commands.hx | 3 +-- >>>>> 7 files changed, 53 insertions(+), 16 deletions(-) >>>> >>>> All of the conversion patches I've read so far add more lines than they >>>> delete (even when you ignore changes to the schema, which is mostly new >>>> documentation), even though I had expected code generation to do the >>>> opposite, that is less hand-written code. >>>> >>>> Is this expected, or are these first examples just exceptions? >>> >>> Yes. These are extremely simple interfaces so unmarshalling a couple >>> strings by hand really isn't all that hard to do. Plus, this series >>> adds 4 new commands and also adds significantly more documentation than >>> has ever existed before (in fact, that's the largest add in this patch). >>> >>> The real code savings comes in for the commands that return complex data >>> structures like query-vnc. Not only do we save code, but we save a lot >>> of complexity. >>> >>> In the full conversion branch, I think we're generating somewhere around >>> 10k lines of code. So there's a pretty significant savings. >>> >>>> >>>>> diff --git a/blockdev.c b/blockdev.c >>>>> index d272659..6b7fc41 100644 >>>>> --- a/blockdev.c >>>>> +++ b/blockdev.c >>>>> @@ -16,6 +16,7 @@ >>>>> #include "sysemu.h" >>>>> #include "hw/qdev.h" >>>>> #include "block_int.h" >>>>> +#include "qmp-commands.h" >>>>> >>>>> static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >>>>> >>>>> @@ -644,32 +645,31 @@ out: >>>>> return ret; >>>>> } >>>>> >>>>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) >>>>> +static int eject_device(BlockDriverState *bs, int force, Error **errp) >>>>> { >>>>> if (!bdrv_is_removable(bs)) { >>>>> - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >>>>> + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >>>>> return -1; >>>>> } >>>>> if (!force&& bdrv_is_locked(bs)) { >>>>> - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >>>>> + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >>>>> return -1; >>>>> } >>>>> bdrv_close(bs); >>>>> return 0; >>>>> } >>>>> >>>>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) >>>>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >>>> >>>> Wow, this is ugly. :-) >>>> >>>> I would suspect that many cases of optional arguments are like this: If >>>> it isn't specified, the very first thing the monitor handler does is to >>>> assign a default value (false in this case). Can't we include default >>>> values in the schema and get the handling outside instead of an >>>> additional has_xyz parameter that can easily be ignored by accident, >>>> like in the code below? >>> >>> There are quite a few commands that actually rely on tristate behavior. >>> So they'll do things like: >>> >>> if (has_force) { >>> if (force) { >>> do_A(); >>> } else { >>> do_B(); >>> } >>> } else { >>> do_C(); >>> } >>> >>> It's not pretty, but it lets us preserve compatibility. I think it's >>> also safer for dealing with pointers because otherwise you have a mix of >>> pointers that may be null and may not be null. Having a clear >>> indication of which pointers are nullable makes for safer code. >> >> I'm not saying that implementing a default value in generic (or >> generated) code works for all cases. But if the schema supported default >> values, we could get rid of the parameter in all simple cases (which I >> would expect to be the majority); and if there is no default value in >> the schema, we could still generate the has_* parameters. > > I had thought about this but forgot to respond. > > The problem is that the schema doesn't help the C API. We're going to > use QMP commands internally too. So if the schema defaulted optional > arguments, you'd end up having those default values open coded in the C > APIs. As ugly as it is, having an explicit not-specified flag allows > the C API to have the same semantics as the wire protocol. Do we need it? One of the major use cases of optional arguments in the QMP protocol is extending existing commands in newer versions. For these cases, there is no problem with requiring all internal C callers to be updated to actually specify the field that is optional externally. > Honestly, having optional arguments in the methods was a bad move. We > shouldn't do that anymore. Optional arguments should always be done via > a structure as Avi sort of suggested in another response. Maybe. I'm not sure if that makes the C code any nicer, though. Kevin
diff --git a/blockdev.c b/blockdev.c index d272659..6b7fc41 100644 --- a/blockdev.c +++ b/blockdev.c @@ -16,6 +16,7 @@ #include "sysemu.h" #include "hw/qdev.h" #include "block_int.h" +#include "qmp-commands.h" static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); @@ -644,32 +645,31 @@ out: return ret; } -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) +static int eject_device(BlockDriverState *bs, int force, Error **errp) { if (!bdrv_is_removable(bs)) { - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); return -1; } if (!force && bdrv_is_locked(bs)) { - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); return -1; } bdrv_close(bs); return 0; } -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) { BlockDriverState *bs; - int force = qdict_get_try_bool(qdict, "force", 0); - const char *filename = qdict_get_str(qdict, "device"); - bs = bdrv_find(filename); + bs = bdrv_find(device); if (!bs) { - qerror_report(QERR_DEVICE_NOT_FOUND, filename); - return -1; + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; } - return eject_device(mon, bs, force); + + eject_device(bs, force, errp); } int do_block_set_passwd(Monitor *mon, const QDict *qdict, @@ -715,7 +715,7 @@ int do_change_block(Monitor *mon, const char *device, return -1; } } - if (eject_device(mon, bs, 0) < 0) { + if (eject_device(bs, 0, NULL) < 0) { return -1; } bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; diff --git a/blockdev.h b/blockdev.h index 3587786..badbf01 100644 --- a/blockdev.h +++ b/blockdev.h @@ -58,7 +58,6 @@ DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi); DriveInfo *add_init_drive(const char *opts); void do_commit(Monitor *mon, const QDict *qdict); -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_change_block(Monitor *mon, const char *device, const char *filename, const char *fmt); diff --git a/hmp-commands.hx b/hmp-commands.hx index 0ccfb28..bcb789b 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -76,8 +76,7 @@ ETEXI .args_type = "force:-f,device:B", .params = "[-f] device", .help = "eject a removable medium (use -f to force it)", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_eject, + .mhandler.cmd = hmp_eject, }, STEXI diff --git a/hmp.c b/hmp.c index 47e1ff7..36eb5b9 100644 --- a/hmp.c +++ b/hmp.c @@ -24,3 +24,17 @@ void hmp_info_name(Monitor *mon) } qapi_free_NameInfo(info); } + +void hmp_eject(Monitor *mon, const QDict *qdict) +{ + int force = qdict_get_try_bool(qdict, "force", 0); + const char *device = qdict_get_str(qdict, "device"); + Error *err = NULL; + + qmp_eject(device, true, force, &err); + if (err) { + monitor_printf(mon, "eject: %s\n", error_get_pretty(err)); + error_free(err); + } +} + diff --git a/hmp.h b/hmp.h index 5fe73f1..6a552c1 100644 --- a/hmp.h +++ b/hmp.h @@ -18,5 +18,6 @@ #include "qapi-types.h" void hmp_info_name(Monitor *mon); +void hmp_eject(Monitor *mon, const QDict *args); #endif diff --git a/qapi-schema.json b/qapi-schema.json index 654409b..934ea81 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -24,3 +24,28 @@ ## { 'command': 'query-name', 'returns': 'NameInfo' } +## +# @eject: +# +# Ejects a device from a removable drive. +# +# @device: The name of the device +# +# @force: @optional If true, eject regardless of whether the drive is locked. +# If not specified, the default value is false. +# +# Returns: Nothing on success +# If @device is not a valid block device, DeviceNotFound +# If @device is not removable and @force is false, DeviceNotRemovable +# If @force is false and @device is locked, DeviceLocked +# +# Notes: If the @force flag is used, the backing file will be closed +# regardless of whether the device is removable. This may result in +# a badly broken guest. +# +# Ejecting a device with no media results in success +# +# Since: 0.14.0 +## +{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} } + diff --git a/qmp-commands.hx b/qmp-commands.hx index 03f67da..81d1800 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -89,8 +89,7 @@ EQMP .args_type = "force:-f,device:B", .params = "[-f] device", .help = "eject a removable medium (use -f to force it)", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_eject, + .mhandler.cmd_new = qmp_marshal_input_eject, }, SQMP
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- blockdev.c | 22 +++++++++++----------- blockdev.h | 1 - hmp-commands.hx | 3 +-- hmp.c | 14 ++++++++++++++ hmp.h | 1 + qapi-schema.json | 25 +++++++++++++++++++++++++ qmp-commands.hx | 3 +-- 7 files changed, 53 insertions(+), 16 deletions(-)