From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHWzG-0006fk-Gh for qemu-devel@nongnu.org; Thu, 05 Sep 2013 06:40:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHWzA-0000FJ-3V for qemu-devel@nongnu.org; Thu, 05 Sep 2013 06:40:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56645) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHWz9-0000FF-SM for qemu-devel@nongnu.org; Thu, 05 Sep 2013 06:40:24 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r85AeMuq023673 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 5 Sep 2013 06:40:22 -0400 Date: Thu, 5 Sep 2013 12:40:20 +0200 From: Kevin Wolf Message-ID: <20130905104019.GD2826@dhcp-200-207.str.redhat.com> References: <1378368620-22682-1-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1378368620-22682-1-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [RFC 0/3] block: Error parameter for opening/creating images List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 05.09.2013 um 10:10 hat Max Reitz geschrieben: > This RFC adds an Error ** parameter to bdrv_open, bdrv_file_open, > bdrv_create and the respective functions provided by a block driver. > > This results in more specific error information than just -errno provided > to the user when opening or creating images (disregarding the fact that > block drivers often already use error_report, which is generally changed > to error_setg through this patch). > > The last patch in this series changes the qcow2 block driver to set an > example of usage in a block driver. > > Note that several I/O tests break by applying this RFC since they expect > different error messages (generally, previously, an error message on > image opening/creation consisted of two lines; the first of which would be > generated by the driver through error_report, the second by the block > layer itself through strerror(-ret); this patch is designed to merge these > two lines into a single one). This applies to the tests 49, 51, 54 and 60. > > Max Reitz (3): > bdrv: Use "Error" for opening images > block: Error parameter for opening functions > qcow2: Use Error parameter One thing I'd like to suggest is to keep the conversions of bdrv_(file_)open and bdrv_create separate. I don't think there are dependencies between them, so two logical changes should come in two separate patches. The other point is that you do a lot of "dummy" conversions where you pass NULL as errp. This is generally not what we want to have in the end because it swallows error messages that would previously be printed by error_report(). I guess most of them are changed into real error handling, but it's hard to keep track of the places that need to be fixed in later patches. In order to address this, I think it would be better to use code like this when you can't forward the errors yet: Error *local_err; foo(&local_err); if (error_is_set(&local_err)) { qerror_report_err(local_err); error_free(local_err); } So that the output isn't lost in any intermediate commit of your series. Kevin