Re: [kvm-ppc-devel] [PATCH 5 of 7] Add dynamic device tree manipulation & change uboot loader for PPC bamboo board model

[Hollis Blanchard <hollisb@us.ibm.com>]

I can only assume that you will actually make the corrections that you
didn't respond to in this mail.

On Tue, 2008-03-18 at 16:35 -0500, Jerone Young wrote:
> On Tue, 2008-03-18 at 16:25 -0500, Hollis Blanchard wrote:
> > 
> > > +#define DT_PROC_INTERFACE_PATH "/proc/device-tree"
> > > +
> > > +/* FUNCTIONS FOR READING FROM DEVICE TREE OF HOST IN /PROC */
> > > +
> > > +/* This function reads device-tree property files that are of
> > > + * a single cell size
> > > + */
> > > +uint32_t read_proc_dt_prop_cell(char *path_in_device_tree)
> > > +{
> > > +	char *buf = NULL;
> > > +	int i;
> > > +	uint32_t num;
> > > +	FILE *stream;
> > > +
> > > +	i = snprintf(buf, 0, "%s/%s", DT_PROC_INTERFACE_PATH,
> > > +		path_in_device_tree);
> > > +
> > > +	buf = (char *)malloc(i);
> > > +	if (buf == NULL)
> > > +	{
> > > +		printf("%s: Unable to malloc string buffer buf\n",
> > > +			__func__);
> > > +		exit(1);
> > > +	}
> > 
> > Braces.
> 
> What is the deal. They are braces. They are done diffrenent through outt
> the qemu code. This

I don't enjoy correcting whitespace, and in fact I hate it when that's
all comments are about. If it weren't so egregious in this patch series,
I probably would have let it slide.

In general I don't really care as long as it's *consistent*. The tabs
you use in this code clashes with the rest of qemu, and that makes it
difficult to use the right editor settings. Despite that, I still didn't
say anything, because at least it's consistent.

In general, don't just make your code work; make it pretty too.

> > > @@ -26,14 +27,22 @@ void bamboo_init(ram_addr_t ram_size, in
> > >  			const char *initrd_filename,
> > >  			const char *cpu_model)
> > >  {
> > > +	char buf[1024];
> > 
> > You previously said you had removed 'buf' and replaced it with dynamic
> > allocation, but I don't see that here.
> 
> Removing of buf discussed was from hw/device_tree.c  not this file.

So you can fix it here too, right?

-- 
Hollis Blanchard
IBM Linux Technology Center


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/