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

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

[Peter A. G. Crosthwaite]

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 <peter.crosthwaite@petalogix.com>
---
 device_tree.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 86a694c..0ed0256 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -32,7 +32,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
     int ret;
     void *fdt = NULL;
 
-    *sizep = 0;
+    if (sizep) {
+        *sizep = 0;
+    }
     dt_size = get_image_size(filename_path);
     if (dt_size < 0) {
         printf("Unable to get size of device tree file '%s'\n",
@@ -65,7 +67,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
             filename_path);
         goto fail;
     }
-    *sizep = dt_size;
+    if (sizep) {
+        *sizep = dt_size;
+    }
     return fdt;
 
 fail:
-- 
1.7.3.2

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

[Stefan Hajnoczi]

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 <peter.crosthwaite@petalogix.com>
> ---
>  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?

Stefan

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

[Peter Crosthwaite]

On Fri, Jun 22, 2012 at 7:15 PM, Stefan Hajnoczi <stefanha@gmail.com> 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 <peter.crosthwaite@petalogix.com>
>> ---
>>  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.

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.

Regards,
Peter

> Stefan

Re: [Qemu-devel] [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
<peter.crosthwaite@petalogix.com> wrote:
> On Fri, Jun 22, 2012 at 7:15 PM, Stefan Hajnoczi <stefanha@gmail.com> 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 <peter.crosthwaite@petalogix.com>
>>> ---
>>>  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

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

[Peter Crosthwaite]

CC device-tree.c original contributors. (Jerome Young and Hollis Blanchard).

I cant find a maintainer for device-tree, and Stefan wants a review.
This patch seem ok?

On Thu, Jun 21, 2012 at 2:51 PM, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> 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 <peter.crosthwaite@petalogix.com>
> ---
>  device_tree.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 86a694c..0ed0256 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -32,7 +32,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>     int ret;
>     void *fdt = NULL;
>
> -    *sizep = 0;
> +    if (sizep) {
> +        *sizep = 0;
> +    }
>     dt_size = get_image_size(filename_path);
>     if (dt_size < 0) {
>         printf("Unable to get size of device tree file '%s'\n",
> @@ -65,7 +67,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>             filename_path);
>         goto fail;
>     }
> -    *sizep = dt_size;
> +    if (sizep) {
> +        *sizep = dt_size;
> +    }
>     return fdt;
>
>  fail:
> --
> 1.7.3.2
>

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

[Alexander Graf]

On 22.06.2012, at 15:17, Peter Crosthwaite wrote:

> CC device-tree.c original contributors. (Jerome Young and Hollis Blanchard).
> 
> I cant find a maintainer for device-tree, and Stefan wants a review.
> This patch seem ok?

Hrm, guess I should file a patch to declare myself maintainer for the time being, unless someone else wants to stand up and take on it.

Acked-by: Alexander Graf <agraf@suse.de>


Alex

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

[Peter Crosthwaite]

On Sat, Jun 23, 2012 at 7:14 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 22.06.2012, at 15:17, Peter Crosthwaite wrote:
>
>> CC device-tree.c original contributors. (Jerome Young and Hollis Blanchard).
>>
>> I cant find a maintainer for device-tree, and Stefan wants a review.
>> This patch seem ok?
>
> Hrm, guess I should file a patch to declare myself maintainer for the time being, unless someone else wants to stand up and take on it.

I'm happy to do it as well.

>
> Acked-by: Alexander Graf <agraf@suse.de>
>

Thanks.

Regards,
Peter
>
> Alex
>

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

[Alexander Graf]

On 23.06.2012, at 02:45, Peter Crosthwaite wrote:

> On Sat, Jun 23, 2012 at 7:14 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 22.06.2012, at 15:17, Peter Crosthwaite wrote:
>> 
>>> CC device-tree.c original contributors. (Jerome Young and Hollis Blanchard).
>>> 
>>> I cant find a maintainer for device-tree, and Stefan wants a review.
>>> This patch seem ok?
>> 
>> Hrm, guess I should file a patch to declare myself maintainer for the time being, unless someone else wants to stand up and take on it.
> 
> I'm happy to do it as well.

Works fine for me :). Just send a patch to MAINTAINERS. If you like, add me to it as well as your backup, though you probably know more about dt than me ;).


Alex