Re: grub 1.96 svn 20080813 and circular lvm2 metadata

[Felix Zielcke <fzielcke@z-51.de>]

Hans,

could you please address Marco's issues and send a new patch so the
topic is brought up again?

Am Freitag, den 29.08.2008, 18:08 +0200 schrieb Marco Gerards:
> 
> > diff -uwr grub-1.96_svn20080813-org/ChangeLog grub-1.96_svn20080813-new/ChangeLog
> > --- grub-1.96_svn20080813-org/ChangeLog	2008-08-13 17:24:36.000000000 +0200
> > +++ grub-1.96_svn20080813-new/ChangeLog	2008-08-29 10:33:03.000000000 +0200
> > @@ -1,3 +1,8 @@
> > +2008-08-28 Hans Lambermont <hans.lambermont@newtec.eu> (tiny change)
> > +	   Jan Derk Gerlings <jan.derk.gerlings@newtec.eu> (tiny change)
> > +
> > +	* disk/lvm.c: Add capability to read circular metadata
> 
> Please describe changes in the changelog entry, not the effect.
> 
> For example:
> 
> 	* disk/lvm.c (grub_lvm_scan_device): Allocate buffer space for the
> 	worst case scenario.
> 	(grub_lvm_scan_device): ...
> 
> Where ... has to be filled in, I have no idea what this code actually
> does or what you changed ;-)
> 
> The tiny change syntax does not seem familiar to me, AFAIK it is not
> from the GCS.  Can you please change that?  Furthermore, if you both
> worked on there two parts of the patch, please send in separate
> patches.  It will make my life a lot easier... :-)
> 
> If only one person worked on this, for example Jan Derk, which Hans
> only forwards this patch, please only list Jan Derk as the
> contributor.
> 
> >  2008-08-12  Robert Millan  <rmh@aybabtu.com>
> >  
> >  	* loader/i386/pc/multiboot.c (grub_multiboot_load_elf32): Move part
> > diff -uwr grub-1.96_svn20080813-org/disk/lvm.c grub-1.96_svn20080813-new/disk/lvm.c
> > --- grub-1.96_svn20080813-org/disk/lvm.c	2008-08-28 14:32:53.000000000 +0200
> > +++ grub-1.96_svn20080813-new/disk/lvm.c	2008-08-28 18:31:19.000000000 +0200
> > @@ -281,7 +281,8 @@
> >        goto fail;
> >      }
> >  
> > -  metadatabuf = grub_malloc (mda_size);
> > +  /* alloc for circular worst-case scenario */
> 
> Nitpick: Please start a sentence with a capital letter and end with a
> `.'.  So this will become:
> 
>   /* Assume circular buffer in a worst case scenario.  */
> 
> 
> > +  metadatabuf = grub_malloc (2*mda_size);
> 
> Please use spaces around operators:
> 
> 2 * md_size
> 
> >    if (! metadatabuf)
> >      goto fail;
> >  
> > @@ -300,6 +301,12 @@
> >      }
> >  
> >    rlocn = mdah->raw_locns;
> > +  if (rlocn->offset + rlocn->size > mdah->size)
> 
> Here rlcon->offset seems to be little endian (64 bits), so please use
> grub_le_to_cpu64.  Same for rlocn->size, please check the size of this
> member before you use a macro (I couldn't find it immediately...).
> 
> > +    {
> > +      /* metadata is circular */
> 
> Same as above.
> 
> > +      grub_memcpy(metadatabuf + mda_size, metadatabuf + mdah->start,
> > +		  ((rlocn->offset + rlocn->size) - mdah->size));
> > +    }
> 
> Please check and correct the endianess.  you use a lot of
> parenthesises.  Actually, I think none are requires.


-- 
Felix Zielcke