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 <mre...@redhat.com>
> ---
> 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
