Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

I have a feeling we may be talking at cross purposes rather too much.

Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed 
kernels"):
> On 25.01.2021 15:53, Ian Jackson wrote:
> > Well how about passing "true" for the fourth argument then ?
> 
> That I did try intermediately, but didn't ever post. It'll
> screw up when libzstd_CFLAGS and libzstd_LIBS were provided
> to override pkg-config. When you look at the expanded code,
> this will end up with pkg_failed set to "untried" and still
> take the error path. I.e. we wouldn't get the overridden
> settings appended to $zlib.

I infer you're reading the autoonf output.  I think pkg_failed is
something to do with tracking whether pkg-config exists at all.  In
general, reading autoconf output is an act of desperation when RTFM
and so on fails.  The output is typically much more complicated than
the input and can be quite confusing.

I noticed that configure.ac fails to say PKG_PROG_PKG_CONFIG contrary
to the imprecations in the documentation.  For example, for
PKG_CHECK_MODULES we have:

 | # Note that if there is a possibility the first call to
 | # PKG_CHECK_MODULES might not happen, you should be sure to include
 | # an explicit call to PKG_PROG_PKG_CONFIG in your configure.ac
 
Indeed our first call to PKG_CHECK_* in the existing configure.ac is
within an if and there is no call to PKG_PROG_PKG_CONFIG.  I think one
should be added probably somewhere near the top (eg, just after
AX_XEN_EXPAND_CONFIG).

I'm not sure exactly what you mean in your paragraph I quote above.  I
think you mean that if the user supplies the options on the command
line bugt pkg-config is absent ?  I don't understand why this
situation should be handled differently for zstd than for any of the
other calls to *PKG* (glib, pixman, libnl).

Perhaps you experienced some issue which would have been fixed by the
addition of the missing PKG_PROG_PKG_CONFIG ?

(Also I note that the docs for PKG_CHECK_EXISTS contain a confusing
slip: there it says "you have to call PKG_CHECK_EXISTS manually" but
surely it must mean PKG_PROG_PKG_CONFIG.)

> >>> If you want a warning I think it should be a call to AC_MSG_WARN in
> >>> ACTION-IF-NOT-FOUND.
> >>
> >> I didn't to avoid the nesting of things yielding even harder
> >> to read code.
> > 
> > In your code it's nested too, just in an if rather than the in the
> > macro argument - but with a separate condition.  Please do it the
> > "usual autoconf way".
> 
> Pieces of shell code look to be permitted - a few lines down
> from the addition to configure.ac there is a shell case
> statement. Or are you telling me that's an abuse I shouldn't
> follow? But then I still don't see how to sensibly replace
> the construct, given the issue described further up.

I don't understand what you are getting at.  I think you must have
misunderstood me.

You explained that you preferred not to use the 4th argument,
ACTION-IF-NOT-FOUND, "to avoid nesting".  I was trying to say that I
didn't think this was a good reason and that instead putting the code
in a separate conditional is not warranted here (and not idiomatic).

There is nothing wrong[1] with including (cautious) shell code in
configure.ac, so that was not part of my argument.

> >>>     unziplen = (size_t)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | 
> >>> gzlen[0];
> >>
> >> Okay, I'll copy that then.
> > 
> > Could you make a macro or inline function in xg_private.h[1] rather
> > than open-coding a copy, please ?
> > 
> > [1] Or, if you prefer, a header with wider scope.
> 
> I can, but it feels wrong, in particular if I gave it a
> generic looking name (get_unaligned_le32() or some such,

That would seem perfect to me.  I don't know what would be wrong
with it.

> >>> I mean the inclusion of $libzstd_PKG_ERRORS in the output.
> >>
> >> I see no point in the warning without including this. In fact
> >> I added the AC_MSG_WARN() just so that the contents of this
> >> variable (and hence an indication to the user of what to do)
> >> was easily accessible.
> > 
> > This is not usual autoconf practice.  The usual approach is to
> > consider that missing features are just to be dealt with with a
> > minimum of fuss.
> 
> Which is why I made the description say what it says. Just
> that - as per above - I don't see viable alternatives (yet).

I think we had concluded not to print a warning ?

Ian.

[1] Contrary to the autoconf docs, which were written for a time when
even some very basic shell constructs such as "if" statements were not
always reliable.  In xen.git we can assume a vaguely posix shell.