From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages] Date: Mon, 19 Apr 2010 22:43:14 +0200 Message-ID: <4BCCC062.5080008@amd.com> References: <4BCB76FD.1020103@amd.com> <4BCB791E.7000204@amd.com> <4BCC0F86.8090707@eu.citrix.com> <19404.30856.176804.871256@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <19404.30856.176804.871256@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , Vincent Hanquez , Keir Fraser , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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