The individual decompression CUs need to only surface their top level
functions to other code. Arrange for everything else to be static, to
make sure no undue uses of that code exist or will appear without
explicitly noticing. (In some cases this also results in code size
reduction, but since this is all init-only code this probably doesn't
matter very much.)

In the LZO case also take the opportunity and convert u8 where lines
get touched anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The downside is that the top level functions will now be non-static
in stubdom builds of libxenguest, but I think that's acceptable.

--- a/xen/common/bunzip2.c
+++ b/xen/common/bunzip2.c
@@ -665,12 +665,11 @@ static int INIT start_bunzip(struct bunz
 
 /* Example usage: decompress src_fd to dst_fd.  (Stops at end of bzip2 data,
    not end of file.) */
-STATIC int INIT bunzip2(unsigned char *buf, unsigned int len,
-			int(*fill)(void*, unsigned int),
-			int(*flush)(void*, unsigned int),
-			unsigned char *outbuf,
-			unsigned int *pos,
-			void(*error)(const char *x))
+int INIT bunzip2(unsigned char *buf, unsigned int len,
+		 int(*fill)(void*, unsigned int),
+		 int(*flush)(void*, unsigned int),
+		 unsigned char *outbuf, unsigned int *pos,
+		 void(*error)(const char *x))
 {
 	struct bunzip_data *bd;
 	int i = -1;
--- a/xen/common/decompress.h
+++ b/xen/common/decompress.h
@@ -7,7 +7,7 @@
 #include <xen/types.h>
 #include <xen/xmalloc.h>
 
-#define STATIC
+#define STATIC static
 #define INIT __init
 #define INITDATA __initdata
 
--- a/xen/common/unlz4.c
+++ b/xen/common/unlz4.c
@@ -22,12 +22,11 @@
 #define LZ4_DEFAULT_UNCOMPRESSED_CHUNK_SIZE (8 << 20)
 #define ARCHIVE_MAGICNUMBER 0x184C2102
 
-STATIC int INIT unlz4(unsigned char *input, unsigned int in_len,
-		      int (*fill)(void *, unsigned int),
-		      int (*flush)(void *, unsigned int),
-		      unsigned char *output,
-		      unsigned int *posp,
-		      void (*error)(const char *x))
+int INIT unlz4(unsigned char *input, unsigned int in_len,
+	       int (*fill)(void *, unsigned int),
+	       int (*flush)(void *, unsigned int),
+	       unsigned char *output, unsigned int *posp,
+	       void (*error)(const char *x))
 {
 	int ret = -1;
 	size_t chunksize = 0;
--- a/xen/common/unlzma.c
+++ b/xen/common/unlzma.c
@@ -528,13 +528,11 @@ static inline int INIT process_bit1(stru
 
 
 
-STATIC int INIT unlzma(unsigned char *buf, unsigned int in_len,
-		       int(*fill)(void*, unsigned int),
-		       int(*flush)(void*, unsigned int),
-		       unsigned char *output,
-		       unsigned int *posp,
-		       void(*error)(const char *x)
-	)
+int INIT unlzma(unsigned char *buf, unsigned int in_len,
+	        int(*fill)(void*, unsigned int),
+	        int(*flush)(void*, unsigned int),
+	        unsigned char *output, unsigned int *posp,
+	        void(*error)(const char *x))
 {
 	struct lzma_header header;
 	int lc, pb, lp;
--- a/xen/common/unlzo.c
+++ b/xen/common/unlzo.c
@@ -114,11 +114,11 @@ static int INIT parse_header(u8 *input,
 	return 1;
 }
 
-STATIC int INIT unlzo(u8 *input, unsigned int in_len,
-		      int (*fill) (void *, unsigned int),
-		      int (*flush) (void *, unsigned int),
-		      u8 *output, unsigned int *posp,
-		      void (*error) (const char *x))
+int INIT unlzo(unsigned char *input, unsigned int in_len,
+	       int (*fill) (void *, unsigned int),
+	       int (*flush) (void *, unsigned int),
+	       unsigned char *output, unsigned int *posp,
+	       void (*error) (const char *x))
 {
 	u8 r = 0;
 	int skip = 0;
--- a/xen/common/unxz.c
+++ b/xen/common/unxz.c
@@ -157,11 +157,11 @@
  * both input and output buffers are available as a single chunk, i.e. when
  * fill() and flush() won't be used.
  */
-STATIC int INIT unxz(unsigned char *in, unsigned int in_size,
-		     int (*fill)(void *dest, unsigned int size),
-		     int (*flush)(void *src, unsigned int size),
-		     unsigned char *out, unsigned int *in_used,
-		     void (*error)(const char *x))
+int INIT unxz(unsigned char *in, unsigned int in_size,
+	      int (*fill)(void *dest, unsigned int size),
+	      int (*flush)(void *src, unsigned int size),
+	      unsigned char *out, unsigned int *in_used,
+	      void (*error)(const char *x))
 {
 	struct xz_buf b;
 	struct xz_dec *s;
--- a/xen/common/unzstd.c
+++ b/xen/common/unzstd.c
@@ -142,12 +142,11 @@ out:
 	return err;
 }
 
-STATIC int INIT unzstd(unsigned char *in_buf, unsigned int in_len,
-		       int (*fill)(void*, unsigned int),
-		       int (*flush)(void*, unsigned int),
-		       unsigned char *out_buf,
-		       unsigned int *in_pos,
-		       void (*error)(const char *x))
+int INIT unzstd(unsigned char *in_buf, unsigned int in_len,
+	        int (*fill)(void*, unsigned int),
+	        int (*flush)(void*, unsigned int),
+	        unsigned char *out_buf, unsigned int *in_pos,
+	        void (*error)(const char *x))
 {
 	ZSTD_inBuffer in;
 	ZSTD_outBuffer out;

On 18/01/2021 11:19, Jan Beulich wrote:
> The individual decompression CUs need to only surface their top level
> functions to other code. Arrange for everything else to be static, to
> make sure no undue uses of that code exist or will appear without
> explicitly noticing. (In some cases this also results in code size
> reduction, but since this is all init-only code this probably doesn't
> matter very much.)
>
> In the LZO case also take the opportunity and convert u8 where lines
> get touched anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The downside is that the top level functions will now be non-static
> in stubdom builds of libxenguest, but I think that's acceptable.

Yeah - not something to lose sleep over.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but I really would
like to see the STATIC and INIT defines disappear eventually.

On 18.01.2021 12:58, Andrew Cooper wrote:
> On 18/01/2021 11:19, Jan Beulich wrote:
>> The individual decompression CUs need to only surface their top level
>> functions to other code. Arrange for everything else to be static, to
>> make sure no undue uses of that code exist or will appear without
>> explicitly noticing. (In some cases this also results in code size
>> reduction, but since this is all init-only code this probably doesn't
>> matter very much.)
>>
>> In the LZO case also take the opportunity and convert u8 where lines
>> get touched anyway.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The downside is that the top level functions will now be non-static
>> in stubdom builds of libxenguest, but I think that's acceptable.
> 
> Yeah - not something to lose sleep over.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, but this actually breaks the stubdom build, which now sees
non-static function definitions without prior declaration, which
with -Werror results in the build to fail. There's this extra hunk
now which addresses this:

--- unstable.orig/tools/libs/guest/xg_dom_decompress_unsafe.h
+++ unstable/tools/libs/guest/xg_dom_decompress_unsafe.h
@@ -1,8 +1,12 @@
+#ifdef __MINIOS__
+# include "../../xen/include/xen/decompress.h"
+#else
 typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
                           int (*fill)(void*, unsigned int),
                           int (*flush)(void*, unsigned int),
                           unsigned char *outbuf, unsigned int *posp,
                           void (*error)(const char *x));
+#endif
 
 int xc_dom_decompress_unsafe(
     decompress_fn fn, struct xc_dom_image *dom, void **blob, size_t *size)

i.e. strictly speaking I may then also need a tools side ack.

> but I really would
> like to see the STATIC and INIT defines disappear eventually.

I too would like to, but I don't see this happen in particular
for INIT and INITDATA, when we have two distinct environments
where this code gets built. Unless you envision the tool stack /
stubdom side of the build to gain

#define __init
#define __initdata

? As far as STATIC goes, being in the middle of the DomU side
work of this, I've found a need to sprinkle around quite a
few of them in zstd/decompress.c, again to silence similar
compiler diagnostics.

Jan

On 18/01/2021 15:13, Jan Beulich wrote:
> On 18.01.2021 12:58, Andrew Cooper wrote:
>> On 18/01/2021 11:19, Jan Beulich wrote:
>>> The individual decompression CUs need to only surface their top level
>>> functions to other code. Arrange for everything else to be static, to
>>> make sure no undue uses of that code exist or will appear without
>>> explicitly noticing. (In some cases this also results in code size
>>> reduction, but since this is all init-only code this probably doesn't
>>> matter very much.)
>>>
>>> In the LZO case also take the opportunity and convert u8 where lines
>>> get touched anyway.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> The downside is that the top level functions will now be non-static
>>> in stubdom builds of libxenguest, but I think that's acceptable.
>> Yeah - not something to lose sleep over.
>>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks, but this actually breaks the stubdom build, which now sees
> non-static function definitions without prior declaration, which
> with -Werror results in the build to fail. There's this extra hunk
> now which addresses this:
>
> --- unstable.orig/tools/libs/guest/xg_dom_decompress_unsafe.h
> +++ unstable/tools/libs/guest/xg_dom_decompress_unsafe.h
> @@ -1,8 +1,12 @@
> +#ifdef __MINIOS__
> +# include "../../xen/include/xen/decompress.h"
> +#else
>  typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
>                            int (*fill)(void*, unsigned int),
>                            int (*flush)(void*, unsigned int),
>                            unsigned char *outbuf, unsigned int *posp,
>                            void (*error)(const char *x));
> +#endif
>  
>  int xc_dom_decompress_unsafe(
>      decompress_fn fn, struct xc_dom_image *dom, void **blob, size_t *size)
>
> i.e. strictly speaking I may then also need a tools side ack.

My ack still stands.

>
>> but I really would
>> like to see the STATIC and INIT defines disappear eventually.
> I too would like to, but I don't see this happen in particular
> for INIT and INITDATA, when we have two distinct environments
> where this code gets built. Unless you envision the tool stack /
> stubdom side of the build to gain
>
> #define __init
> #define __initdata
>
> ?

I think that's fine.

One way or another, some header file somewhere needs to plumb this up
for the differing environments.  I just don't see the need for an extra
level of indirection.

> As far as STATIC goes, being in the middle of the DomU side
> work of this, I've found a need to sprinkle around quite a
> few of them in zstd/decompress.c, again to silence similar
> compiler diagnostics.

Do you mean you've had to add more STATIC's in?

~Andrew

On 18.01.2021 16:31, Andrew Cooper wrote:
> On 18/01/2021 15:13, Jan Beulich wrote:
>> On 18.01.2021 12:58, Andrew Cooper wrote:
>>> but I really would
>>> like to see the STATIC and INIT defines disappear eventually.
>> I too would like to, but I don't see this happen in particular
>> for INIT and INITDATA, when we have two distinct environments
>> where this code gets built. Unless you envision the tool stack /
>> stubdom side of the build to gain
>>
>> #define __init
>> #define __initdata
>>
>> ?
> 
> I think that's fine.
> 
> One way or another, some header file somewhere needs to plumb this up
> for the differing environments.  I just don't see the need for an extra
> level of indirection.

I see. Maybe I can do so while I'm touching this code anyway.

>> As far as STATIC goes, being in the middle of the DomU side
>> work of this, I've found a need to sprinkle around quite a
>> few of them in zstd/decompress.c, again to silence similar
>> compiler diagnostics.
> 
> Do you mean you've had to add more STATIC's in?

Yes indeed. Plus #ifdef out some of the functions that aren't
used at all, i.e. where with static added the compiler would
still warn. Yet I didn't want to delete code there, in order
for future changes we might want to grab from Linux to apply
reasonably.

Jan

Jan Beulich writes ("[PATCH] xen/decompress: make helper symbols static"):
> The individual decompression CUs need to only surface their top level
> functions to other code. Arrange for everything else to be static, to
> make sure no undue uses of that code exist or will appear without
> explicitly noticing. (In some cases this also results in code size
> reduction, but since this is all init-only code this probably doesn't
> matter very much.)
> 
> In the LZO case also take the opportunity and convert u8 where lines
> get touched anyway.

AFAICT, this patch

* was first posted after the last posting date for Xen 4.15.
* is not a bugfix.

In which case it has missed 4.15.

Please correct me if you think I am wrong.

Thanks,
Ian.

On 18/01/2021 16:09, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH] xen/decompress: make helper symbols static"):
>> The individual decompression CUs need to only surface their top level
>> functions to other code. Arrange for everything else to be static, to
>> make sure no undue uses of that code exist or will appear without
>> explicitly noticing. (In some cases this also results in code size
>> reduction, but since this is all init-only code this probably doesn't
>> matter very much.)
>>
>> In the LZO case also take the opportunity and convert u8 where lines
>> get touched anyway.
> AFAICT, this patch
>
> * was first posted after the last posting date for Xen 4.15.
> * is not a bugfix.
>
> In which case it has missed 4.15.
>
> Please correct me if you think I am wrong.

It's part of unbreaking Linux kernel compressed with zstd, which I put
on the 4.15 tracking list.

Without the work built on top of this, you can't boot Fedora guests. 
The dom0 work is committed into staging today.  The domU work is in
progress.

~Andrew

On 18.01.2021 17:09, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH] xen/decompress: make helper symbols static"):
>> The individual decompression CUs need to only surface their top level
>> functions to other code. Arrange for everything else to be static, to
>> make sure no undue uses of that code exist or will appear without
>> explicitly noticing. (In some cases this also results in code size
>> reduction, but since this is all init-only code this probably doesn't
>> matter very much.)
>>
>> In the LZO case also take the opportunity and convert u8 where lines
>> get touched anyway.
> 
> AFAICT, this patch
> 
> * was first posted after the last posting date for Xen 4.15.
> * is not a bugfix.
> 
> In which case it has missed 4.15.
> 
> Please correct me if you think I am wrong.

You aren't, and I don't view this as a big problem - its more of
an aid to be sure no bad (unintended) references exist (Arm's
unhelpful re-use of xz's CRC32 function triggered the change). I
can surely hold this back until 4.16 opens, hoping it won't
collide with the zstd decompression work I'm now doing on the
DomU side (lack which, if you agree with Andrew's assessment, is
more a bug fix than a feature addition).

But yes - I will need to remind myself to filter what is eligible
for committing, from now on until the branch point.

Jan

Jan Beulich writes ("Re: [PATCH] xen/decompress: make helper symbols static"):
> On 18.01.2021 17:09, Ian Jackson wrote:
> > AFAICT, this patch
> > 
> > * was first posted after the last posting date for Xen 4.15.
> > * is not a bugfix.
> > 
> > In which case it has missed 4.15.
> > 
> > Please correct me if you think I am wrong.
> 
> You aren't, and I don't view this as a big problem - its more of
> an aid to be sure no bad (unintended) references exist (Arm's
> unhelpful re-use of xz's CRC32 function triggered the change).

Ah I see.  Thanks for that explanation.  Well, I might be inclined to
grant a freeze exception on the basis that the point of this is to
remove rather than add risk.

If you think that might be sensible, would you care to provide a frank
assessment of the risks to 4.15 of taking this patch, vs. the risks to
4.15 of postponing it ?

> I can surely hold this back until 4.16 opens, hoping it won't
> collide with the zstd decompression work I'm now doing on the DomU
> side (lack which, if you agree with Andrew's assessment, is more a
> bug fix than a feature addition).

I agree that zstd is a blocker for 4.15.

> But yes - I will need to remind myself to filter what is eligible
> for committing, from now on until the branch point.

If there are edge cases, or things that would warrant an exception,
please do ask me explicitly.

In general my objective for the freeze policy is to try to minimise
the number and severity of bugs in 4.15 :-) and if I think a freeze
exception is the best way of doing that, I will grant one.

Ian.

On 19.01.2021 15:58, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] xen/decompress: make helper symbols static"):
>> On 18.01.2021 17:09, Ian Jackson wrote:
>>> AFAICT, this patch
>>>
>>> * was first posted after the last posting date for Xen 4.15.
>>> * is not a bugfix.
>>>
>>> In which case it has missed 4.15.
>>>
>>> Please correct me if you think I am wrong.
>>
>> You aren't, and I don't view this as a big problem - its more of
>> an aid to be sure no bad (unintended) references exist (Arm's
>> unhelpful re-use of xz's CRC32 function triggered the change).
> 
> Ah I see.  Thanks for that explanation.  Well, I might be inclined to
> grant a freeze exception on the basis that the point of this is to
> remove rather than add risk.
> 
> If you think that might be sensible, would you care to provide a frank
> assessment of the risks to 4.15 of taking this patch, vs. the risks to
> 4.15 of postponing it ?

Considering that we've been building fine without this adjustment,
I don't think the risk of not taking the patch is non-negligible.
The added safety is more for future work, to prevent anyone
mistakenly using any of the so far globally available symbols.
(There's actually more to do I think, in a separate patch, further
reducing exposure of the just introduced zstd decompressor
functions. I have this on my list of things to look into.)

Taking the patch has a minimal risk of breaking the build, in
case in some configuration a bad reference actually exists. I
can't see any other risk, as the actual code doesn't get changed
at all.

Jan