>>> On 4/23/2014 at 02:44 PM, in message <53576153.6CE : 102 : 21807>, Chun Yan
>>> Liu
wrote:
>
>>>> On 4/22/2014 at 07:17 AM, in message <5355a71e.2010...@redhat.com>, Eric
>>>> Blake
> <ebl...@redhat.com> wrote:
> > On 04/10/2014 11:54 AM, Chunyan Liu wrote:
>> > Change block layer to support both QemuOpts and QEMUOptionParameter.
>> > After this patch, it will change backend drivers one by one. At the end,
>> > QEMUOptionParameter will be removed and only QemuOpts is kept.
>> >
>> > Signed-off-by: Dong Xu Wang <wdon...@linux.vnet.ibm.com>
>> > Signed-off-by: Chunyan Liu <cy...@suse.com>
>> > ---
>>
>> > @@ -419,8 +420,27 @@ static void coroutine_fn bdrv_create_co_entry(void
>> *opaque)
>> >
>> > CreateCo *cco = opaque;
>>
>> > + if (cco->drv->bdrv_create2) {
>> > + QemuOptsList *opts_list = NULL;
>> > + QemuOpts *opts = NULL;
>> > + if (!cco->opts) {
>>
>> Isn't your conversion pair-wise per driver, in that you always pair
>> bdrv_create2 with options, and bdrv_create with opts? That is, won't
>> cco->opts always be false if cco->drv->bdrv_create2 is non-NULL, since
>> we already guaranteed that there is at most one of the two creation
>> function callbacks defined? Maybe you have a missing assertion at the
>> point where the callbacks are registered? [1]
>
> To handle both QEMUOptionParameter and QemuOpts, we convert
> QEMUOptionParameter to QemuOpts first for unified processing.
> And according to v22 discussion, it's better to convert back to
> QEMUOptionParameter at the last moment.
> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02181.html.
> For bdrv_create, either do this in bdrv_create(), or here in
> bdrv_create_co_entry(). bdrv_create() just calls bdrv_create_co_entry
> to do the work. Do you think we should finish conversion in bdrv_create()
> before calling bdrv_create_co_entry()?
>
Eric, how do you think of this? Do we need a change?
>>
>> > + opts_list = params_to_opts(cco->options);
>> > + cco->opts = opts =
>> > + qemu_opts_create(opts_list, NULL, 0, &error_abort);
>>
>> Ouch. This assigns to cco->opts,...
>>
>> > + }
>> > + ret = cco->drv->bdrv_create2(cco->filename, cco->opts,
>> > &local_err);
>> > + qemu_opts_del(opts);
>>
>> ...but this frees the memory. You have modified the caller's opaque
>> pointer to point to what is now stale memory. Is this intentional? [2]
>
> Yes. My intension is to avoid deleting cco->opts errorly, only the
> converted
> one from cco->options should be deleted, so I use a temp variable to
> retrieve
> the new allocated opts, just new and delete the temp variable is OK.
>
> The work could also be done as:
>
> if (cco->options) {
> opts_list = params_to_opts(cco->options);
> cco->opts =
> qemu_opts_create(opts_list, NULL, 0, &error_abort);
> }
> ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err);
> if (cco->options) {
> qemu_opts_del(cco->opts);
> qemu_opts_free(opts_list);
> }
>
And here. Should we change it to this way?
I'll update and prepare a new version.
Regards,
Chunyan
>>
>> > + qemu_opts_free(opts_list);
>> > + } else {
>> > + QEMUOptionParameter *options = NULL;
>> > + if (!cco->options) {
>> > + cco->options = options = opts_to_params(cco->opts);
>> > + }
>> > + ret = cco->drv->bdrv_create(cco->filename, cco->options,
> &local_err);
>> > + free_option_parameters(options);
>>
>> Same question [2] in the opposite direction for cco->options.
>>
>>
>>
>> > @@ -5296,28 +5328,25 @@ void bdrv_img_create(const char *filename, const
>> char *fmt,
>>
>> > /* Parse -o options */
>> > if (options) {
>> > - param = parse_option_parameters(options, create_options, param);
>> > - if (param == NULL) {
>> > + if (qemu_opts_do_parse(opts, options, NULL) != 0) {
>> > error_setg(errp, "Invalid options for file format '%s'.",
>> fmt);
>>
>> Pre-existing (and probably worth an independent patch to qemu-trivial),
>> but error messages typically should not end in '.'
>
> Will add new patch for that.
>
>>
>> > +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter
>> > *options,
>> > + QemuOpts *opts)
>> > {
>> > - if (bs->drv->bdrv_amend_options == NULL) {
>> > + int ret;
>> > + assert(!(options && opts));
>> > +
>> > + if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
>>
>> Hmm, should we also guarantee that at most one of these callback
>> functions exist? [1]
>>
>> > return -ENOTSUP;
>> > }
>> > - return bs->drv->bdrv_amend_options(bs, options);
>> > + if (bs->drv->bdrv_amend_options2) {
>> > + QemuOptsList *tmp_opts_list = NULL;
>> > + QemuOpts *tmp_opts = NULL;
>> > + if (options) {
>> > + tmp_opts_list = params_to_opts(options);
>> > + opts = tmp_opts =
>> > + qemu_opts_create(tmp_opts_list, NULL, 0, &error_abort);
>> > + }
>> > + ret = bs->drv->bdrv_amend_options2(bs, opts);
>> > + qemu_opts_del(tmp_opts);
>>
>> Why do you need tmp_opts? Isn't manipulation of 'opts' sufficient here?
>
> Same as answer to [2]. The intension is to avoid deleting opts errorly.
>
>>
>> > +++ b/include/block/block_int.h
>> > @@ -118,6 +118,8 @@ struct BlockDriver {
>> > void (*bdrv_rebind)(BlockDriverState *bs);
>> > int (*bdrv_create)(const char *filename, QEMUOptionParameter
>> > *options,
>> > Error **errp);
>> > + /* FIXME: will remove the duplicate and rename back to bdrv_create
>> later */
>> > + int (*bdrv_create2)(const char *filename, QemuOpts *opts, Error
>> **errp);
>> > int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
>> > int (*bdrv_make_empty)(BlockDriverState *bs);
>> > /* aio */
>> > @@ -217,7 +219,10 @@ struct BlockDriver {
>> >
>> > /* List of options for creating images, terminated by name == NULL */
>> >
>> > QEMUOptionParameter *create_options;
>> > -
>> > + /* FIXME: will replace create_options.
>> > + * These two fields are mutually exclusive. At most one is non-NULL.
>> > + */
>> > + QemuOptsList *create_opts;
>>
>> Do you also want to mention that create_options may only be set with
>> bdrv_create, and that create_opts may only be set with bdrv_create2? [1]
>
> Will add description.
>
>>
>>
>> > @@ -1340,40 +1340,36 @@ static int img_convert(int argc, char **argv)
>>
>> >
>> > - if (options) {
>> > - param = parse_option_parameters(options, create_options, param);
>> > - if (param == NULL) {
>> > - error_report("Invalid options for file format '%s'.",
>> out_fmt);
>> > - ret = -1;
>> > - goto out;
>> > - }
>> > - } else {
>> > - param = parse_option_parameters("", create_options, param);
>> > + opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
>> > + if (options && qemu_opts_do_parse(opts, options, NULL)) {
>> > + error_report("Invalid options for file format '%s'.", out_fmt);
>>
>> Pre-existing, but another instance of trailing '.' in the error message.
>>
>> Back to [1], rather than asserting in individual functions at use time,
>> why not do all the sanity checking up front in bdrv_register()? That
>> is, I think it is easier to add:
>>
>> if (bdrv->bdrv_create) {
>> assert(!bdrv->bdrv_create2);
>> assert(!bdrv->opts && !bdrv->bdrv_amend_options2);
>> } else if (bdrv_{
>> assert(!bdrv->options && !bdrv->bdrv_amend_options);
>> }
>>
>> at the point where drivers are registered, rather than having to
>> duplicate checks across all points that might use a driver.
>
> That makes sense. But [1] still needs the asserting individually, since
> it doesn't use bdrv->create_opts or bdrv->create_options, but the opts or
> options passed in.
>
>>
>> --
>> Eric Blake eblake redhat com +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>>
>>
>
