From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60628) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzlP9-0003yU-JO for qemu-devel@nongnu.org; Tue, 02 Jun 2015 08:34:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YzlP4-0007zh-L4 for qemu-devel@nongnu.org; Tue, 02 Jun 2015 08:34:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzlP4-0007zY-De for qemu-devel@nongnu.org; Tue, 02 Jun 2015 08:34:46 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 154D0B7CEB for ; Tue, 2 Jun 2015 12:34:46 +0000 (UTC) Message-ID: <556DA2DF.3010809@redhat.com> Date: Tue, 02 Jun 2015 06:34:39 -0600 From: Eric Blake MIME-Version: 1.0 References: <1432815695-31687-1-git-send-email-armbru@redhat.com> <1432815695-31687-6-git-send-email-armbru@redhat.com> <55673FC2.8040804@redhat.com> <878uc2cpk9.fsf@blackfin.pond.sub.org> In-Reply-To: <878uc2cpk9.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="N5v7vsM6Jmpm4qBQdl6EVJ6J0ABRN5tGE" Subject: Re: [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: pbonzini@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --N5v7vsM6Jmpm4qBQdl6EVJ6J0ABRN5tGE Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/02/2015 05:33 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> On 05/28/2015 06:21 AM, Markus Armbruster wrote: >>> Retain the function value for now, to permit selective conversion of >>> its callers. >>> >>> Signed-off-by: Markus Armbruster >>> --- >>> -typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque); >>> +typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Erro= r **errp); >> >> Might be nice to justify in the commit message why swapping the >> arguments of the callback made sense. But doesn't affect code >> correctness, so: >> >> Reviewed-by: Eric Blake >=20 > Not sure what to write. Not even quite sure why I like it better this > way, only that I do. Could be because it puts the object we're > modifying (when we modify any of the argument objects) on the left. That's indeed weak, but it's more justification than nothing, so I'll buy it. >>> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, >>> - void *opaque) >>> + void *opaque, Error **errp) >>> { >>> Location loc; >>> QemuOpts *opts; >>> @@ -1062,7 +1063,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_= opts_loopfunc func, >>> loc_push_none(&loc); >>> QTAILQ_FOREACH(opts, &list->head, next) { >>> loc_restore(&opts->loc); >>> - rc =3D func(opts, opaque); >>> + rc =3D func(opaque, opts, errp); >>> if (rc) { >>> return rc; >>> } >> >> Do you want to enforce that if errp is set, that rc is non-zero, to >> match your contract? Perhaps by assert(!*errp) at this point? But if= >> the return value goes away later in the series in favor of only using >> errp, it may be a moot question. >=20 > assert(!*errp) is incorrect, because callers may pass a null errp to > ignore errors. Could do >=20 > if (rc) { > return rc; > } > assert(!errp || !*errp); >=20 > Catches misbehaving func() only when caller passes non-null errp. To > catcht it always, we'd need to use Error *err here, and > error_propagate(errp, err). I doubt it's worth the trouble. Or, you could do: if (!errp) { errp =3D &error_abort; } =2E.. if (rc) { return rc; } assert(!*errp); and not have to use error_propagate() - either the caller is tracking errors, or the caller passed NULL because they are tracking return value and we can assume that their callback will not raise errors (even on negative returns). But that's a slightly different contract. You're right that the possibility of NULL makes it not as trivial as I first thought, so I'm starting to waffle on whether enforcing the contract is worth the extra lines of code. Anyone else with an opinion? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --N5v7vsM6Jmpm4qBQdl6EVJ6J0ABRN5tGE 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/ iQEcBAEBCAAGBQJVbaLfAAoJEKeha0olJ0NqaO8H/jI528qFtXDgWPpU1XgrmDid 0urh+vfdby3W3oVAys2ikLjbd2pTXeYRwj8FtORDKRgSrUw6iVKWYpXqXbFIBwRL gNR2Mj46uSMrCMkwDkPn3A9zrLFPYiWpYJxQ+79wRLtXfDChxwfC1nCNx6J6RlEW 8fUaw8i+7APIYocPFHBLmKsNmUzGF6qwdSVFv97J6cWe4w/yXZQoTYV0urj0ZFam SThNvWFbIU32xPbwUJ+DAbwMC8Ga/gJHA37rYl4Y3niZ+a0OptXO0TqkJ5aU/i3G koQ1B7djjmq2LMovG4ifUNEQ6cImrcU905sK62tET/XSdjMaJO3QQYl0NJF+YP8= =9c1Z -----END PGP SIGNATURE----- --N5v7vsM6Jmpm4qBQdl6EVJ6J0ABRN5tGE--