[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_re
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue() |
Date: |
Tue, 19 Feb 2019 17:00:03 +0100 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Tue 12 Feb 2019 05:28:06 PM CET, Kevin Wolf wrote:
>> 1) Set of child options: the options are removed from the parent's
>> options QDict and are passed to the child with a recursive
>> bdrv_reopen_queue() call. This case was already working fine.
>
> Small addition: This is only allowed if the child was implicitly
> created (i.e. it inherits from the parent we're coming from).
Ok, fixed.
>> 2) Child reference: there's two possibilites here.
>>
>> 2a) Reference to the current child: the child is put in the
>> reopen queue, keeping its current set of options (since this
>> was a child reference there was no way to specify a
>> different set of options).
>
> Why even put it in the reopen queue when nothing is going to change?
>
> Ah, it's because inherited options might change, right?
>
> But if the child did not inherit from this parent, we don't need to
> put it into the queue anyway. The operation should still be allowed.
I updated the comment to clarify that.
>> 2b) Reference to a different BDS: the current child is not put
>> in the reopen queue at all. Passing a reference to a
>> different BDS can be used to replace a child, although at
>> the moment no driver implements this, so it results in an
>> error. In any case, the current child is not going to be
>> reopened (and might in fact disappear if it's replaced)
>
> Do we need to break a possible inheritance relationship between the
> parent and the old child node in this case?
Actually I think it makes sense (but not in this patch). I guess that
should be done in bdrv_set_backing_hd().
>> 4) Missing option: at the moment "backing" is the only case where
>> this can happen. With "blockdev-add", leaving "backing" out
>> means that the default backing file is opened. We don't want to
>> open a new image during reopen, so we require that "backing" is
>> always present. We'll relax this requirement a bit in the next
>> patch.
>
> I think this is only for keep_old_opts == false; otherwise it should
> be treated the same as 2a to avoid breaking compatibility.
Ok, I clarified that too.
>> QLIST_FOREACH(child, &bs->children, next) {
>> - QDict *new_child_options;
>> - char *child_key_dot;
>> + QDict *new_child_options = NULL;
>> + bool child_keep_old = keep_old_opts;
>>
>> /* reopen can only change the options of block devices that were
>> * implicitly created and inherited options. For other (referenced)
>> @@ -3043,13 +3055,31 @@ static BlockReopenQueue
>> *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>> continue;
>> }
>
> The 'continue' in the context just skipped any children that don't
> inherit from this parent. Child options for those will stay unmodified
> in the options dict.
>
> For case 1, we'll later get an error because keys like 'child.foo'
> will still be present and can't be processed by the driver. Implements
> exactly what I commented above.
>
> For case 2, the child isn't put in the reopen queue, and the child
> reference can be used later. Matches my comment as well.
>
> Good.
That's correct.
>> - child_key_dot = g_strdup_printf("%s.", child->name);
>> - qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
>> - qdict_extract_subqdict(options, &new_child_options, child_key_dot);
>> - g_free(child_key_dot);
>> + /* Check if the options contain a child reference */
>> + if (qdict_haskey(options, child->name)) {
>> + const char *childref = qdict_get_try_str(options, child->name);
>> + /*
>> + * The current child must not be reopened if the child
>> + * reference does not point to it.
>> + */
>> + if (g_strcmp0(childref, child->bs->node_name)) {
>
> This is where we would break the inheritance relationship.
>
> Is this the code path that a QNull should take as well? (case 3)
Yes, I updated the comment to mention the null case explicitly.
>> + if (reopen_state->backing_missing) {
>> + error_setg(errp, "backing is missing for '%s'",
>> + reopen_state->bs->node_name);
>> + ret = -EINVAL;
>> + goto error;
>> + }
>
> What about drivers that don't even support backing files?
In practice this doesn't matter much because of the changes in patch 7,
but I think it's a good idea to make it explicit and set backing_missing
only when bs->drv->supports_backing is true (because it doesn't make
sense otherwise).
Berto