Re: [PATCH] block/rbd: support driver-specific reopen

[Raphael Pour]

On 7/1/22 11:41, Hanna Reitz wrote:
> On 13.04.22 14:26, Raphael Pour wrote:
>  >>       }
> > -    return ret;
> > +    /*
> > +     * Remove all keys from the generic layer which
> > +     * can't be converted by rbd
> > +     * >
> Does any other driver do this?  Shouldn’t we leave them there so that 
> the generic layer can verify that they aren’t changed?
>
> > +    qdict_del(state->options, "driver");
> > +    qdict_del(state->options, "node-name");
> > +    qdict_del(state->options, "auto-read-only");
> > +    qdict_del(state->options, "discard");
> > +    qdict_del(state->options, "cache");
>
> Because AFAIU this would mean that users could attempt to change e.g. 
> the @cache option, and wouldn’t receive an error back, even though there 
> is no support for changing it.
>
> > +
> > +    /*
> > +     * To maintain the compatibility prior the rbd-reopen,
> > +     * where the generic layer can be altered without any
> > +     * rbd argument given,
>
> What does “the generic layer can be altered” mean?  As far as I 
> understand, it was only possible to change between read/write and 
> read-only access.
>
> > +
> > +    keypairs = g_strdup(qdict_get_try_str(state->options, 
> > "=keyvalue-pairs"));
> > +    if (keypairs) {
> > +        qdict_del(state->options, "=keyvalue-pairs");
> > +    }
> > +
> > +    secretid = g_strdup(qdict_get_try_str(state->options, 
> > "password-secret"));
> > +    if (secretid) {
> > +        qdict_del(state->options, "password-secret");
> > +    }
> > +
> > +    r = qemu_rbd_convert_options(state->options, &opts, &local_err);
> > +    if (local_err) {
> > +        /*
> > +         * If keypairs are present, that means some options are 
> > present in
> > +         * the modern option format.  Don't attempt to parse legacy 
> > option
> > +         * formats, as we won't support mixed usage.
> > +         */
> > +        if (keypairs) {
> > +            error_propagate(errp, local_err);
> > +            goto out;
> > +        }
> > +
> > +        /*
> > +         * If the initial attempt to convert and process the options 
> > failed,
> > +         * we may be attempting to open an image file that has the 
> > rbd options
> > +         * specified in the older format consisting of all key/value 
> > pairs
> > +         * encoded in the filename.  Go ahead and attempt to parse the
> > +         * filename, and see if we can pull out the required options.
> > +         */
> > +        r = qemu_rbd_attempt_legacy_options(state->options, &opts, 
> > &keypairs);
> > +        if (r < 0) {
> > +            /*
> > +             * Propagate the original error, not the legacy parsing 
> > fallback
> > +             * error, as the latter was just a best-effort attempt.
> > +             */
> > +            error_propagate(errp, local_err);
> > +            goto out;
> > +        }
> > +        /*
> > +         * Take care whenever deciding to actually deprecate; once 
> > this ability
> > +         * is removed, we will not be able to open any images with 
> > legacy-styled
> > +         * backing image strings.
> > +         */
> > +        warn_report("RBD options encoded in the filename as keyvalue 
> > pairs "
> > +                    "is deprecated");
> > +    }
> > +
> > +    /*
> > +     * Remove the processed options from the QDict (the visitor 
> > processes
> > +     * _all_ options in the QDict)
> > +     */
> > +    while ((e = qdict_first(state->options))) {
> > +        qdict_del(state->options, e->key);
> > +    }
>
> OK, I see why you’d then want to remove all non-RBD options before this
> point.  Other block drivers seem to solve this by having an
> X_runtime_opts QemuOptsList, which contains all driver-handled options,
> so they can then use qemu_opts_absorb_qdict() to extract the options
> they can handle.  (Compare e.g. qcow2_update_options_prepare().)
>

Looking through all the drivers, rbd seems to be the only one not 
absorbing its options via runtime_opts but instead using 
qemu_rbd_convert_options. The converter visits all the options from 
BlockdevOptionsRbd defined in block-core.json. If there is an unknown 
option the conversion fails with EINVAL.

The runtime_opts in contrast to the already defined json schema with the 
visitor-code-generation seem to be redundant. Do you have some insights 
why and when this redundancy is reasonable?

I came up with several alternatives:

1. add own runtime_opts:
  - pro: "the qemu way", it behaves like most of the drivers
  - con: redundancy to qemu_rbd_convert_options which does the same 
except skipping the generic block layer options and adds +1k lines
  - con: I couldn't figure out how to add non-primitive options to the 
runtime_opts like encrypt or server
2. tell visitor to ignore unknown keys (?)
3. parse rbd options manually (opposite of deleting the generic block keys)

I agree deleting the generic block opts isn't optimal.

What do you think?

Your remaining points are also reasonable and I'll submit their fix 
along with the solution to the issue above.

OpenPGP_0xCDB1EBB785C5EB7E.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature