[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 06/13] block: Add block-specific QD
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 06/13] block: Add block-specific QDict header |
Date: |
Wed, 06 Jun 2018 15:10:33 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Max Reitz <address@hidden> writes:
> On 2018-05-10 16:54, Eric Blake wrote:
>> On 05/09/2018 11:55 AM, Max Reitz wrote:
>>> There are numerous QDict functions that have been introduced for and are
>>> used only by the block layer. Move their declarations into an own
>>
>> s/an own/their own/
>>
>>> header file to reflect that.
>>>
>>> While qdict_extract_subqdict() is in fact used outside of the block
>>> layer (in util/qemu-config.c), it is still a function related very
>>> closely to how the block layer works with nested QDicts, namely by
>>> sometimes flattening them. Therefore, its declaration is put into this
>>> header as well and util/qemu-config.c includes it with a comment stating
>>> exactly which function it needs.
>>>
>>> Suggested-by: Markus Armbruster <address@hidden>
>>> Signed-off-by: Max Reitz <address@hidden>
>>> ---
>>
>>> +++ b/include/block/qdict.h
>>> @@ -0,0 +1,35 @@
>>> +/*
>>> + * Special QDict functions used by the block layer
>>> + *
>>> + * Copyright (c) 2018 Red Hat, Inc.
>>
>> You are extracting this from qdict.h which has:
>> * Copyright (C) 2009 Red Hat Inc.
>>
>> Is it worth listing 2009-2018, instead of just this year?
>
> I don't know, is it? I don't know whether it makes a real difference.
Where's your taste for pedantry? Here's mine: please make it 2013-2018,
because the oldest code moved is from 2013.
>>> + *
>>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or
>>> later.
>>> + * See the COPYING.LIB file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef BLOCK_QDICT_H
>>> +#define BLOCK_QDICT_H
>>> +
>>> +#include "qapi/qmp/qdict.h"
>>> +#include "qapi/qmp/qobject.h"
>>> +#include "qapi/error.h"
Only the first #include is necessary, due to qemu/typedefs.h. Please
drop the other two.
>>> +
>>> +
>>> +void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>>> +void qdict_set_default_str(QDict *dst, const char *key, const char
>>> *val);
>>
>> These two might be useful outside of the block layer; we can move them
>> back in a later patch if so. But for now, I agree with your choice of
>> moving them.
>>
>>> +
>>> +void qdict_join(QDict *dest, QDict *src, bool overwrite);
>>
>> This is borderline whether it would be useful outside of the block layer.
>
> I decided I wanted to move the *_default* functions, and if I did that,
> I would have to move this one as well. I decided I can't be biased just
> because I wrote this one. :-)
>
> But in general I'd say if anyone wants to use any of these functions
> outside of the block layer, they are welcome to move them back to the
> main header, provided they have a good reason to do so. I suppose a
> good reason for using qdict_join() may indeed turn up sooner or later.
I like it this way.
With the trivial changes I asked for:
Reviewed-by: Markus Armbruster <address@hidden>
- Re: [Qemu-block] [Qemu-devel] [PATCH 06/13] block: Add block-specific QDict header,
Markus Armbruster <=