[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |