From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHwM3-0003XF-U9 for qemu-devel@nongnu.org; Fri, 06 Sep 2013 09:45:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHwLx-0007H5-QA for qemu-devel@nongnu.org; Fri, 06 Sep 2013 09:45:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36787) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHwLx-0007H1-HQ for qemu-devel@nongnu.org; Fri, 06 Sep 2013 09:45:37 -0400 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 r86DjZFM012573 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 6 Sep 2013 09:45:36 -0400 Date: Fri, 6 Sep 2013 15:45:32 +0200 From: Kevin Wolf Message-ID: <20130906134532.GL2588@dhcp-200-207.str.redhat.com> References: <1378389342-4749-1-git-send-email-mreitz@redhat.com> <1378389342-4749-5-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-5-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [RFC v2 4/6] block: Error parameter for create functions 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: > Add an Error ** parameter to bdrv_create and its associated functions to > allow more specific error messages. > > Signed-off-by: Max Reitz > --- > block.c | 72 +++++++++++++++++++++++++++++++++------------------ > block/cow.c | 2 +- > block/qcow.c | 2 +- > block/qcow2.c | 2 +- > block/qed.c | 2 +- > block/raw_bsd.c | 2 +- > block/vvfat.c | 2 +- > include/block/block.h | 5 ++-- > qemu-img.c | 16 ++++-------- > 9 files changed, 61 insertions(+), 44 deletions(-) > @@ -1053,11 +1078,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > drv->format_name); > } > > - ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options); > + ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err); > free_option_parameters(create_options); > if (ret < 0) { > - error_setg_errno(errp, -ret, "Could not create temporary overlay " > - "'%s'", tmp_filename); > goto fail; > } I think this message adds value, because a message that makes sense for a caller of bdrv_create(), doesn't necessarily make sense for a bdrv_open() caller without further context (like that it's about the temporary overlay). I guess you should do something along the lines of: error_setg_errno(errp, -ret, "Could not create temporary overlay " "'%s': %s", tmp_filename, error_get_pretty(local_err)); error_free(local_err); local_err = NULL; > @@ -4553,6 +4576,7 @@ void bdrv_img_create(const char *filename, const char *fmt, > BlockDriverState *bs = NULL; > BlockDriver *drv, *proto_drv; > BlockDriver *backing_drv = NULL; > + Error *local_err = NULL; > int ret = 0; > > /* Find driver and parse its options */ > @@ -4639,10 +4663,8 @@ void bdrv_img_create(const char *filename, const char *fmt, > bs = bdrv_new(""); > > ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags, > - backing_drv, NULL); > + backing_drv, &local_err); > if (ret < 0) { > - error_setg_errno(errp, -ret, "Could not open '%s'", > - backing_file->value.s); Same thing about context of the error message here. > goto out; > } > bdrv_get_geometry(bs, &size); > @@ -4661,22 +4683,19 @@ void bdrv_img_create(const char *filename, const char *fmt, > print_option_parameters(param); > puts(""); > } > - ret = bdrv_create(drv, filename, param); > - if (ret < 0) { > - if (ret == -ENOTSUP) { > - error_setg(errp,"Formatting or formatting option not supported for " > - "file format '%s'", fmt); > - } else if (ret == -EFBIG) { > - const char *cluster_size_hint = ""; > - if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) { > - cluster_size_hint = " (try using a larger cluster size)"; > - } > - error_setg(errp, "The image size is too large for file format '%s'%s", > - fmt, cluster_size_hint); > - } else { > - error_setg(errp, "%s: error while creating %s: %s", filename, fmt, > - strerror(-ret)); > + ret = bdrv_create(drv, filename, param, &local_err); > + if (ret == -EFBIG) { > + /* This is generally a better message than whatever the driver would > + * deliver (especially because of the cluster_size_hint), since that > + * is most probably not much different from "image too large". */ > + const char *cluster_size_hint = ""; > + if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) { > + cluster_size_hint = " (try using a larger cluster size)"; > } > + error_setg(errp, "The image size is too large for file format '%s'" > + "%s", fmt, cluster_size_hint); > + error_free(local_err); > + local_err = NULL; > } This should eventually be moved into the block drivers as well, but okay with me for now. Kevin