[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.