[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 12:30, Ian Jackson wrote: > Hi. Thanks for this. Firstly, RM hat: this is the tools half of zstd > decompression support which I think is a blocker for the release. So > I am going to waive the last posting date requirement. Therefore, > > Assuming it's committed this week: > > Release-Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx> Thanks. > Secondly, I think it would be sensible for me to review it: > >> As far as configure.ac goes, I'm pretty sure there is a better (more >> "standard") way of using PKG_CHECK_MODULES(). > > Yes, what you have done is rather unidiomatic and seems to rely on > undocumented internals of the PKG_*. macros. Which specific part of the construct are you referring to? I didn't think I used anything outright undocumented. Of course I did have some trouble finding suitable docs, but in the end I managed to locate at least something that I was able to grok. > Why not do as was done for bz2, lzma, lzo2 ? Because the pkg-config approach is more flexible - aiui AC_CHECK_HEADER() and AC_CHECK_LIB() won't find a dependency when sitting in some custom place, which the *.pc files are specifically supposed to cover for. > Printing the errors to configure's terminal is > not normally done, either. With this you mean the AC_MSG_WARN()? I'm okay to drop it; I was actually half tempted to myself already, but thought having it would be better in line with PKG_CHECK_MODULES() when not passed a 4th argument (where it gets quite verbose, but of course also fails the configure process altogether). >> The construct also gets >> put next to the other decompression library checks, albeit I think they >> all ought to be x86-specific (e.g. placed in the existing case block a >> few lines down). > > I don't understand why there is an x86-specific angle here. On a "normal" libxenguest build decompression is available only on x86, because of SRCS-$(CONFIG_X86) += xg_dom_bzimageloader.c Hence the dependencies thereof also only ought to need checking on x86. I have to admit I'm uncertain about the stubdom build. I was merely implying that if decompression is unavailable in "normal" builds outside of x86, then _if_ non-x86 builds of stubdom exist in the first place, decompression code there is at best dead (the quoted restriction from Makefile applies in this case too, and hence I can't see callers of that code, despite ifeq ($(CONFIG_LIBXC_MINIOS),y) SRCS-y += xg_dom_decompress_unsafe.c SRCS-y += xg_dom_decompress_unsafe_bzip2.c SRCS-y += xg_dom_decompress_unsafe_lzma.c SRCS-y += xg_dom_decompress_unsafe_lzo1x.c SRCS-y += xg_dom_decompress_unsafe_xz.c SRCS-y += xg_dom_decompress_unsafe_zstd.c endif not restricting it to x86). >> This follows the logic used for other decompression methods utilizing an >> external library, albeit here we can't ignore the 32-bit size field >> appended to the compressed image - its presence causes decompression to >> fail. Leverage the field instead to allocate the output buffer in one >> go, i.e. without incrementally realloc()ing. > >> + insize = *size - 4; >> + outsize = *(uint32_t *)(*blob + insize); > > Potentiallty unaligned access. IDK if this kind of thing is thought > OK in hypervisor code but it it's not sufficiently portable for tools. Also a possible endianness issue, yes. Since as per above this code gets used on x86 only, I thought this would be fine at least for now. In fact before using this simplistic approach I did check whether xg_dom_bzimageloader.c had suitable abstraction available, yet I couldn't spot any. > The rest of this code looks OK to me. I spent quite a while trying to > figure out the memory management / ownership rules for the interface > to these decompression functions. This business where they all > allocate a new buffer, and overwrite their input pointer with it (but > only on success), is pretty nasty. I wasn't able to find where the > old buffer was freed. But the other decompressors all seem to work > the same way. Urgh. In summary: nasty, but, this new code seems to > follow the existing convension. Yes, this isn't pretty, but looks to have served the purpose. I'd be happy to see it improved, but I'm afraid beyond what's in this series I won't have much time to help the overall situation. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |