From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vl23J-0000oX-7s for qemu-devel@nongnu.org; Mon, 25 Nov 2013 14:42:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vl23D-0006JT-8T for qemu-devel@nongnu.org; Mon, 25 Nov 2013 14:42:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:64658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vl23C-0006JL-VA for qemu-devel@nongnu.org; Mon, 25 Nov 2013 14:42:31 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAPJgUTu018287 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 25 Nov 2013 14:42:30 -0500 Message-ID: <5293A840.60002@redhat.com> Date: Mon, 25 Nov 2013 20:42:56 +0100 From: Max Reitz MIME-Version: 1.0 References: <1385136658-16347-1-git-send-email-mreitz@redhat.com> <1385136658-16347-2-git-send-email-mreitz@redhat.com> <20131125134032.GI3009@dhcp-200-207.str.redhat.com> In-Reply-To: <20131125134032.GI3009@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/7] blkdebug: Use errp for read_config() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Fam Zheng , qemu-devel@nongnu.org, Stefan Hajnoczi On 25.11.2013 14:40, Kevin Wolf wrote: > Am 22.11.2013 um 17:10 hat Max Reitz geschrieben: >> Use an Error variable in the read_config() function. >> >> Signed-off-by: Max Reitz >> --- >> block/blkdebug.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/block/blkdebug.c b/block/blkdebug.c >> index 16d2b91..9dfa712 100644 >> --- a/block/blkdebug.c >> +++ b/block/blkdebug.c >> @@ -271,7 +271,7 @@ static void remove_rule(BlkdebugRule *rule) >> g_free(rule); >> } >> =20 >> -static int read_config(BDRVBlkdebugState *s, const char *filename) >> +static int read_config(BDRVBlkdebugState *s, const char *filename, Er= ror **errp) > For a complete conversion, this should become a void function. I wanted to do this at first, but blkdebug_open needs to return -errno=20 again; in the hunk below, -errno from fopen is returned. We could just=20 return some dummy value in blkdebug_open again, but I thought it better=20 to pass the correct value. >> { >> FILE *f; >> int ret; >> @@ -279,11 +279,16 @@ static int read_config(BDRVBlkdebugState *s, con= st char *filename) >> =20 >> f =3D fopen(filename, "r"); >> if (f =3D=3D NULL) { >> + error_setg_errno(errp, errno, "Could not read blkdebug config= file " >> + "'%s'", filename); >> return -errno; >> } >> =20 >> ret =3D qemu_config_parse(f, config_groups, filename); >> if (ret < 0) { >> + error_setg(errp, "Could not parse blkdebug config file '%s'", >> + filename); >> + ret =3D -EINVAL; >> goto fail; >> } > Any reason for adding the file name here without mentioning it in the > commit message? No, not really. I don't know why I added this, the original=20 error_setg_errno() in blkdebug_open didn't have it, so=85 > I'm not completely sure here, but the information is probably redundant= ; > the caller already knows what the config file was. On the command line, > this will lead to error messages such as: > > qemu: -drive file=3Dblkdebug:/path/to/blkdebug.cfg:foo.qcow2: Could not > read blkdebug config file '/path/to/blkdebug.cfg': No such file or > directory Personally, I like such redundant information, but I guess I still need=20 to learn that qemu sometimes differs from my preferences. ;-) >> @@ -370,9 +375,8 @@ static int blkdebug_open(BlockDriverState *bs, QDi= ct *options, int flags, >> /* Read rules from config file */ >> config =3D qemu_opt_get(opts, "config"); >> if (config) { >> - ret =3D read_config(s, config); >> - if (ret < 0) { >> - error_setg_errno(errp, -ret, "Could not read blkdebug con= fig file"); >> + ret =3D read_config(s, config, errp); >> + if (ret) { >> goto fail; >> } >> } > Hm, I see. You actually need to return -errno for bdrv_open(). Then > let's just check if we really want to add the file name in the message. If you don't see the point, I do not either. The original=20 error_setg_errno() didn't have it, so we should leave it that way. Max