On 12/16/10 12:35, Kevin Wolf wrote: > Am 16.12.2010 12:04, schrieb jes.soren...@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