| Message ID | 1463532149-11625-1-git-send-email-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 05/17 20:42, John Snow wrote: > If you use HMP's eject but the CDROM tray is locked, you may get a > confusing error message informing you that the "tray isn't open." > > As this is the point of eject, we can do a little better and help > clarify that the tray was locked and that it (might) open up later, > so try again. > > It's not ideal, but it makes the semantics of the (legacy) eject > command more understandable to end users when they try to use it. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > blockdev.c | 36 +++++++++++++++++++++++++++++------- > 1 file changed, 29 insertions(+), 7 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 1892b8e..feb8484 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2290,16 +2290,26 @@ exit: > block_job_txn_unref(block_job_txn); > } > > +static int do_open_tray(const char *device, bool has_force, bool force, > + Error **errp); > + > void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > { > Error *local_err = NULL; > + int rc; > > - qmp_blockdev_open_tray(device, has_force, force, &local_err); > + rc = do_open_tray(device, has_force, force, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } > > + if (rc == -EINPROGRESS) { > + error_setg(errp, "Device '%s' is locked and force was not specified, " > + "wait for tray to open and try again", device); > + return; > + } > + > qmp_x_blockdev_remove_medium(device, errp); > } > > @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, > aio_context_release(aio_context); > } > > -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, > - Error **errp) > +static int do_open_tray(const char *device, bool has_force, bool force, > + Error **errp) Personally I feel the has_force and force could be merged as one parameter. > { > BlockBackend *blk; > bool locked; > @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, > if (!blk) { > error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > "Device '%s' not found", device); > - return; > + return -ENODEV; > } > > if (!blk_dev_has_removable_media(blk)) { > error_setg(errp, "Device '%s' is not removable", device); > - return; > + return -ENOTSUP; > } > > if (!blk_dev_has_tray(blk)) { > /* Ignore this command on tray-less devices */ > - return; > + return -ENOSYS; I'm not sure how acceptable it is to leave errp untouched while setting ret code to non-zero. Markus? Fam > } > > if (blk_dev_is_tray_open(blk)) { > - return; > + return 0; > } > > locked = blk_dev_is_medium_locked(blk); > @@ -2366,6 +2376,18 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, > if (!locked || force) { > blk_dev_change_media_cb(blk, false); > } > + > + if (locked && !force) { > + return -EINPROGRESS; > + } > + > + return 0; > +} > + > +void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, > + Error **errp) > +{ > + do_open_tray(device, has_force, force, errp); > } > > void qmp_blockdev_close_tray(const char *device, Error **errp) > -- > 2.4.11 > >
On 05/17/2016 06:42 PM, John Snow wrote: > If you use HMP's eject but the CDROM tray is locked, you may get a > confusing error message informing you that the "tray isn't open." > > As this is the point of eject, we can do a little better and help > clarify that the tray was locked and that it (might) open up later, > so try again. > > It's not ideal, but it makes the semantics of the (legacy) eject > command more understandable to end users when they try to use it. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > blockdev.c | 36 +++++++++++++++++++++++++++++------- > 1 file changed, 29 insertions(+), 7 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On 05/17/2016 08:18 PM, Fam Zheng wrote: > On Tue, 05/17 20:42, John Snow wrote: >> If you use HMP's eject but the CDROM tray is locked, you may get a >> confusing error message informing you that the "tray isn't open." >> >> As this is the point of eject, we can do a little better and help >> clarify that the tray was locked and that it (might) open up later, >> so try again. >> >> It's not ideal, but it makes the semantics of the (legacy) eject >> command more understandable to end users when they try to use it. >> >> if (!blk_dev_has_removable_media(blk)) { >> error_setg(errp, "Device '%s' is not removable", device); >> - return; >> + return -ENOTSUP; >> } >> >> if (!blk_dev_has_tray(blk)) { >> /* Ignore this command on tray-less devices */ >> - return; >> + return -ENOSYS; > > I'm not sure how acceptable it is to leave errp untouched while setting ret > code to non-zero. Markus? You're basically returning a tri-state: errp set (fatal, error issued), 0 (success, no error needed), or errp clear and negative (potential error, but caller must decide). It's unusual enough that you probably ought to document it at the top of the function, and it may mess with Markus' pending work to return status codes from functions taking Error **. It might also be worth considering using a POSITIVE return value when not setting an error code, so that a quick <0 check is synonymous with error set, 0 remains the code for complete success, and the locked tray with a retry now has an identifiable sentinel - particularly since you don't care about what errno values were in use except for EINPROGRESS.
Fam Zheng <famz@redhat.com> writes: > On Tue, 05/17 20:42, John Snow wrote: >> If you use HMP's eject but the CDROM tray is locked, you may get a >> confusing error message informing you that the "tray isn't open." >> >> As this is the point of eject, we can do a little better and help >> clarify that the tray was locked and that it (might) open up later, >> so try again. >> >> It's not ideal, but it makes the semantics of the (legacy) eject >> command more understandable to end users when they try to use it. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> blockdev.c | 36 +++++++++++++++++++++++++++++------- >> 1 file changed, 29 insertions(+), 7 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 1892b8e..feb8484 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2290,16 +2290,26 @@ exit: >> block_job_txn_unref(block_job_txn); >> } >> >> +static int do_open_tray(const char *device, bool has_force, bool force, >> + Error **errp); >> + >> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >> { >> Error *local_err = NULL; >> + int rc; >> >> - qmp_blockdev_open_tray(device, has_force, force, &local_err); >> + rc = do_open_tray(device, has_force, force, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> return; >> } >> >> + if (rc == -EINPROGRESS) { >> + error_setg(errp, "Device '%s' is locked and force was not specified, " >> + "wait for tray to open and try again", device); >> + return; >> + } >> + >> qmp_x_blockdev_remove_medium(device, errp); >> } >> >> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, >> aio_context_release(aio_context); >> } >> >> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >> - Error **errp) >> +static int do_open_tray(const char *device, bool has_force, bool force, >> + Error **errp) > > Personally I feel the has_force and force could be merged as one parameter. For qmp_blockdev_open_tray(), the signature is dictated by scripts/qapi-commands.py. To make has_FOO go away, you need to make the FOO non-optional. You have to duplicate the cumbersome has_FOO, FOO couple in your helper functions only when an absent value (has_FOO=false) has special meaning you can't get with any present value. Not my favorite interface design, by the way. We've discussed two improvements to the QAPI language and generators: * Optional with default: has_FOO goes away, and instead FOO assumes the default value declared in the schema when it's absent. Optional without default stays at it is, i.e. has_FOO tells whether it's present. * Use null pointer for absent when it can't be a value. If Eric stops flooding me with QAPI patches, I might even get to implement them :) >> { >> BlockBackend *blk; >> bool locked; >> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >> if (!blk) { >> error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> "Device '%s' not found", device); >> - return; >> + return -ENODEV; >> } >> >> if (!blk_dev_has_removable_media(blk)) { >> error_setg(errp, "Device '%s' is not removable", device); >> - return; >> + return -ENOTSUP; >> } >> >> if (!blk_dev_has_tray(blk)) { >> /* Ignore this command on tray-less devices */ >> - return; >> + return -ENOSYS; > > I'm not sure how acceptable it is to leave errp untouched while setting ret > code to non-zero. Markus? It's questionable style, becaue it gives the two plausible ways to check for errors different meaning: if (do_open_tray(...) < 0) ... and Error *err = NULL; do_open_tray(..., &err); if (err) ... I find this confusing. The former way lets me pass a null Error * argument, which is convenient when I'm not interested in error details. Whenever practical, separate an Error-setting function's values into distinct error and success sets. Example: when a function looks up something, return pointer to it on success, set error and return null on failure. This isn't always practical, for instance, when a pointer-valued function can legitimately return null. That causes confusion, too. We fixed a few bugs around such functions. Whether it isn't practical for *this* function I can't say without developing a better understanding of its purpose and context. [...]
On Wed, 05/18 07:36, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > > > On Tue, 05/17 20:42, John Snow wrote: > >> If you use HMP's eject but the CDROM tray is locked, you may get a > >> confusing error message informing you that the "tray isn't open." > >> > >> As this is the point of eject, we can do a little better and help > >> clarify that the tray was locked and that it (might) open up later, > >> so try again. > >> > >> It's not ideal, but it makes the semantics of the (legacy) eject > >> command more understandable to end users when they try to use it. > >> > >> Signed-off-by: John Snow <jsnow@redhat.com> > >> --- > >> blockdev.c | 36 +++++++++++++++++++++++++++++------- > >> 1 file changed, 29 insertions(+), 7 deletions(-) > >> > >> diff --git a/blockdev.c b/blockdev.c > >> index 1892b8e..feb8484 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -2290,16 +2290,26 @@ exit: > >> block_job_txn_unref(block_job_txn); > >> } > >> > >> +static int do_open_tray(const char *device, bool has_force, bool force, > >> + Error **errp); > >> + > >> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > >> { > >> Error *local_err = NULL; > >> + int rc; > >> > >> - qmp_blockdev_open_tray(device, has_force, force, &local_err); > >> + rc = do_open_tray(device, has_force, force, &local_err); > >> if (local_err) { > >> error_propagate(errp, local_err); > >> return; > >> } > >> > >> + if (rc == -EINPROGRESS) { > >> + error_setg(errp, "Device '%s' is locked and force was not specified, " > >> + "wait for tray to open and try again", device); > >> + return; > >> + } > >> + > >> qmp_x_blockdev_remove_medium(device, errp); > >> } > >> > >> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, > >> aio_context_release(aio_context); > >> } > >> > >> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, > >> - Error **errp) > >> +static int do_open_tray(const char *device, bool has_force, bool force, > >> + Error **errp) > > > > Personally I feel the has_force and force could be merged as one parameter. > > For qmp_blockdev_open_tray(), the signature is dictated by > scripts/qapi-commands.py. To make has_FOO go away, you need to make the > FOO non-optional. > > You have to duplicate the cumbersome has_FOO, FOO couple in your helper > functions only when an absent value (has_FOO=false) has special meaning I was only talking about the helper function, but that is more of a personal taste thing. > you can't get with any present value. Not my favorite interface design, > by the way. > > We've discussed two improvements to the QAPI language and generators: > > * Optional with default: has_FOO goes away, and instead FOO assumes the > default value declared in the schema when it's absent. Optional > without default stays at it is, i.e. has_FOO tells whether it's > present. > > * Use null pointer for absent when it can't be a value. > > If Eric stops flooding me with QAPI patches, I might even get to > implement them :) > > >> { > >> BlockBackend *blk; > >> bool locked; > >> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, > >> if (!blk) { > >> error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > >> "Device '%s' not found", device); > >> - return; > >> + return -ENODEV; > >> } > >> > >> if (!blk_dev_has_removable_media(blk)) { > >> error_setg(errp, "Device '%s' is not removable", device); > >> - return; > >> + return -ENOTSUP; > >> } > >> > >> if (!blk_dev_has_tray(blk)) { > >> /* Ignore this command on tray-less devices */ > >> - return; > >> + return -ENOSYS; > > > > I'm not sure how acceptable it is to leave errp untouched while setting ret > > code to non-zero. Markus? > > It's questionable style, becaue it gives the two plausible ways to check > for errors different meaning: > > if (do_open_tray(...) < 0) ... > > and > > Error *err = NULL; > do_open_tray(..., &err); > if (err) ... > > I find this confusing. > > The former way lets me pass a null Error * argument, which is convenient > when I'm not interested in error details. > > Whenever practical, separate an Error-setting function's values into > distinct error and success sets. Example: when a function looks up > something, return pointer to it on success, set error and return null on > failure. > > This isn't always practical, for instance, when a pointer-valued > function can legitimately return null. That causes confusion, too. We > fixed a few bugs around such functions. > > Whether it isn't practical for *this* function I can't say without > developing a better understanding of its purpose and context. We have this question because errp is mostly human oriented, whereas return codes are also used for control logic. From an error pointer a caller can only tell if the called function succeeded or not, but cannot tell which type the failure is. Comparing this to exception handling systems in other OO languages such as Python, I feel this is because lacking of the type information which would cover this case if we had one too. With error type information, the idiom with "ret code + errp" would then become similar to: try: do_open_tray() except EjectInProgress: pass except Exception: # report error ... And a return code is not needed. (not saying this is the only type of control flow, Functions looking up something will still return pointers, but on the other hand it's possible those function may want to return error type too.) We used to have rich type errors, which has been abandoned, but I think it probably makes some sense to let Error carry a standard error code like EINPROGRESS, ENOTSUP, etc? Error *err = NULL; do_open_tray(..., &err); if (error_num(err) == EINPROGRESS) { ... } else{ ... } Or should we simply use errno in this case? Fam
Fam Zheng <famz@redhat.com> writes: > On Wed, 05/18 07:36, Markus Armbruster wrote: >> Fam Zheng <famz@redhat.com> writes: >> >> > On Tue, 05/17 20:42, John Snow wrote: >> >> If you use HMP's eject but the CDROM tray is locked, you may get a >> >> confusing error message informing you that the "tray isn't open." >> >> >> >> As this is the point of eject, we can do a little better and help >> >> clarify that the tray was locked and that it (might) open up later, >> >> so try again. >> >> >> >> It's not ideal, but it makes the semantics of the (legacy) eject >> >> command more understandable to end users when they try to use it. >> >> >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> >> --- >> >> blockdev.c | 36 +++++++++++++++++++++++++++++------- >> >> 1 file changed, 29 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/blockdev.c b/blockdev.c >> >> index 1892b8e..feb8484 100644 >> >> --- a/blockdev.c >> >> +++ b/blockdev.c >> >> @@ -2290,16 +2290,26 @@ exit: >> >> block_job_txn_unref(block_job_txn); >> >> } >> >> >> >> +static int do_open_tray(const char *device, bool has_force, bool force, >> >> + Error **errp); >> >> + >> >> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >> >> { >> >> Error *local_err = NULL; >> >> + int rc; >> >> >> >> - qmp_blockdev_open_tray(device, has_force, force, &local_err); >> >> + rc = do_open_tray(device, has_force, force, &local_err); >> >> if (local_err) { >> >> error_propagate(errp, local_err); >> >> return; >> >> } >> >> >> >> + if (rc == -EINPROGRESS) { >> >> + error_setg(errp, "Device '%s' is locked and force was not specified, " >> >> + "wait for tray to open and try again", device); >> >> + return; >> >> + } >> >> + >> >> qmp_x_blockdev_remove_medium(device, errp); >> >> } >> >> >> >> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, >> >> aio_context_release(aio_context); >> >> } >> >> >> >> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >> >> - Error **errp) >> >> +static int do_open_tray(const char *device, bool has_force, bool force, >> >> + Error **errp) >> > >> > Personally I feel the has_force and force could be merged as one parameter. >> >> For qmp_blockdev_open_tray(), the signature is dictated by >> scripts/qapi-commands.py. To make has_FOO go away, you need to make the >> FOO non-optional. >> >> You have to duplicate the cumbersome has_FOO, FOO couple in your helper >> functions only when an absent value (has_FOO=false) has special meaning > > I was only talking about the helper function, but that is more of a personal > taste thing. My personal taste is to omit unnecessary has_FOOs. >> you can't get with any present value. Not my favorite interface design, >> by the way. >> >> We've discussed two improvements to the QAPI language and generators: >> >> * Optional with default: has_FOO goes away, and instead FOO assumes the >> default value declared in the schema when it's absent. Optional >> without default stays at it is, i.e. has_FOO tells whether it's >> present. >> >> * Use null pointer for absent when it can't be a value. >> >> If Eric stops flooding me with QAPI patches, I might even get to >> implement them :) >> >> >> { >> >> BlockBackend *blk; >> >> bool locked; >> >> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >> >> if (!blk) { >> >> error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> >> "Device '%s' not found", device); >> >> - return; >> >> + return -ENODEV; >> >> } >> >> >> >> if (!blk_dev_has_removable_media(blk)) { >> >> error_setg(errp, "Device '%s' is not removable", device); >> >> - return; >> >> + return -ENOTSUP; >> >> } >> >> >> >> if (!blk_dev_has_tray(blk)) { >> >> /* Ignore this command on tray-less devices */ >> >> - return; >> >> + return -ENOSYS; >> > >> > I'm not sure how acceptable it is to leave errp untouched while setting ret >> > code to non-zero. Markus? >> >> It's questionable style, becaue it gives the two plausible ways to check >> for errors different meaning: >> >> if (do_open_tray(...) < 0) ... >> >> and >> >> Error *err = NULL; >> do_open_tray(..., &err); >> if (err) ... >> >> I find this confusing. >> >> The former way lets me pass a null Error * argument, which is convenient >> when I'm not interested in error details. >> >> Whenever practical, separate an Error-setting function's values into >> distinct error and success sets. Example: when a function looks up >> something, return pointer to it on success, set error and return null on >> failure. >> >> This isn't always practical, for instance, when a pointer-valued >> function can legitimately return null. That causes confusion, too. We >> fixed a few bugs around such functions. >> >> Whether it isn't practical for *this* function I can't say without >> developing a better understanding of its purpose and context. > > We have this question because errp is mostly human oriented, whereas return > codes are also used for control logic. From an error pointer a caller can only > tell if the called function succeeded or not, but cannot tell which type the > failure is. Comparing this to exception handling systems in other OO languages > such as Python, I feel this is because lacking of the type information which > would cover this case if we had one too. With error type information, the > idiom with "ret code + errp" would then become similar to: > > try: > do_open_tray() > except EjectInProgress: > pass > except Exception: > # report error > ... > > And a return code is not needed. (not saying this is the only type of control > flow, Functions looking up something will still return pointers, but on the > other hand it's possible those function may want to return error type too.) C is a spartan language. We can struggle against this and try to build imitations of what richer languages provide. That way is Greenspun's tenth. I feel it's best to embrace C's spartan nature. Integer error codes are as spartan as it gets. You can use a common fixed set like POSIX errno codes. The fixed codes will rarely fit exactly, but you can shoehorn them into service often enough. Document how you use them, hold your nose if you must. You can also use function-specific sets, like getaddrinfo()'s EAI_ codes. Try not to invent too many of them. You can try to dream up a way to define an integer error code extension mechanism, but that's not spartan, sorry. A single Error type encapsulating a human-readable message is differently spartan. The spot detecting an error commonly lacks context to know how to handle it, and the spot handling it commonly lacks detail to create a decent error message. This Error type lets the former create an error message, so the latter doesn't have to. Occasionally, you need to handle different errors differently, and then this basic Error type is of no help. Sometimes, you need to because you created a do_everything() function that can consequently fail in every imaginable way. Splitting it up can make the error handling problem go away. But sometimes, it refuses to go away. Then you need to add an error code somehow. Python and many other languages use subtyping to let you add arbitrary data to an error. Plus, since you can match the type itself, you don't need to add an error code, just use the type. Duplicating that in C is not spartan, sorry. Spartans get to add the error code to the error object or return it separately. GLib adds two integer codes: domain and code. This is basically an extensible set of fixed sets. Pushing the limits of spartan, if you ask me. Our Error has an enumeration code ErrorClass. A fundamental design mistake was made early on: each ErrorClass value was tied to a single error message. As if a human readable error message was a grudging concession to human users. The resulting error messages certainly felt like it. We've since servered the connection between ErrorClass value and error message, deprecated ErrorClass, and got rid of most ErrorClass values. In the few places where we need to handle different errors differently, we additionally return an ad hoc error code, commonly -errno. > We used to have rich type errors, which has been abandoned, but I think it > probably makes some sense to let Error carry a standard error code like > EINPROGRESS, ENOTSUP, etc? > > Error *err = NULL; > do_open_tray(..., &err); > if (error_num(err) == EINPROGRESS) { > ... > } else{ > ... > } > > Or should we simply use errno in this case? We could revive ErrorClass, or add something new. Adding errno would be simple enough. Not sure it's worth it.
On 05/18/2016 02:36 AM, Fam Zheng wrote: > On Wed, 05/18 07:36, Markus Armbruster wrote: >> Fam Zheng <famz@redhat.com> writes: >> >>> On Tue, 05/17 20:42, John Snow wrote: >>>> If you use HMP's eject but the CDROM tray is locked, you may get a >>>> confusing error message informing you that the "tray isn't open." >>>> >>>> As this is the point of eject, we can do a little better and help >>>> clarify that the tray was locked and that it (might) open up later, >>>> so try again. >>>> >>>> It's not ideal, but it makes the semantics of the (legacy) eject >>>> command more understandable to end users when they try to use it. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> blockdev.c | 36 +++++++++++++++++++++++++++++------- >>>> 1 file changed, 29 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 1892b8e..feb8484 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -2290,16 +2290,26 @@ exit: >>>> block_job_txn_unref(block_job_txn); >>>> } >>>> >>>> +static int do_open_tray(const char *device, bool has_force, bool force, >>>> + Error **errp); >>>> + >>>> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >>>> { >>>> Error *local_err = NULL; >>>> + int rc; >>>> >>>> - qmp_blockdev_open_tray(device, has_force, force, &local_err); >>>> + rc = do_open_tray(device, has_force, force, &local_err); >>>> if (local_err) { >>>> error_propagate(errp, local_err); >>>> return; >>>> } >>>> >>>> + if (rc == -EINPROGRESS) { >>>> + error_setg(errp, "Device '%s' is locked and force was not specified, " >>>> + "wait for tray to open and try again", device); >>>> + return; >>>> + } >>>> + >>>> qmp_x_blockdev_remove_medium(device, errp); >>>> } >>>> >>>> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, >>>> aio_context_release(aio_context); >>>> } >>>> >>>> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >>>> - Error **errp) >>>> +static int do_open_tray(const char *device, bool has_force, bool force, >>>> + Error **errp) >>> >>> Personally I feel the has_force and force could be merged as one parameter. >> >> For qmp_blockdev_open_tray(), the signature is dictated by >> scripts/qapi-commands.py. To make has_FOO go away, you need to make the >> FOO non-optional. >> >> You have to duplicate the cumbersome has_FOO, FOO couple in your helper >> functions only when an absent value (has_FOO=false) has special meaning > > I was only talking about the helper function, but that is more of a personal > taste thing. > The single solitary reason I didn't squash them for the helper was so that the git diff looked smaller (The new do_eject is functionally identical to the old qmp_blockdev_open_tray.) >> you can't get with any present value. Not my favorite interface design, >> by the way. >> >> We've discussed two improvements to the QAPI language and generators: >> >> * Optional with default: has_FOO goes away, and instead FOO assumes the >> default value declared in the schema when it's absent. Optional >> without default stays at it is, i.e. has_FOO tells whether it's >> present. >> >> * Use null pointer for absent when it can't be a value. >> >> If Eric stops flooding me with QAPI patches, I might even get to >> implement them :) >> >>>> { >>>> BlockBackend *blk; >>>> bool locked; >>>> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >>>> if (!blk) { >>>> error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >>>> "Device '%s' not found", device); >>>> - return; >>>> + return -ENODEV; >>>> } >>>> >>>> if (!blk_dev_has_removable_media(blk)) { >>>> error_setg(errp, "Device '%s' is not removable", device); >>>> - return; >>>> + return -ENOTSUP; >>>> } >>>> >>>> if (!blk_dev_has_tray(blk)) { >>>> /* Ignore this command on tray-less devices */ >>>> - return; >>>> + return -ENOSYS; >>> >>> I'm not sure how acceptable it is to leave errp untouched while setting ret >>> code to non-zero. Markus? >> >> It's questionable style, becaue it gives the two plausible ways to check >> for errors different meaning: >> >> if (do_open_tray(...) < 0) ... >> >> and >> >> Error *err = NULL; >> do_open_tray(..., &err); >> if (err) ... >> >> I find this confusing. >> >> The former way lets me pass a null Error * argument, which is convenient >> when I'm not interested in error details. >> >> Whenever practical, separate an Error-setting function's values into >> distinct error and success sets. Example: when a function looks up >> something, return pointer to it on success, set error and return null on >> failure. >> >> This isn't always practical, for instance, when a pointer-valued >> function can legitimately return null. That causes confusion, too. We >> fixed a few bugs around such functions. >> >> Whether it isn't practical for *this* function I can't say without >> developing a better understanding of its purpose and context. > > We have this question because errp is mostly human oriented, whereas return > codes are also used for control logic. From an error pointer a caller can only > tell if the called function succeeded or not, but cannot tell which type the > failure is. Comparing this to exception handling systems in other OO languages > such as Python, I feel this is because lacking of the type information which > would cover this case if we had one too. With error type information, the > idiom with "ret code + errp" would then become similar to: > > try: > do_open_tray() > except EjectInProgress: > pass > except Exception: > # report error > ... > > And a return code is not needed. (not saying this is the only type of control > flow, Functions looking up something will still return pointers, but on the > other hand it's possible those function may want to return error type too.) > > We used to have rich type errors, which has been abandoned, but I think it > probably makes some sense to let Error carry a standard error code like > EINPROGRESS, ENOTSUP, etc? > > Error *err = NULL; > do_open_tray(..., &err); > if (error_num(err) == EINPROGRESS) { > ... > } else{ > ... > } > > Or should we simply use errno in this case? > > Fam > I can't comment on the historical path that QEMU has taken, but I agree (Naively?) with pretty much everything you've just written. Perhaps global, standardized exceptions is too tall of a task to tackle, but you are right that errors + error codes (or classes) would be useful precisely for situations like these.
On 05/18/2016 01:36 AM, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > >> On Tue, 05/17 20:42, John Snow wrote: >>> If you use HMP's eject but the CDROM tray is locked, you may get a >>> confusing error message informing you that the "tray isn't open." >>> >>> As this is the point of eject, we can do a little better and help >>> clarify that the tray was locked and that it (might) open up later, >>> so try again. >>> >>> It's not ideal, but it makes the semantics of the (legacy) eject >>> command more understandable to end users when they try to use it. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> blockdev.c | 36 +++++++++++++++++++++++++++++------- >>> 1 file changed, 29 insertions(+), 7 deletions(-) >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index 1892b8e..feb8484 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -2290,16 +2290,26 @@ exit: >>> block_job_txn_unref(block_job_txn); >>> } >>> >>> +static int do_open_tray(const char *device, bool has_force, bool force, >>> + Error **errp); >>> + >>> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >>> { >>> Error *local_err = NULL; >>> + int rc; >>> >>> - qmp_blockdev_open_tray(device, has_force, force, &local_err); >>> + rc = do_open_tray(device, has_force, force, &local_err); >>> if (local_err) { >>> error_propagate(errp, local_err); >>> return; >>> } >>> >>> + if (rc == -EINPROGRESS) { >>> + error_setg(errp, "Device '%s' is locked and force was not specified, " >>> + "wait for tray to open and try again", device); >>> + return; >>> + } >>> + >>> qmp_x_blockdev_remove_medium(device, errp); >>> } >>> >>> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, >>> aio_context_release(aio_context); >>> } >>> >>> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >>> - Error **errp) >>> +static int do_open_tray(const char *device, bool has_force, bool force, >>> + Error **errp) >> >> Personally I feel the has_force and force could be merged as one parameter. > > For qmp_blockdev_open_tray(), the signature is dictated by > scripts/qapi-commands.py. To make has_FOO go away, you need to make the > FOO non-optional. > > You have to duplicate the cumbersome has_FOO, FOO couple in your helper > functions only when an absent value (has_FOO=false) has special meaning > you can't get with any present value. Not my favorite interface design, > by the way. > > We've discussed two improvements to the QAPI language and generators: > > * Optional with default: has_FOO goes away, and instead FOO assumes the > default value declared in the schema when it's absent. Optional > without default stays at it is, i.e. has_FOO tells whether it's > present. > > * Use null pointer for absent when it can't be a value. > > If Eric stops flooding me with QAPI patches, I might even get to > implement them :) > >>> { >>> BlockBackend *blk; >>> bool locked; >>> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >>> if (!blk) { >>> error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >>> "Device '%s' not found", device); >>> - return; >>> + return -ENODEV; >>> } >>> >>> if (!blk_dev_has_removable_media(blk)) { >>> error_setg(errp, "Device '%s' is not removable", device); >>> - return; >>> + return -ENOTSUP; >>> } >>> >>> if (!blk_dev_has_tray(blk)) { >>> /* Ignore this command on tray-less devices */ >>> - return; >>> + return -ENOSYS; >> >> I'm not sure how acceptable it is to leave errp untouched while setting ret >> code to non-zero. Markus? > > It's questionable style, becaue it gives the two plausible ways to check > for errors different meaning: > > if (do_open_tray(...) < 0) ... > > and > > Error *err = NULL; > do_open_tray(..., &err); > if (err) ... > > I find this confusing. > > The former way lets me pass a null Error * argument, which is convenient > when I'm not interested in error details. > > Whenever practical, separate an Error-setting function's values into > distinct error and success sets. Example: when a function looks up > something, return pointer to it on success, set error and return null on > failure. > > This isn't always practical, for instance, when a pointer-valued > function can legitimately return null. That causes confusion, too. We > fixed a few bugs around such functions. > > Whether it isn't practical for *this* function I can't say without > developing a better understanding of its purpose and context. > > [...] > Basically, in some contexts certain callers *may* consider certain error/success conditions as an error, and in others they may not. For instance, when using qmp_blockdev_open_tray directly via QMP, it has a few outcomes: 1) Tray is unlocked, force parameter is irrelevant, command succeeds. 2) Tray is locked, force is false, command "fails," but Guest VM is notified of a desire to open the tray and may or may not respect the command. We have "successfully" applied a best effort to opening the tray. 3) Tray is locked, force is true, command succeeds. It's the behavior of #2 that I am trying to clarify here. When used indirectly via qmp_eject, #2 is definitely an error -- the full routine of eject will NOT succeed (we cannot remove the medium) so the error reported to the user should be from Eject's perspective, not medium_remove's. To this end, I thought I'd add error codes into the helper to help callers differentiate hard errors from "soft errors." If this is too iffy for people, I can: - Leave all error codes set to -ERRNO if we set errp, and - Change all "maybe error codes" +ERRNO if we don't set errp. (Eric's suggestion.) Good?
John Snow <jsnow@redhat.com> writes: > On 05/18/2016 01:36 AM, Markus Armbruster wrote: >> Fam Zheng <famz@redhat.com> writes: >> >>> On Tue, 05/17 20:42, John Snow wrote: >>>> If you use HMP's eject but the CDROM tray is locked, you may get a >>>> confusing error message informing you that the "tray isn't open." >>>> >>>> As this is the point of eject, we can do a little better and help >>>> clarify that the tray was locked and that it (might) open up later, >>>> so try again. >>>> >>>> It's not ideal, but it makes the semantics of the (legacy) eject >>>> command more understandable to end users when they try to use it. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> blockdev.c | 36 +++++++++++++++++++++++++++++------- >>>> 1 file changed, 29 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 1892b8e..feb8484 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -2290,16 +2290,26 @@ exit: >>>> block_job_txn_unref(block_job_txn); >>>> } >>>> >>>> +static int do_open_tray(const char *device, bool has_force, bool force, >>>> + Error **errp); >>>> + >>>> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >>>> { >>>> Error *local_err = NULL; >>>> + int rc; >>>> >>>> - qmp_blockdev_open_tray(device, has_force, force, &local_err); >>>> + rc = do_open_tray(device, has_force, force, &local_err); >>>> if (local_err) { >>>> error_propagate(errp, local_err); >>>> return; >>>> } >>>> >>>> + if (rc == -EINPROGRESS) { >>>> + error_setg(errp, "Device '%s' is locked and force was not specified, " >>>> + "wait for tray to open and try again", device); >>>> + return; >>>> + } >>>> + >>>> qmp_x_blockdev_remove_medium(device, errp); >>>> } >>>> >>>> @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, >>>> aio_context_release(aio_context); >>>> } >>>> >>>> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >>>> - Error **errp) >>>> +static int do_open_tray(const char *device, bool has_force, bool force, >>>> + Error **errp) >>> >>> Personally I feel the has_force and force could be merged as one parameter. >> >> For qmp_blockdev_open_tray(), the signature is dictated by >> scripts/qapi-commands.py. To make has_FOO go away, you need to make the >> FOO non-optional. >> >> You have to duplicate the cumbersome has_FOO, FOO couple in your helper >> functions only when an absent value (has_FOO=false) has special meaning >> you can't get with any present value. Not my favorite interface design, >> by the way. >> >> We've discussed two improvements to the QAPI language and generators: >> >> * Optional with default: has_FOO goes away, and instead FOO assumes the >> default value declared in the schema when it's absent. Optional >> without default stays at it is, i.e. has_FOO tells whether it's >> present. >> >> * Use null pointer for absent when it can't be a value. >> >> If Eric stops flooding me with QAPI patches, I might even get to >> implement them :) >> >>>> { >>>> BlockBackend *blk; >>>> bool locked; >>>> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, >>>> if (!blk) { >>>> error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >>>> "Device '%s' not found", device); >>>> - return; >>>> + return -ENODEV; >>>> } >>>> >>>> if (!blk_dev_has_removable_media(blk)) { >>>> error_setg(errp, "Device '%s' is not removable", device); >>>> - return; >>>> + return -ENOTSUP; >>>> } >>>> >>>> if (!blk_dev_has_tray(blk)) { >>>> /* Ignore this command on tray-less devices */ >>>> - return; >>>> + return -ENOSYS; >>> >>> I'm not sure how acceptable it is to leave errp untouched while setting ret >>> code to non-zero. Markus? >> >> It's questionable style, becaue it gives the two plausible ways to check >> for errors different meaning: >> >> if (do_open_tray(...) < 0) ... >> >> and >> >> Error *err = NULL; >> do_open_tray(..., &err); >> if (err) ... >> >> I find this confusing. >> >> The former way lets me pass a null Error * argument, which is convenient >> when I'm not interested in error details. >> >> Whenever practical, separate an Error-setting function's values into >> distinct error and success sets. Example: when a function looks up >> something, return pointer to it on success, set error and return null on >> failure. >> >> This isn't always practical, for instance, when a pointer-valued >> function can legitimately return null. That causes confusion, too. We >> fixed a few bugs around such functions. >> >> Whether it isn't practical for *this* function I can't say without >> developing a better understanding of its purpose and context. >> >> [...] >> > > Basically, in some contexts certain callers *may* consider certain > error/success conditions as an error, and in others they may not. This is an instance of the general pattern: callee knows exactly what went wrong, but not how to handle it. The place that can decide how to handle it is up-stack, where we don't know much about what went wrong anymore. The current code comonly solves it like this. The callee returns an error code and sets an error. Code and error get propagated to the place that can decide. Said place examines the code. It may decide that this isn't an error after all. It probably has no use for the error then, so it frees it. > For instance, when using qmp_blockdev_open_tray directly via QMP, it has > a few outcomes: > > 1) Tray is unlocked, force parameter is irrelevant, command succeeds. > 2) Tray is locked, force is false, command "fails," but Guest VM is > notified of a desire to open the tray and may or may not respect the > command. We have "successfully" applied a best effort to opening the tray. > 3) Tray is locked, force is true, command succeeds. > > It's the behavior of #2 that I am trying to clarify here. > > When used indirectly via qmp_eject, #2 is definitely an error -- the > full routine of eject will NOT succeed (we cannot remove the medium) so > the error reported to the user should be from Eject's perspective, not > medium_remove's. > > To this end, I thought I'd add error codes into the helper to help > callers differentiate hard errors from "soft errors." > > If this is too iffy for people, I can: > > - Leave all error codes set to -ERRNO if we set errp, and > - Change all "maybe error codes" +ERRNO if we don't set errp. (Eric's > suggestion.) > > Good? If both X and -X occur for some value of X, this is confusing. But I guess (hope!) no such X exists. If only one of them can occur, then this is basically using the sign to encode the callee's best guess on what callers may wish to treat as an error. I find that... unusual. Wouldn't do it. I'd leave the decision to the caller, and keep the interface simple.
diff --git a/blockdev.c b/blockdev.c index 1892b8e..feb8484 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2290,16 +2290,26 @@ exit: block_job_txn_unref(block_job_txn); } +static int do_open_tray(const char *device, bool has_force, bool force, + Error **errp); + void qmp_eject(const char *device, bool has_force, bool force, Error **errp) { Error *local_err = NULL; + int rc; - qmp_blockdev_open_tray(device, has_force, force, &local_err); + rc = do_open_tray(device, has_force, force, &local_err); if (local_err) { error_propagate(errp, local_err); return; } + if (rc == -EINPROGRESS) { + error_setg(errp, "Device '%s' is locked and force was not specified, " + "wait for tray to open and try again", device); + return; + } + qmp_x_blockdev_remove_medium(device, errp); } @@ -2327,8 +2337,8 @@ void qmp_block_passwd(bool has_device, const char *device, aio_context_release(aio_context); } -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, - Error **errp) +static int do_open_tray(const char *device, bool has_force, bool force, + Error **errp) { BlockBackend *blk; bool locked; @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, if (!blk) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", device); - return; + return -ENODEV; } if (!blk_dev_has_removable_media(blk)) { error_setg(errp, "Device '%s' is not removable", device); - return; + return -ENOTSUP; } if (!blk_dev_has_tray(blk)) { /* Ignore this command on tray-less devices */ - return; + return -ENOSYS; } if (blk_dev_is_tray_open(blk)) { - return; + return 0; } locked = blk_dev_is_medium_locked(blk); @@ -2366,6 +2376,18 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, if (!locked || force) { blk_dev_change_media_cb(blk, false); } + + if (locked && !force) { + return -EINPROGRESS; + } + + return 0; +} + +void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, + Error **errp) +{ + do_open_tray(device, has_force, force, errp); } void qmp_blockdev_close_tray(const char *device, Error **errp)
If you use HMP's eject but the CDROM tray is locked, you may get a confusing error message informing you that the "tray isn't open." As this is the point of eject, we can do a little better and help clarify that the tray was locked and that it (might) open up later, so try again. It's not ideal, but it makes the semantics of the (legacy) eject command more understandable to end users when they try to use it. Signed-off-by: John Snow <jsnow@redhat.com> --- blockdev.c | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-)