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

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

The quoted-reply part of this message may be going off into the weeds.
Feel free to ignore it, or parts of it, if you think you can make
progress without disabusing me of what I think are my
misunderstandings...

Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed 
kernels"):
> On 25.01.2021 17:17, Ian Jackson wrote:
> >  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.

Yes, but I think that just means adding the [true] fourth argument, to
make failure to find the library a no-op.  I don't think it needs any
more complex handling.  At least, I have not yet understood a need for
more complex handling...

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

I guess I should take care of this then, since I think it's probably
an accident waiting to happen.

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

If you put this new definition in xg_private.h for now then it ought
not to conflict with anyone else.  (Assuming it's a static inline, or
a macro.  If it will have external linkage then it will need a name
prefix which I would prefer to avoid.)

I think it best to add this one macro/inline now.  When someone wants
more of these, they can add them.  If someone want them elsewhere they
can do the work of finding or making a suitably central place.  If it
weren't for the release timing I might think it better to add more,
but my general rule is that steps towards the best possible situation
are better than steps that go away from the best possible situation,
even if the former are not complete.

I think a reasonable alternative would be to arrange to import a
comprehensive set from somewhere.

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

I'm afraid I don't understand what you mean.  In particular, what you
mean by "the intended use of [true] still looks like that can't be
intended usage".

  the intended {by whom for what puropose?} use of [true] still looks
  like that {what?} can't be intended {by whom?} usage

I have the feeling that I have totally failed to grasp your mental
model, which naturally underlies your comments.

Do you mean that with "true" for the 4th argument, the printed output
is not correct, in the failure case ?  Maybe it needs a call to AC_MSG
or something (but AIUI most of these PKG_* macros ought to do that for
us).  I'm just guessing at your meaing 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).

Quoting this because I think it may still be relevant for
understanding the foregoing...

Ian.