From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33070) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0uIX-0000k7-Jj for qemu-devel@nongnu.org; Mon, 23 Nov 2015 11:49:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a0uIS-0002DK-2w for qemu-devel@nongnu.org; Mon, 23 Nov 2015 11:49:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37311) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0uIR-0002D2-KG for qemu-devel@nongnu.org; Mon, 23 Nov 2015 11:48:55 -0500 References: <144828831182.22449.10763712892565113940.stgit@localhost> From: Eric Blake Message-ID: <56534371.3000604@redhat.com> Date: Mon, 23 Nov 2015 09:48:49 -0700 MIME-Version: 1.0 In-Reply-To: <144828831182.22449.10763712892565113940.stgit@localhost> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="jPHnjdxIoTW1rD0hTssvpdtuW2pNGNOGh" Subject: Re: [Qemu-devel] [PATCH] [doc] Introduce coding style for errors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Llu=c3=ads_Vilanova?= , qemu-devel@nongnu.org Cc: Stefan Hajnoczi , Thomas Huth , "Dr . David Alan Gilbert" , Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --jPHnjdxIoTW1rD0hTssvpdtuW2pNGNOGh Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/23/2015 07:18 AM, Llu=C3=ADs Vilanova wrote: > Gives some general guidelines for reporting errors in QEMU. This is more than just a doc patch; you are actually proposing new code in the way of &error_now. The commit message must absolutely mention changes like this, or possibly even split this into two changes (document existing behavior, then add new code and its matching docs). By the way, titling your message '[doc]' rather than 'doc:' means that 'git am' will eat the '[doc]' and leave just "Introduce coding style for errors" as the final subject, which doesn't match usual conventions. > +++ b/HACKING > @@ -157,3 +157,55 @@ painful. These are: > * you may assume that integers are 2s complement representation > * you may assume that right shift of a signed integer duplicates > the sign bit (ie it is an arithmetic shift, not a logical shift) > + > +7. Error reporting > + > +QEMU provides two different mechanisms for reporting errors. You shoul= d use one > +of these mechanisms instead of manually reporting them (i.e., do not u= se > +'printf', 'exit' or 'abort'). There are places where calling exit() is okay (when parsing command-line arguments), and where abort() is okay (in code that cannot be reached, such as a dead switch() branch). So I don't know if this sentence should be reworded, or if adding even something as simple as "generally" will weaken it too much. > + > +7.1. Errors in user inputs > + > +QEMU provides the functions in "include/qemu/error-report.h" to report= errors > +related to inputs provided by the user (e.g., command line arguments o= r > +configuration files). > + > +These functions generate error messages with a uniform format that can= reference > +a location on the offending input. > + > +7.2. Other errors > + > +QEMU provides the functions in "include/qapi/error.h" to report other = types of > +errors (i.e., not triggered by command line arguments or configuration= files). > + > +Functions in this header are used to accumulate error messages in an '= Error' > +object, which can be propagated up the call chain where it is finally = reported. > + This part describes existing behavior, > +In its simplest form, you can immediately report an error with: > + > + error_setg(&error_now, "Error with %s", "arguments"); and this is brand new but not mentioned in the commit message. > + > +For convenience, you can also use 'error_setg_errno' and 'error_setg_w= in32' to > +append a message for OS-specific errors, and 'error_setg_file_open' fo= r errors > +when opening files. I'm worried that we may be trying to duplicate too much information, in which case one will get out of sync. Merely pointing to error.h may be good enough for the purposes of HACKING, rather than calling out all the subtleties (leaving that for error.h). > =20 > +/* > + * Pass to error_setg() & friends to immediately show an error. > + * > + * Has the same formatting of #error_abort, but does not terminate QEM= U. Doesn't that make it useful only for warnings? If so, should it automatically prepend "warning: "? Where do you propose using it, for a demonstration of why it is useful? > +++ b/util/error.c > @@ -27,6 +27,7 @@ struct Error > =20 > Error *error_abort; > Error *error_fatal; > +Error *error_now; > =20 > static void error_handle_fatal(Error **errp, Error *err) > { > @@ -62,9 +63,14 @@ static void error_setv(Error **errp, > err->func =3D func; > =20 > error_handle_fatal(errp, err); > - *errp =3D err; > - > - errno =3D saved_errno; > + if (errp =3D=3D &error_now) { > + fprintf(stderr, "Unexpected error in %s() at %s:%d:\n", > + err->func, err->src, err->line); > + error_report_err(err); This lacks timestamping information on the "Unexpected error..." output; is that going to be a problem? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --jPHnjdxIoTW1rD0hTssvpdtuW2pNGNOGh 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWU0NxAAoJEKeha0olJ0Nq57oIAKINr8KcxhaNNE01YOiPsNgb 0r7OITLX00rzBP9pUm316yiOo8mOr8/H2iPGc5D7I32Siw9HLaWcTDksoKw3JLaZ 0icTiLmjeiK7e4Kn/ZWDbZyFuHurY3lWAqZ6wswCyViFlhDs+X/yfi4BUVpT9O6A I4CthwDv7mthpCzlol5KhWz22Ljl4dEKLPPm0sHZT7Pm2EaXuZoVUiu9MTq1nvp5 OlTZa50xEO25Cows90rOht+qoRANEz3rxyVIYkfc8aur6HFP7Rd4bcl09bpoz/6R hEBs/zHYmJK3FeDIO8aK+iV15yk79lP+W5K6np9QByFVEdDuQ1AE/9IJrGlqSEg= =5ayL -----END PGP SIGNATURE----- --jPHnjdxIoTW1rD0hTssvpdtuW2pNGNOGh--