Re: [Xen-devel] [PATCH v2 1/2] xen: move perform_gunzip to common

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 13 Aug 2015, Jan Beulich wrote:
> >>> On 13.08.15 at 13:21, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -55,6 +55,7 @@ obj-y += vmap.o
> >  obj-y += vsprintf.o
> >  obj-y += wait.o
> >  obj-y += xmalloc_tlsf.o
> > +obj-y += gunzip.o
> >  
> >  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo 
> > unlz4 earlycpio,$(n).init.o)
> 
> Isn't the code you add in gunzip.c all __init / __initdata (or could at least
> be)? If so, this should become obj-bin-y += gunzip.o just like is being
> done for all the other decompressors.

OK, I'll make the change


> > --- /dev/null
> > +++ b/xen/common/gunzip.c
> > @@ -0,0 +1,138 @@
> > +#include <xen/config.h>
> 
> This should no be included explicitly in new code.

OK
        

> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/mm.h>
> > +
> > +#define HEAPORDER 3
> > +
> > +static unsigned char *__initdata window;
> > +#define memptr long
> > +static memptr __initdata free_mem_ptr;
> > +static memptr __initdata free_mem_end_ptr;
> > +
> > +#define WSIZE           0x80000000
> > +
> > +static unsigned char *__initdata inbuf;
> > +static unsigned __initdata insize;
> > +
> > +/* Index of next byte to be processed in inbuf: */
> > +static unsigned __initdata inptr;
> > +
> > +/* Bytes in output buffer: */
> > +static unsigned __initdata outcnt;
> > +
> > +#define OF(args)        args
> > +#define STATIC          static
> > +
> > +#define memzero(s, n)   memset((s), 0, (n))
> 
> I understand that you're mostly moving code, but I'd appreciate if
> you did some formatting adjustments along the way (like removing
> the superfluous parentheses here...

I prefer to keep code movement as code movement -- much easier to bisect
or spot regressions. If you have any changes that you really require on
top of the movement, I could carry them on a separate patch on top of
this.


> > +typedef unsigned char   uch;
> > +typedef unsigned short  ush;
> > +typedef unsigned long   ulg;
> > +
> > +#define INIT            __init
> > +#define INITDATA        __initdata
> > +
> > +#define get_byte()      (inptr < insize ? inbuf[inptr++] : fill_inbuf())
> > +
> > +/* Diagnostic functions */
> > +#ifdef DEBUG
> > +#  define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0)
> > +#  define Trace(x)      do { fprintf x; } while (0)
> > +#  define Tracev(x)     do { if (verbose) fprintf x ; } while (0)
> > +#  define Tracevv(x)    do { if (verbose > 1) fprintf x ; } while (0)
> > +#  define Tracec(c, x)  do { if (verbose && (c)) fprintf x ; } while (0)
> > +#  define Tracecv(c, x) do { if (verbose > 1 && (c)) fprintf x ; } while 
> > (0)
> > +#else
> > +#  define Assert(cond, msg)
> > +#  define Trace(x)
> > +#  define Tracev(x)
> > +#  define Tracevv(x)
> > +#  define Tracec(c, x)
> > +#  define Tracecv(c, x)
> > +#endif
> > +
> > +static long __initdata bytes_out;
> > +static void flush_window(void);
> > +
> > +static __init void error(char *x)
> > +{
> > +    panic("%s", x);
> > +}
> > +
> > +static __init int fill_inbuf(void)
> > +{
> > +        error("ran out of input data");
> > +        return 0;
> 
> ... or indentation here).
> 
> > +}
> > +
> > +
> 
> Just one blank line please.
> 
> > +#include "inflate.c"
> > +
> > +static __init void flush_window(void)
> 
> Any reason this can't be placed ahead of the #include, avoiding the
> extra declaration earlier on?
> 
> > +__init int gzip_check(char *image, unsigned long image_len)
> 
> int __init gzip_check(...
> 
> > +{
> > +    unsigned char magic0, magic1;
> > +
> > +    if ( image_len < 2 )
> > +        return 0;
> > +
> > +    magic0 = (unsigned char)image[0];
> > +    magic1 = (unsigned char)image[1];
> 
> Pointless casts?
> 
> > +__init int perform_gunzip(char *output, char *image, unsigned long 
> > image_len)
> > +{
> > +    int rc;
> > +
> > +    if ( !gzip_check(image, image_len) )
> > +        return 1;
> > +
> > +    window = (unsigned char *)output;
> 
> Any reason output and image can't be declared "unsigned char *"
> right away, avoiding such bogus casts? (Same then for
> gzip_check().)

all this code was left untouched by the movement, but FYI simply
changing output and image to unsigned char * would break the compilation
of bzimage.c


> > +
> > +    free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0);
> > +    free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER);
> > +
> > +    inbuf = (unsigned char *)image;
> > +    insize = image_len;
> > +    inptr = 0;
> > +
> > +    makecrc();
> > +
> > +    if ( gunzip() < 0 )
> > +    {
> > +        rc = -EINVAL;
> > +    }
> > +    else
> > +    {
> > +        rc = 0;
> > +    }
> 
> Many pointless braces.
> 
> > --- /dev/null
> > +++ b/xen/include/xen/gunzip.h
> > @@ -0,0 +1,7 @@
> > +#ifndef __XEN_GUNZIP_H
> > +#define __XEN_GUNZIP_H
> > +
> > +__init int gzip_check(char *image, unsigned long image_len);
> > +__init int perform_gunzip(char *output, char *image, unsigned long 
> > image_len);
> 
> No __init on declarations please.

OK

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel