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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.


+#define FREE_IF_NOT_ZERO(ptr) if((ptr) != NULL) {free(ptr); ptr = NULL;}

This is redundant because  free(NULL)  is a legal no-op.  It is also
bad macro practice to include an unterminated if like that - it might
eat subsequent elses.  Furthermore, you should wrap ptr up in parens
to avoid macro precedence problems.

So you should write something like one of these:

+#define FREE_AND_ZERO(ptr)  (free((ptr), (ptr) = 0)
+#define FREE_AND_ZERO(ptr)  do{ free((ptr)); (ptr) = 0; }while(0)

Finally, this macro should be in libxl_internal.h where every part of
libxl can use it.


Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add version_info 
function"):
> instead of this chain of if else, why don't you just init the structure 
> to NULL at the beginning of the call, and not do any else ?

Yes.

Furthermore the documentation comment should make it clear that the
version info structure can be undefined beforehand (and the reader can
therefore conclude that libxl_get_version_info won't free it).

> last thing, is instead of using strdup, you should use 
> libxl_sprintf(ctx, "%s", string) so that you don't have to worry about
> freeing memory at all, and you can just omit the free_version_info call 
> completely.

I don't think that's correct because memory allocated by libxl_sprintf
does not survive return from libxl into the caller.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel