Re: [Xen-devel] [PATCH 05/24] golang/xenlight: define KeyValueList builtin type

[George Dunlap <george.dunlap@citrix.com>]

On 10/7/19 4:12 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Define KeyValueList builtin type, analagous to libxl_key_value_list as
> map[string]string, and implement its fromC and toC functions.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> 
>  tools/golang/xenlight/xenlight.go | 33 ++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index 4d4fad2a9d..8196a42855 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -202,11 +202,42 @@ func (chwcap C.libxl_hwcap) toGo() (ghwcap Hwcap) {
>  	return
>  }
>  
> +// KeyValueList represents a libxl_key_value_list.
> +type KeyValueList map[string]string
> +
> +func (kvl KeyValueList) fromC(ckvl *C.libxl_key_value_list) error {
> +	size := int(C.libxl_key_value_list_length(ckvl))
> +	list := (*[1 << 30]*C.char)(unsafe.Pointer(ckvl))[:size:size]
> +
> +	for i := 0; i < size*2; i += 2 {
> +		kvl[C.GoString(list[i])] = C.GoString(list[i+1])
> +	}

It looks like when you use this, you use patterns like this:

	var keyValueListXsdata KeyValueList
	if err := keyValueListXsdata.fromC(&xc.xsdata); err != nil {

But this never calls make(); so won't this crash with a null pointer
deref?  Or am I missing something?

Would it be better to take a pointer method here, and set `*kvl =
make(map[string]string)` before copying the strings over?

That would also very naturally take care of the case where you called
the .fromC() method twice with two different key value lists.  As it is,
if the caller had to initialize it, you'd get a "clobbered union" of the
two lists (where in the case of duplicate keys, the second value
clobbers the first); this way, you only get the most recent list, which
is probably closer to what you wanted.

Also, when going the other direction, how are callers of, say,
libxl_domain_create_new() supposed to initialize this and fill in values?

Looking through the code -- it seems that this type is somewhat
vestigal.  It's only used for two fields of a single struct, and those
fields aren't actually used by xl or libvirt at the moment; and after
some discussion it was determined that anything they might be used to
achieve should probably be done a different way.

So we *could* actually just `type KeyValueList struct { }`, and punt on
all these initialization questions until such time as it turns out that
they're needed.

On the other hand, I think we may need to actually think about
initializing structures.  You've carefully coded DefBool such that the
"zero" value is undefined; but for DevId, for instance, the "initial"
value is supposed to be -1; but the way it's coded, an uninitialized Go
structure will end up as 0, which may be a valid devid.

At the moment, all implemented methods take scalar arguments; but when
they take structs, having  non-default values means "try to get this
specific devid", as opposed to "just choose a free one for me".

Anyway, perhaps we can think about structure initialization, and
implement it after we do the basic structure /  marshalling implementaiton.

In the mean time, we could either keep the KeyValueList you've
implemented here (perhaps adding a make() to the fromC method, and
having toC return NULL if kvl is NULL), or just replace it with a
placeholder until it's needed.

What do you think?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel