From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHwvP-0002OU-JF for qemu-devel@nongnu.org; Fri, 06 Sep 2013 10:22:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHwvJ-0003A2-8Z for qemu-devel@nongnu.org; Fri, 06 Sep 2013 10:22:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33785) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHwvI-00039h-VX for qemu-devel@nongnu.org; Fri, 06 Sep 2013 10:22:09 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r86EM8xL028138 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 6 Sep 2013 10:22:08 -0400 Date: Fri, 6 Sep 2013 16:22:05 +0200 From: Kevin Wolf Message-ID: <20130906142205.GN2588@dhcp-200-207.str.redhat.com> References: <1378389342-4749-1-git-send-email-mreitz@redhat.com> <1378389342-4749-7-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1378389342-4749-7-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [RFC v2 6/6] bdrv: No silent error message discarding 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 15:55 hat Max Reitz geschrieben: > Using NULL as errp parameters to bdrv_open, bdrv_file_open and > bdrv_create in all block drivers which do not yet implement proper error > propagation is not a good idea, since this now silently discards error > messages. Fix this by using a proper parameter and printing its content > on error (until proper propagation is implemented). > > Signed-off-by: Max Reitz > --- > Note that many error propagation are actually trivial (such as in > raw_bsd), but I intentionally refrained from implementing the propagation > and tenaciously followed the pattern: > Error *local_err = NULL; > foo(&local_err); > if (/*error condition*/) { > qerror_report_err(local_err): > error_free(local_err); > } > This is because the propagation may sometimes be trivial, however, it is > often still driver-specific, therefore this deserves its own patch for > every driver, in my opinion. Also, I think it is easier to review this > way. ;-) Agreed. But this patch should be squashed into the patches introducing the NULL argument. Otherwise I'd need to check if this patch catches all places that were previously introduced. > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 6722771..a9d8019 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -1401,9 +1401,10 @@ static int sd_prealloc(const char *filename) > uint32_t idx, max_idx; > int64_t vdi_size; > void *buf = g_malloc0(SD_DATA_OBJ_SIZE); > + Error *local_err = NULL; > int ret; > > - ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, NULL); > + ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err); > if (ret < 0) { > goto out; > } You didn't feel like printing the message here? :-) Kevin