Re: [Qemu-trivial] [PATCH v1] device_tree: load_device_tree(): Allow NULL sizep

[Stefan Hajnoczi]

On Fri, Jun 22, 2012 at 11:24 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Fri, Jun 22, 2012 at 7:15 PM, Stefan Hajnoczi <address@hidden> wrote:
>> On Thu, Jun 21, 2012 at 02:51:24PM +1000, Peter A. G. Crosthwaite wrote:
>>> The sizep arg is populated with the size of the loaded device tree. Since 
>>> this
>>> is one of those informational "please populate" type arguments it should be
>>> optional. Guarded writes to *sizep against NULL accordingly.
>>>
>>> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
>>> ---
>>>  device_tree.c |    8 ++++++--
>>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> Can someone who knows device trees review this?
>>
>> All current users need the size in order to copy the fdt into guest
>> memory.  Why should the size argument be optional?
>>
>
> Hi Stefan,
>
> Its required for an incomplete series i'm working on ATM. But it
> should be optional just from a general coding practice because its an
> informational "please populate on return argument". Forcing clients to
> declare dummy variables to hold return values they dont want is just
> bad programming practice. And assuming that clients are going to use
> the function to blob in memory, therefore they need to know the size
> in putting policy to an API that is more about mechanism. It would
> also be more consistent with the rest of the qemu_devtree_foo
> functions in that they all have length pointer functions (*len) that
> are optional.

Given the code that we have in tree today it doesn't look like a good
thing to do: all callers make use of the size and I wonder how you
will get by without the size.  It may be fine to make this change but
please get review from someone who understands device_tree.c (that's
not me, sorry).

> Regarding my large series, Im sending through general cleanup patches
> like this as I go, rather than confusing my series with stuff like
> this. The intention is to make life easier from a review point of view
> as I don't wan't to be in that [PATCH v20 50/50] place.

You're seeing the cons to sending out patches that lay the groundwork
without the actual "meat".  Reviewers have no context and may be
unhappy about making speculative changes.

If your patch isn't a clear win with today's qemu.git/master code, I
suggest keeping it in your main patch series.

Stefan