| Message ID | 1292497454-32573-2-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > This patch re-factors img_create() moving the code doing the actual > work into block.c where it can be shared with QEMU. This is needed to > be able to create images from QEMU to be used for live snapshots. > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > --- > block.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block.h | 4 ++ > qemu-img.c | 108 +-------------------------------------------- > 3 files changed, 150 insertions(+), 106 deletions(-) > > diff --git a/block.c b/block.c > index b4aaf41..765f9f3 100644 > --- a/block.c > +++ b/block.c > @@ -2758,3 +2758,147 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) > { > return bs->dirty_count; > } > + > +int bdrv_img_create(const char *filename, const char *fmt, > + const char *base_filename, const char *base_fmt, > + char *options, uint64_t img_size, int flags) > +{ > + QEMUOptionParameter *param = NULL, *create_options = NULL; > + QEMUOptionParameter *backing_fmt; > + BlockDriverState *bs = NULL; > + BlockDriver *drv, *proto_drv; > + int ret = 0; > + > + /* Find driver and parse its options */ > + drv = bdrv_find_format(fmt); > + if (!drv) { > + error_report("Unknown file format '%s'", fmt); > + ret = -1; > + goto out; > + } > + > + proto_drv = bdrv_find_protocol(filename); > + if (!proto_drv) { > + error_report("Unknown protocol '%s'", filename); > + ret = -1; > + goto out; > + } > + > + create_options = append_option_parameters(create_options, > + drv->create_options); > + create_options = append_option_parameters(create_options, > + proto_drv->create_options); > + > + /* Create parameter list with default values */ > + param = parse_option_parameters("", create_options, param); > + > + set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size); > + > + /* Parse -o options */ > + if (options) { > + param = parse_option_parameters(options, create_options, param); > + if (param == NULL) { > + error_report("Invalid options for file format '%s'.", fmt); > + ret = -1; > + goto out; > + } > + } > + > + if (base_filename) { > + if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE, > + base_filename)) { > + error_report("Backing file not supported for file format '%s'", > + fmt); > + ret = -1; > + goto out; > + } > + } > + > + backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT); > + if (backing_fmt && backing_fmt->value.s) { > + if (!bdrv_find_format(backing_fmt->value.s)) { > + error_report("Unknown backing file format '%s'", > + backing_fmt->value.s); > + ret = -1; > + goto out; > + } > + } > + > + if (base_fmt) { > + if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) { > + error_report("Backing file format not supported for file " > + "format '%s'", fmt); > + ret = -1; > + goto out; > + } > + } The order is wrong here. If you use -F, the backing format won't be checked. > + > + // The size for the image must always be specified, with one exception: > + // If we are using a backing file, we can obtain the size from there > + if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) { > + QEMUOptionParameter *backing_file = > + get_option_parameter(param, BLOCK_OPT_BACKING_FILE); > + > + if (backing_file && backing_file->value.s) { > + uint64_t size; > + const char *fmt = NULL; > + char buf[32]; > + > + if (backing_fmt && backing_fmt->value.s) { > + fmt = backing_fmt->value.s; > + } > + > + bs = bdrv_new(""); > + if (!bs) { > + error_report("Not enough memory to allocate BlockDriverState"); > + ret = -1; > + goto out; > + } bdrv_new never returns NULL (it's an indirect qemu_malloc call). > + ret = bdrv_open(bs, backing_file->value.s, flags, drv); > + if (ret < 0) { > + error_report("Could not open '%s'", filename); > + ret = -1; > + goto out; > + } > + bdrv_get_geometry(bs, &size); > + size *= 512; > + > + snprintf(buf, sizeof(buf), "%" PRId64, size); > + set_option_parameter(param, BLOCK_OPT_SIZE, buf); > + } else { > + error_report("Image creation needs a size parameter"); > + ret = -1; > + goto out; > + } > + } > + > + printf("Formatting '%s', fmt=%s ", filename, fmt); > + print_option_parameters(param); > + puts(""); > + > + ret = bdrv_create(drv, filename, param); > + free_option_parameters(create_options); > + free_option_parameters(param); These need to be after out: to avoid leaking in error cases. You're basically reverting a87a6721d with this. > + > + if (ret < 0) { > + if (ret == -ENOTSUP) { > + error_report("Formatting or formatting option not supported for " > + "file format '%s'", fmt); > + } else if (ret == -EFBIG) { > + error_report("The image size is too large for file format '%s'", > + fmt); > + } else { > + error_report("%s: error while creating %s: %s", filename, fmt, > + strerror(-ret)); > + } > + } > + > +out: > + if (bs) { > + bdrv_delete(bs); > + } > + if (ret) { > + return 1; > + } Maybe we should better use the usual 0/-errno style. In qemu-img it was the exit code of the program, but now it's a block layer function. Kevin
On 12/16/10 12:35, Kevin Wolf wrote: > Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com: >> + >> + backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT); >> + if (backing_fmt && backing_fmt->value.s) { >> + if (!bdrv_find_format(backing_fmt->value.s)) { >> + error_report("Unknown backing file format '%s'", >> + backing_fmt->value.s); >> + ret = -1; >> + goto out; >> + } >> + } >> + >> + if (base_fmt) { >> + if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) { >> + error_report("Backing file format not supported for file " >> + "format '%s'", fmt); >> + ret = -1; >> + goto out; >> + } >> + } > > The order is wrong here. If you use -F, the backing format won't be checked. Urgh yes, my bad! I had it the other way, but decided to change it - very silly. >> + >> + // The size for the image must always be specified, with one exception: >> + // If we are using a backing file, we can obtain the size from there >> + if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) { >> + QEMUOptionParameter *backing_file = >> + get_option_parameter(param, BLOCK_OPT_BACKING_FILE); >> + >> + if (backing_file && backing_file->value.s) { >> + uint64_t size; >> + const char *fmt = NULL; >> + char buf[32]; >> + >> + if (backing_fmt && backing_fmt->value.s) { >> + fmt = backing_fmt->value.s; >> + } >> + >> + bs = bdrv_new(""); >> + if (!bs) { >> + error_report("Not enough memory to allocate BlockDriverState"); >> + ret = -1; >> + goto out; >> + } > > bdrv_new never returns NULL (it's an indirect qemu_malloc call). I see - this was copied wholesale from qemu-img.c but I'll just remove the error check. >> + ret = bdrv_open(bs, backing_file->value.s, flags, drv); >> + if (ret < 0) { >> + error_report("Could not open '%s'", filename); >> + ret = -1; >> + goto out; >> + } >> + bdrv_get_geometry(bs, &size); >> + size *= 512; >> + >> + snprintf(buf, sizeof(buf), "%" PRId64, size); >> + set_option_parameter(param, BLOCK_OPT_SIZE, buf); >> + } else { >> + error_report("Image creation needs a size parameter"); >> + ret = -1; >> + goto out; >> + } >> + } >> + >> + printf("Formatting '%s', fmt=%s ", filename, fmt); >> + print_option_parameters(param); >> + puts(""); >> + >> + ret = bdrv_create(drv, filename, param); >> + free_option_parameters(create_options); >> + free_option_parameters(param); > > These need to be after out: to avoid leaking in error cases. > > You're basically reverting a87a6721d with this. Whoops - another one of those conflicting ones. It's all Stefan's fault :) >> + >> + if (ret < 0) { >> + if (ret == -ENOTSUP) { >> + error_report("Formatting or formatting option not supported for " >> + "file format '%s'", fmt); >> + } else if (ret == -EFBIG) { >> + error_report("The image size is too large for file format '%s'", >> + fmt); >> + } else { >> + error_report("%s: error while creating %s: %s", filename, fmt, >> + strerror(-ret)); >> + } >> + } >> + >> +out: >> + if (bs) { >> + bdrv_delete(bs); >> + } >> + if (ret) { >> + return 1; >> + } > > Maybe we should better use the usual 0/-errno style. In qemu-img it was > the exit code of the program, but now it's a block layer function. I like errno, I made it behave like this for consistency with the rest of QEMU. In most places we don't seem to like anything but -1/1 for error values. Let me know what you prefer, or alternatively we can change it in a follow-on patch? Cheers, Jes
diff --git a/block.c b/block.c index b4aaf41..765f9f3 100644 --- a/block.c +++ b/block.c @@ -2758,3 +2758,147 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) { return bs->dirty_count; } + +int bdrv_img_create(const char *filename, const char *fmt, + const char *base_filename, const char *base_fmt, + char *options, uint64_t img_size, int flags) +{ + QEMUOptionParameter *param = NULL, *create_options = NULL; + QEMUOptionParameter *backing_fmt; + BlockDriverState *bs = NULL; + BlockDriver *drv, *proto_drv; + int ret = 0; + + /* Find driver and parse its options */ + drv = bdrv_find_format(fmt); + if (!drv) { + error_report("Unknown file format '%s'", fmt); + ret = -1; + goto out; + } + + proto_drv = bdrv_find_protocol(filename); + if (!proto_drv) { + error_report("Unknown protocol '%s'", filename); + ret = -1; + goto out; + } + + create_options = append_option_parameters(create_options, + drv->create_options); + create_options = append_option_parameters(create_options, + proto_drv->create_options); + + /* Create parameter list with default values */ + param = parse_option_parameters("", create_options, param); + + set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size); + + /* Parse -o options */ + if (options) { + param = parse_option_parameters(options, create_options, param); + if (param == NULL) { + error_report("Invalid options for file format '%s'.", fmt); + ret = -1; + goto out; + } + } + + if (base_filename) { + if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE, + base_filename)) { + error_report("Backing file not supported for file format '%s'", + fmt); + ret = -1; + goto out; + } + } + + backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT); + if (backing_fmt && backing_fmt->value.s) { + if (!bdrv_find_format(backing_fmt->value.s)) { + error_report("Unknown backing file format '%s'", + backing_fmt->value.s); + ret = -1; + goto out; + } + } + + if (base_fmt) { + if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) { + error_report("Backing file format not supported for file " + "format '%s'", fmt); + ret = -1; + goto out; + } + } + + // The size for the image must always be specified, with one exception: + // If we are using a backing file, we can obtain the size from there + if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) { + QEMUOptionParameter *backing_file = + get_option_parameter(param, BLOCK_OPT_BACKING_FILE); + + if (backing_file && backing_file->value.s) { + uint64_t size; + const char *fmt = NULL; + char buf[32]; + + if (backing_fmt && backing_fmt->value.s) { + fmt = backing_fmt->value.s; + } + + bs = bdrv_new(""); + if (!bs) { + error_report("Not enough memory to allocate BlockDriverState"); + ret = -1; + goto out; + } + ret = bdrv_open(bs, backing_file->value.s, flags, drv); + if (ret < 0) { + error_report("Could not open '%s'", filename); + ret = -1; + goto out; + } + bdrv_get_geometry(bs, &size); + size *= 512; + + snprintf(buf, sizeof(buf), "%" PRId64, size); + set_option_parameter(param, BLOCK_OPT_SIZE, buf); + } else { + error_report("Image creation needs a size parameter"); + ret = -1; + goto out; + } + } + + printf("Formatting '%s', fmt=%s ", filename, fmt); + print_option_parameters(param); + puts(""); + + ret = bdrv_create(drv, filename, param); + free_option_parameters(create_options); + free_option_parameters(param); + + if (ret < 0) { + if (ret == -ENOTSUP) { + error_report("Formatting or formatting option not supported for " + "file format '%s'", fmt); + } else if (ret == -EFBIG) { + error_report("The image size is too large for file format '%s'", + fmt); + } else { + error_report("%s: error while creating %s: %s", filename, fmt, + strerror(-ret)); + } + } + +out: + if (bs) { + bdrv_delete(bs); + } + if (ret) { + return 1; + } + return 0; +} diff --git a/block.h b/block.h index 78ecfac..b812172 100644 --- a/block.h +++ b/block.h @@ -227,6 +227,10 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, int64_t pos, int size); +int bdrv_img_create(const char *filename, const char *fmt, + const char *base_filename, const char *base_fmt, + char *options, uint64_t img_size, int flags); + #define BDRV_SECTORS_PER_DIRTY_CHUNK 2048 void bdrv_set_dirty_tracking(BlockDriverState *bs, int enable); diff --git a/qemu-img.c b/qemu-img.c index 1d936ed..603bdb3 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -287,9 +287,6 @@ static int img_create(int argc, char **argv) const char *base_fmt = NULL; const char *filename; const char *base_filename = NULL; - BlockDriver *drv, *proto_drv; - QEMUOptionParameter *param = NULL, *create_options = NULL; - QEMUOptionParameter *backing_fmt = NULL; char *options = NULL; for(;;) { @@ -350,110 +347,9 @@ static int img_create(int argc, char **argv) goto out; } - /* Find driver and parse its options */ - drv = bdrv_find_format(fmt); - if (!drv) { - error("Unknown file format '%s'", fmt); - ret = -1; - goto out; - } - - proto_drv = bdrv_find_protocol(filename); - if (!proto_drv) { - error("Unknown protocol '%s'", filename); - ret = -1; - goto out; - } - - create_options = append_option_parameters(create_options, - drv->create_options); - create_options = append_option_parameters(create_options, - proto_drv->create_options); - - /* Create parameter list with default values */ - param = parse_option_parameters("", create_options, param); - - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size); - - /* Parse -o options */ - if (options) { - param = parse_option_parameters(options, create_options, param); - if (param == NULL) { - error("Invalid options for file format '%s'.", fmt); - ret = -1; - goto out; - } - } - - /* Add old-style options to parameters */ - ret = add_old_style_options(fmt, param, base_filename, base_fmt); - if (ret < 0) { - goto out; - } - - backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT); - if (backing_fmt && backing_fmt->value.s) { - if (!bdrv_find_format(backing_fmt->value.s)) { - error("Unknown backing file format '%s'", - backing_fmt->value.s); - ret = -1; - goto out; - } - } - - // The size for the image must always be specified, with one exception: - // If we are using a backing file, we can obtain the size from there - if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) { - - QEMUOptionParameter *backing_file = - get_option_parameter(param, BLOCK_OPT_BACKING_FILE); - - if (backing_file && backing_file->value.s) { - BlockDriverState *bs; - uint64_t size; - const char *fmt = NULL; - char buf[32]; - - if (backing_fmt && backing_fmt->value.s) { - fmt = backing_fmt->value.s; - } - - bs = bdrv_new_open(backing_file->value.s, fmt, BDRV_O_FLAGS); - if (!bs) { - ret = -1; - goto out; - } - bdrv_get_geometry(bs, &size); - size *= 512; - bdrv_delete(bs); - - snprintf(buf, sizeof(buf), "%" PRId64, size); - set_option_parameter(param, BLOCK_OPT_SIZE, buf); - } else { - error("Image creation needs a size parameter"); - ret = -1; - goto out; - } - } - - printf("Formatting '%s', fmt=%s ", filename, fmt); - print_option_parameters(param); - puts(""); - - ret = bdrv_create(drv, filename, param); - - if (ret < 0) { - if (ret == -ENOTSUP) { - error("Formatting or formatting option not supported for file format '%s'", fmt); - } else if (ret == -EFBIG) { - error("The image size is too large for file format '%s'", fmt); - } else { - error("%s: error while creating %s: %s", filename, fmt, strerror(-ret)); - } - } + ret = bdrv_img_create(filename, fmt, base_filename, base_fmt, + options, img_size, BDRV_O_FLAGS); out: - free_option_parameters(create_options); - free_option_parameters(param); if (ret) { return 1; }