From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36047) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b32Lt-00065M-DO for qemu-devel@nongnu.org; Wed, 18 May 2016 10:21:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b32Lp-00015R-5p for qemu-devel@nongnu.org; Wed, 18 May 2016 10:21:32 -0400 References: <1463532149-11625-1-git-send-email-jsnow@redhat.com> <20160518021825.GB1864@ad.usersys.redhat.com> <87wpmsq65h.fsf@dusky.pond.sub.org> From: John Snow Message-ID: <573C7A63.7030607@redhat.com> Date: Wed, 18 May 2016 10:21:23 -0400 MIME-Version: 1.0 In-Reply-To: <87wpmsq65h.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Markus Armbruster , Fam Zheng Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com On 05/18/2016 01:36 AM, Markus Armbruster wrote: > 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. > > [...] > 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?