Re: [RFC PATCH 1/2] block: Allow changing bs->file on reopen

[Alberto Garcia <berto@igalia.com>]

On Mon 18 Jan 2021 11:15:17 AM CET, Vladimir Sementsov-Ogievskiy wrote:
>> +static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state,
>> +                                  GSList **tran,
>> +                                  Error **errp)
>> +{
>> +    BlockDriverState *bs = reopen_state->bs;
>> +    BlockDriverState *new_file_bs;
>> +    QObject *value;
>> +    const char *str;
>> +
>> +    value = qdict_get(reopen_state->options, "file");
>> +    if (value == NULL) {
>> +        return 0;
>> +    }
>> +
>> +    /* The 'file' option only allows strings */
>> +    assert(qobject_type(value) == QTYPE_QSTRING);
>> +
>> +    str = qobject_get_try_str(value);
>> +    new_file_bs = bdrv_lookup_bs(NULL, str, errp);
>> +    if (new_file_bs == NULL) {
>> +        return -EINVAL;
>> +    } else if (bdrv_recurse_has_child(new_file_bs, bs)) {
>> +        error_setg(errp, "Making '%s' a file of '%s' "
>> +                   "would create a cycle", str, bs->node_name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    assert(bs->file && bs->file->bs);
>
> why are we sure at this point? Probably, we should just return an
> error..

Unlike 'backing', 'file' is a BlockdevRef and it is not optional, so
block devices that accept that parameter must have it set.

>> +    /* At the moment only backing links are frozen */
>> +    assert(!bs->file->frozen);
>
> I think it can: file-child based filters can be a part of frozen
> backing chain currently.

You're right, since 7b99a26600e bdrv_freeze_backing_chain() uses
bdrv_filter_or_cow_child().

Berto