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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- /dev/null
> +++ b/xen/common/gunzip.c
> @@ -0,0 +1,138 @@
> +#include <xen/config.h>

This should no be included explicitly in new code.

> +#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...

> +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().)

> +
> +    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.

Jan

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