From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPByU-0003p0-Rq for qemu-devel@nongnu.org; Mon, 18 Jul 2016 13:05:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPByP-0006Te-OC for qemu-devel@nongnu.org; Mon, 18 Jul 2016 13:04:57 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:36735) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPByP-0006TU-Et for qemu-devel@nongnu.org; Mon, 18 Jul 2016 13:04:53 -0400 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u6IGx4TR059357 for ; Mon, 18 Jul 2016 13:04:52 -0400 Received: from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109]) by mx0a-001b2d01.pphosted.com with ESMTP id 24827f8bhu-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 18 Jul 2016 13:04:52 -0400 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 18 Jul 2016 18:04:50 +0100 References: <1468845016-43468-1-git-send-email-pasic@linux.vnet.ibm.com> <578CFA57.3080806@linux.vnet.ibm.com> From: Halil Pasic Date: Mon, 18 Jul 2016 19:04:21 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="lXlKXmBGFSc4CMnwiNfKeiS6sxsCiVbAq" Message-Id: <578D0C15.40706@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/1] block: improve error handling in raw_open List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: Kevin Wolf , Cornelia Huck , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --lXlKXmBGFSc4CMnwiNfKeiS6sxsCiVbAq From: Halil Pasic To: Max Reitz , qemu-block@nongnu.org Cc: Kevin Wolf , Cornelia Huck , qemu-devel@nongnu.org Message-ID: <578D0C15.40706@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/1] block: improve error handling in raw_open References: <1468845016-43468-1-git-send-email-pasic@linux.vnet.ibm.com> <578CFA57.3080806@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 07/18/2016 05:57 PM, Max Reitz wrote: > On 18.07.2016 17:48, Halil Pasic wrote: >> >> >> On 07/18/2016 04:41 PM, Max Reitz wrote: >>> On 18.07.2016 14:30, Halil Pasic wrote: >>>> Make raw_open for POSIX more consistent in handling errors by settin= g >>>> the error object also when qemu_open fails. The error object was >>>> generally set in case of errors, but I guess this case was overlooke= d. >>>> Do the same for win32. >>>> >>>> Signed-off-by: Halil Pasic >>>> Reviewed-by: Cornelia Huck >>>> Reviewed-by: Sascha Silbe >>>> Tested-by: Marc Hartmayer (POSIX only)= >>>> >>>> --- >>>> >>>> Stumbled upon this (POSIX) while testing VMs with too many SCSI disk= s in >>>> respect to my nofile limit. When open hits the nofile limit while tr= ying >>>> to hotplug yet another SCSI disk via libvirt we end up with no adequ= ate >>>> error message (one stating too many files). Sadly this patch in not >>>> sufficient to fix this problem because drive_new (/qemu/blockdev.c) >>>> handles errors using error_report_err which is documented as not to = be >>>> used in QMP context. Do not have a patch for that, because I'm unsur= e >>>> whats the best way to deal with it. My guess right now is to make su= re >>>> we propagate errors at least until reaching code which is called on= ly >>>> QMP in context and handle communicating the error to the requester o= f >>>> the operation there. Any suggestions or ideas? >>>> >>>> The win32 part was not tested, and the sole reason I touched it is >>>> to not introduce unnecessary divergence. >>>> --- >>>> block/raw-posix.c | 1 + >>>> block/raw-win32.c | 1 + >>>> 2 files changed, 2 insertions(+) >>>> >>>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>>> index c979ac3..4a7056e 100644 >>>> --- a/block/raw-posix.c >>>> +++ b/block/raw-posix.c >>>> @@ -489,6 +489,7 @@ static int raw_open_common(BlockDriverState *bs,= QDict *options, >>>> if (ret =3D=3D -EROFS) { >>>> ret =3D -EACCES; >>>> } >>>> + error_setg_errno(errp, -ret, "Could not open file"); >>> >>> How about putting this above the "if (ret =3D=3D -EROFS)" block? Whil= e other >>> parts of qemu may want to treat EROFS and EACCES in the same way, I >>> think it makes sense to distinguish both cases in messages meant for = a >>> human user. >>> >>> Max >> >> >> Thanks for the comment! >> >> Have no strong opinion here. AFAIU the errno argument is only used to >> generate a message so there should be no consistency issue, and it wou= ld >> be more consistent with the win32. How about moving both (posix and >> win32) before the conditional statements readjusting the return value >> and use errno and err directly? >=20 > Regarding win32, the issue is that we don't get an errno value but a > Windows-specific error value from GetLastError(). I don't think > error_setg_errno() understands those values. Therefore, for win32 we Of course you are right regarding the nature of the error code for win32.= Was not aware of that :/, so the win32 part was completely broken.=20 > don't have much choice but to use the "preprocessed" errno value. >=20 We could use error_setg_win32 with the return value of GetLastError(). It basically uses=20 https://developer.gnome.org/glib/stable/glib-Windows-Compatibility-Functi= ons.html#g-win32-error-message to get a message string from the error code. Would that be OK with you? > I don't really see a consistency issue. It's just a human-readable erro= r > message and I think we should be as specific as we can be; it's just > that it depends on the OS how much that is. > I agree. Wanted to say the same regarding consistency ;). Sorry if it did= not came across. Thanks again for the catches! Halil =20 > Max >=20 >> >> Cheers, >> Halil >> >>> >>>> goto fail; >>>> } >>>> s->fd =3D fd; >>>> diff --git a/block/raw-win32.c b/block/raw-win32.c >>>> index 62edb1a..f324f4e 100644 >>>> --- a/block/raw-win32.c >>>> +++ b/block/raw-win32.c >>>> @@ -342,6 +342,7 @@ static int raw_open(BlockDriverState *bs, QDict = *options, int flags, >>>> } else { >>>> ret =3D -EINVAL; >>>> } >>>> + error_setg_errno(errp, err, "Could not open file"); >>>> goto fail; >>>> } >>>> =20 >>>> >>> >>> >> >=20 >=20 --lXlKXmBGFSc4CMnwiNfKeiS6sxsCiVbAq Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (GNU/Linux) iQIcBAEBAgAGBQJXjQwtAAoJEA0vhuyXGx0ABNsP/2ys8X8YeXGLqMWJC1GrbMk3 8+0REBYLDb02dUZvxf2AJw4xt87EeKJ8MhQzNvf/AVhgvA7SJ3jNSIjZ5RU5KIRP YjRkONrgUhRxV0qSHrAmPrN7o/Zo9ypcOkcWWOcR4qnOBaJaYQ43zZfUulQlksyo 8oiqjqkQ8sDf+uUCs/TjT6sJ8Y1lkXKoIBXWWG5EkVmsM9+peA3FeixzwbA6bJXx 8Ck2LOBen1f7aNZkeu0brni6gizo0nZqQbnHegjjXG1+UvaPwHYV1PvHrvYctb89 mnvrNDrz2bGv2GZNtrc9z+nQmBHwfVbVO0AzQbswehMBcvpKNlHsb6cFR43VlbwV fqkJ5QuUARCJp8iaZnqLQ0ustXgw0LwMCSgV4dV/1bu7L0jWQWjZyqlPpzVw3I8p AUuQqQIEMqnmEtKXX0C2VFcYwcPLwob4Gh8NUAkxKQAnsR3B76/9vozxLVsgR4Yr CDoshu/2YofHkmBAchp/5SwkIx3+O2mn7CNUIeNiskUtxdRYumHgaIW+nYB+SjCY nHTVwhfdCI7r1nYD62VOGwJKNdIxE9bhvL6Yi1YrAbbTim91bmywZJgC7RJds0UJ MmRUT+a2NwvcxRTEmnfId5zRGHRJBA3tILwqsQWNL3yyl/4ndivp7pp42aBf6tQ/ h5wyL6QCH4DAWI6yrKxS =/rX9 -----END PGP SIGNATURE----- --lXlKXmBGFSc4CMnwiNfKeiS6sxsCiVbAq--