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
|
Home