| Message ID | 1340254284-20415-1-git-send-email-peter.crosthwaite@petalogix.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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
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 >
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
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:
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(-)