[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH] block: clarify error message for q
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH] block: clarify error message for qmp-eject |
Date: |
Wed, 18 May 2016 07:36:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Fam Zheng <address@hidden> 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 <address@hidden>
>> ---
>> 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.
[...]
Re: [Qemu-block] [PATCH] block: clarify error message for qmp-eject, Eric Blake, 2016/05/17