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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.08.15 at 12:17, <ian.campbell@xxxxxxxxxx> wrote:
> On Thu, 2015-08-13 at 03:57 -0600, Jan Beulich wrote:
>> > 
>> > > > On 13.08.15 at 11:28, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>> > On Thu, 13 Aug 2015, Jan Beulich wrote:
>> > > > > > On 12.08.15 at 18:15, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>> > > > On Wed, 12 Aug 2015, Jan Beulich wrote:
>> > > > > > > > On 12.08.15 at 16:47, <stefano.stabellini@xxxxxxxxxxxxx> 
>> > > > > > > > wrote:
>> > > > > > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char 
>> > > > > > *inbuf, unsigned int len,
>> > > > > >   * dependent).
>> > > > > >   */
>> > > > > >  
>> > > > > > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
>> > > > > > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, 
>> > > > > > unlz4;
>> > > > > >  
>> > > > > >  int decompress(void *inbuf, unsigned int len, void *outbuf);
>> > > > > >  
>> > > > > > +static inline unsigned long output_length(char *image, 
>> > > > > > unsigned long image_len)
>> > > > > 
>> > > > > Neither of the callers gets moved out of bzimage.c - why does 
>> > > > > this
>> > > > > function need to move?
>> > > > 
>> > > > We'll use it on arm.
>> > > 
>> > > Hmm, the way it is used on x86 makes it quite architecture specific
>> > > (namely because of the assumption that the size is also in said
>> > > place for non-gz compression methods). I'd therefore prefer code
>> > > duplication over code sharing here. 
>> > 
>> > Actually after seeing the size and quality of the resulting patches, I
>> > am starting to feel the same way.
>> > 
>> > In terms of code changes, I was thinking that the best result would be
>> > moving the "boilerplate" code from xen/arch/x86/bzimage.c to
>> > xen/common/inflate.c, see below, then the interface would become just
>> > perform_gunzip and gzip_check. But I guess you wouldn't want inflate.c
>> > to be modified, right?
>> 
>> Yes, unless really unavoidable.
>> 
>> > Alternatively we could move it to a new file, let's call it gunzip.h,
>> > that would #include "inflate.c", so:
>> > 
>> > bzimage.c -- #include --> gunzip.h -- #include --> inflate.c
>> > 
>> > And again we just leave the perform_gunzip and gzip_check calls in
>> > bzimage.c.  What do you think?
> 
> How about putting perform_gunzip and gzip_check into a new gunzip.c which
> includes inflate.c?

Would seem as good to me, provided specifically the output_length()
helper then can stay where it is.

Jan


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