Re: [PATCH 3/7] Initial support for U-Boot platforms

[Leif Lindholm]

On Sat, Mar 30, 2013 at 05:20:54PM +0100, Francesco Lavra wrote:
> Mostly cosmetic comments from my side here...
 
Typos will be fixed.

> On 03/24/2013 06:01 PM, Leif Lindholm wrote:
> [...]
> > === added file 'grub-core/kern/uboot/init.c'
> > --- grub-core/kern/uboot/init.c     1970-01-01 00:00:00 +0000
> > +++ grub-core/kern/uboot/init.c     2013-03-24 12:58:23 +0000
> [...]
> > +/*
> > + * grub_machine_get_bootlocation():
> > + *   Called from kern/main.c, which expects a device name (minus 
> > parentheses)
> > + *   and a filesystem path back, if any are known.
> > + *   Any returned values must be pointers to dynamically allocated strings.
> > + */
> > +void
> > +grub_machine_get_bootlocation (char **device, char **path)
> > +{
> > +  char *tmp;
> > +
> > +  tmp = uboot_env_get ("grub_bootdev");
> > +  if (tmp)
> > +    {
> > +      *device = grub_malloc (grub_strlen (tmp) + 1);
> > +      if (*device == NULL)
> > +   return;
> > +      grub_strncpy (*device, tmp, grub_strlen (tmp) + 1);
> 
> Why not simply call grub_strcpy (*device, tmp)?
 
Because I am an overtly paranoid person. :)

[...]

> > === added file 'grub-core/kern/uboot/uboot.c'
> > --- grub-core/kern/uboot/uboot.c    1970-01-01 00:00:00 +0000
> > +++ grub-core/kern/uboot/uboot.c    2013-03-24 12:58:23 +0000
> [...]
> > +/* All functions below are wrappers around the uboot_syscall() function */
> > +
> > +/*
> > + * int API_getc(int *c)
> > + */
> > +int
> > +uboot_getc (void)
> 
> The comment preceding the function does not correspond to the function
> prototype. This applies as well to all the subsequent functions in this
> file.

This was a probably misguided way of showing what the "receiving end" of
the call ended up being (in U-Boot). I should either document this somewhere
or just drop these. Does anyone see any value in keeping them around?
 
> > === added file 'include/grub/uboot/uboot.h'
> > --- include/grub/uboot/uboot.h      1970-01-01 00:00:00 +0000
> > +++ include/grub/uboot/uboot.h      2013-03-24 12:58:23 +0000
[...]
> > +/* All functions below are wrappers around the uboot_syscall() function */
> > +
> > +/*
> > + * int API_getc(int *c)
> > + */
> > +int uboot_getc (void);
> 
> Same as for kern/uboot/uboot.c: the comments preceding the functions do
> not correspond to the function prototypes.
 
Same reason. Same question.

/
    Leif