From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58270) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2u9u-0001in-Ou for qemu-devel@nongnu.org; Wed, 18 May 2016 01:36:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b2u9s-00024d-Fw for qemu-devel@nongnu.org; Wed, 18 May 2016 01:36:37 -0400 From: Markus Armbruster References: <1463532149-11625-1-git-send-email-jsnow@redhat.com> <20160518021825.GB1864@ad.usersys.redhat.com> Date: Wed, 18 May 2016 07:36:26 +0200 In-Reply-To: <20160518021825.GB1864@ad.usersys.redhat.com> (Fam Zheng's message of "Wed, 18 May 2016 10:18:25 +0800") Message-ID: <87wpmsq65h.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block: clarify error message for qmp-eject List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: John Snow , qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com Fam Zheng 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 >> --- >> 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. [...]