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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2021 17:17, Ian Jackson wrote:
> 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.

Well, after Michael's report I had to understand why the
construct behaved the way it does (and not the way I
thought would be sensible), and short of any documentation
clearly saying so I had to go look at the generated shell
code. Which made me notice the apparently (see below)
unhelpful behavior wrt user overrides.

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

Probably, but I don't think I should do so here. I did ask
about making the compression checks x86-only, and that would
be the point where I would have seen the need. But you've
asked for the checks to remain arch-independent. 

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

Ah, looks like I indeed got mislead by the bad indentation
of the generate shell code. So let me try again with [true]
as the 4th argument.

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

The difference is that glib and pixman aren't optional (if
building qemu), i.e. we want configure to fail if they can't
be found or are too old.

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

I don't think so, no, as I've not tried configuring in a way
where the earlier PKG_CHECK_MODULES() would be bypassed.

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

I think the confusion results from my misunderstanding of when
"untried" would result, see above. For that reason I did
consider it necessary to evaluate things once _after_ the
entire construct, rather than inside.

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

Using this (most?) natural name has two issues in my view:
For one, it'll likely cause conflicts with how other code
(using hypervisor files) gets built. And then I consider it
odd to have just one out of a larger set of functions, but
I would consider it odd as well if I had to introduce them
all right here.

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

Yes. Even in the projected new form of using the construct I
don't intend to change the description's wording, as the
intended use of [true] still looks like that can't be intended
usage. IOW my remark extended beyond the warning; I'm sorry if
this did end up confusing because you were referring to just
the warning.

Jan