From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45276) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3lis-0000yt-0M for qemu-devel@nongnu.org; Fri, 20 May 2016 10:48:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b3lip-0005Zh-R6 for qemu-devel@nongnu.org; Fri, 20 May 2016 10:48:16 -0400 From: Markus Armbruster References: <1463608420-26837-1-git-send-email-jsnow@redhat.com> <1463608420-26837-2-git-send-email-jsnow@redhat.com> Date: Fri, 20 May 2016 16:48:06 +0200 In-Reply-To: <1463608420-26837-2-git-send-email-jsnow@redhat.com> (John Snow's message of "Wed, 18 May 2016 17:53:40 -0400") Message-ID: <87vb283hw9.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 1/1] block: clarify error message for qmp-eject List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com John Snow writes: > 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 [...] > @@ -2327,35 +2340,36 @@ 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) > +/** > + * returns -errno on fatal error, +errno for non-fatal situations. > + * errp will always be set when the return code is negative. > + * May return +ENOSYS if the device has no tray, > + * or +EINPROGRESS if the tray is locked and the guest has been notified. > + */ Returning or testing for positive errno instead of a negative one is a fairly common error. The more we can restrict use of positive errno codes to errno itself, the less likely such errors are. Moreover, I feel fatal vs. non-fatal is not for this function to decide. It's the caller's business. I'd return -errno on any error. If you need this function to also set an error, because it can do a better job than its callers, then set it on any error. If a caller wants to suppress a certain error, it can simply free the error. Clean separation of concerns, and a simpler interface. > +static int do_open_tray(const char *device, bool force, Error **errp) > { > BlockBackend *blk; > bool locked; > > - if (!has_force) { > - force = false; > - } > - > blk = blk_by_name(device); > 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 +2380,21 @@ 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) > +{ > + if (!has_force) { > + force = false; > + } > + do_open_tray(device, force, errp); > } > > void qmp_blockdev_close_tray(const char *device, Error **errp)