Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Andre Przywara <andre.przywara@amd.com>]

Ian Jackson wrote:
> Andre Przywara writes ("[Xen-devel] [PATCH 3/4] libxl: add version_info function"):
>> Xen provides a xen_version hypercall to query the values of several
>> interesting things (like hypervisor version, commandline used,
>> actual changeset, etc.). Create a user-friendly and efficient
>> wrapper around the libxc function to provide values for xl info output.
> 
> As Vincent says, I think the query mask is overkill.  I would suggeset
> just doing without it, and always acquire and return all the
> information.  Ie, do away with query_mask and LIBXL_VERSION_FOO_MASK
> and the correspondings ifs.
I am not fully convinced of this. There is quite a lot of information 
copied (up to 4KB), and some fields (like pagesize or the version 
numbers) are just plain integers. And this approach just propagates the 
underlying design.
In the xl info implementation I used this feature to just get the 
pagesize. One could think of just querying the version number, too. If 
you don't like the additional parameter, what about a wrapping macro, then?
I don't see the problem with the complexity, though, as it is hidden 
inside the library.
I can use libxl_sprintf function to get rid of the free() function, but 
I personally don't like the idea of accumulated mallocs much, left alone 
the "hidden" free operation.

I will incorporate your other comments.

Thanks for the review!

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12