[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 06/21] block: Exclude nested options only for
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2 06/21] block: Exclude nested options only for children in append_open_options() |
Date: |
Mon, 30 Nov 2015 17:13:45 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
On 30.11.2015 10:01, Kevin Wolf wrote:
> Am 27.11.2015 um 18:58 hat Max Reitz geschrieben:
>> On 23.11.2015 16:59, Kevin Wolf wrote:
>>> Some drivers have nested options (e.g. blkdebug rule arrays), which
>>> don't belong to a child node and shouldn't be removed. Don't remove all
>>> options with "." in their name, but check for the complete prefixes of
>>> actually existing child nodes.
>>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>> block.c | 19 +++++++++++++++----
>>> include/block/block_int.h | 1 +
>>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> Thanks, now I don't need to fix it myself. :-)
>>
>> (I would have had to do that for an in-work series of mine)
>>
>>> diff --git a/block.c b/block.c
>>> index 23d9e10..02125e2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const
>>> char **pfilename,
>>>
>>> static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>>> BlockDriverState *child_bs,
>>> + const char *child_name,
>>> const BdrvChildRole *child_role)
>>> {
>>> BdrvChild *child = g_new(BdrvChild, 1);
>>> *child = (BdrvChild) {
>>> .bs = child_bs,
>>> + .name = child_name,
>>> .role = child_role,
>>> };
>>>
>>> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
>>> BlockDriverState *backing_hd)
>>> bs->backing = NULL;
>>> goto out;
>>> }
>>> - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing);
>>> + bs->backing = bdrv_attach_child(bs, backing_hd, "backing",
>>> &child_backing);
>>> bs->open_flags &= ~BDRV_O_NO_BACKING;
>>> pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>>> backing_hd->filename);
>>> pstrcpy(bs->backing_format, sizeof(bs->backing_format),
>>> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename,
>>> goto done;
>>> }
>>>
>>> - c = bdrv_attach_child(parent, bs, child_role);
>>> + c = bdrv_attach_child(parent, bs, bdref_key, child_role);
>>>
>>> done:
>>> qdict_del(options, bdref_key);
>>> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d,
>>> BlockDriverState *bs)
>>> {
>>> const QDictEntry *entry;
>>> QemuOptDesc *desc;
>>> + BdrvChild *child;
>>> bool found_any = false;
>>> + const char *p;
>>>
>>> for (entry = qdict_first(bs->options); entry;
>>> entry = qdict_next(bs->options, entry))
>>> {
>>> - /* Only take options for this level */
>>> - if (strchr(qdict_entry_key(entry), '.')) {
>>> + /* Exclude options for children */
>>> + QLIST_FOREACH(child, &bs->children, next) {
>>> + if (strstart(qdict_entry_key(entry), child->name, &p)
>>> + && (!*p || *p == '.'))
>>> + {
>>> + break;
>>> + }
>>> + }
>>> + if (child) {
>>> continue;
>>> }
>>>
>>
>> A good general solution, but I think bdrv_refresh_filename() may be bad
>> enough to break with general solutions. ;-)
>>
>> bdrv_refresh_filename() only considers "file" and "backing" (actually,
>> it only supports "file" for now, I'm working on "backing", though). The
>> only drivers with other children are quorum, blkdebug, blkverify and
>> VMDK. The former three have their own implementation of
>> bdrv_refresh_filename(), so they don't use append_open_options() at all.
>> The latter, however, (VMDK) does not.
>>
>> This change to append_open_options results in the extent.%d options
>> simply being omitted altogether because bdrv_refresh_filename() does not
>> fetch them. Before, they were included in the VMDK BDS's options, which
>> is not ideal but works more or less.
>
> Are you sure? As far as I can tell, this patch should only keep options
> that were previously removed, but not remove options that were
> previously kept (with the exception of direct use of child names, which
> I added here to address your review comments for v1).
>
> Specifically for "extents.%d", this is a child name and is therefore
> omitted. However, it contains a '.', so it was already removed without
> this patch.
Right, it is broken already. The same applies to qcow2's options
containing a dot.
Max
> I'm accepting proof of the contrary in the form of a test case. ;-)
>
>> In order to "fix" this, I see three ways right now:
>> 1. Just care about "file" and "backing". bdrv_refresh_filename()
>> doesn't support anything else, so that will be fine.
>> 2. Implement bdrv_refresh_filename() specifically for VMDK so
>> append_open_options() will never have to handle anything but "file"
>> and "backing".
>> 3. Fix bdrv_refresh_filename() so that it handles all children and not
>> just "file" and "backing".
>>
>> Since we are shooting for 2.6 anyway (I assume ;-)), I think we should
>> go for option 3. This means that this patch is fine, and I'll see to
>> fixing bdrv_refresh_filename() (because I'm working on that anyway).
>
> Yes, I agree.
>
> Kevin
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v2 05/21] block: Consider all block layer options in append_open_options, (continued)
- [Qemu-block] [PATCH v2 03/21] mirror: Error out when a BDS would get two BBs, Kevin Wolf, 2015/11/23
- [Qemu-block] [PATCH v2 01/21] qcow2: Add .bdrv_join_options callback, Kevin Wolf, 2015/11/23
- [Qemu-block] [PATCH v2 07/21] block: Pass driver-specific options to .bdrv_refresh_filename(), Kevin Wolf, 2015/11/23
- [Qemu-block] [PATCH v2 09/21] block: Allow specifying child options in reopen, Kevin Wolf, 2015/11/23
- [Qemu-block] [PATCH v2 10/21] block: reopen: Document option precedence and refactor accordingly, Kevin Wolf, 2015/11/23
- [Qemu-block] [PATCH v2 12/21] block: Split out parse_json_protocol(), Kevin Wolf, 2015/11/23