[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
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> 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. Why not do as was done for bz2, lzma, lzo2 ? Printing the errors to configure's terminal is not normally done, either. > 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. > 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. 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. Thanks, Ian.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |