Re: [Qemu-devel] [PATCH 06/13] block: Add block-specific QDict header

From: Markus Armbruster <armbru@redhat.com>
To: Max Reitz <mreitz@redhat.com>
Cc: Eric Blake <eblake@redhat.com>,
	qemu-block@nongnu.org, Kevin Wolf <kwolf@redhat.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>,
	qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 06/13] block: Add block-specific QDict header
Date: Wed, 06 Jun 2018 15:10:33 +0200	[thread overview]
Message-ID: <87y3fs83hy.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <9ad8a3f2-d56f-5eaf-3136-ed5264ec9eb4@redhat.com> (Max Reitz's message of "Fri, 11 May 2018 20:11:24 +0200")

Max Reitz <mreitz@redhat.com> 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 <armbru@redhat.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>> 
>>> +++ 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 <armbru@redhat.com>