[05/24] golang/xenlight: define KeyValueList builtin type

Commit Message

Nick Rosbrook Oct. 7, 2019, 3:12 p.m. UTC

From: Nick Rosbrook <[e-mail]>

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 <[e-mail]>
---
Cc: George Dunlap <[e-mail]>
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>

 tools/golang/xenlight/xenlight.go | 33 ++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

Comments

George Dunlap Oct. 24, 2019, 4:34 p.m. UTC | #1

On 10/7/19 4:12 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> Cc: George Dunlap <[e-mail]>
> Cc: Ian Jackson <[e-mail]>
> Cc: Wei Liu <[e-mail]>
> 
>  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

Nick Rosbrook Oct. 24, 2019, 7:54 p.m. UTC | #2

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

If there is no clear need for this type to be implemented in the Go
package, then I would be in favor of not doing so. IMO, a smaller,
more focused package is ideal.

> 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.
>
> [...]
>
> Anyway, perhaps we can think about structure initialization, and
> implement it after we do the basic structure /  marshalling implementaiton.

That's probably best. However, at a quick glance it seems like it
would be pretty straight-forward to generate NewStructType functions
analogous to libxl_struct_type_init, if that's the desired behavior.

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

Based on what you said above, I think I would like to drop the
implementation for now. But, if we keep the current implementation, I
will make those corrections.

-NR

George Dunlap Oct. 25, 2019, 11:26 a.m. UTC | #3

> On Oct 24, 2019, at 8:54 PM, Nick Rosbrook <[e-mail]> wrote:
> 
>> 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.
> 
> If there is no clear need for this type to be implemented in the Go
> package, then I would be in favor of not doing so. IMO, a smaller,
> more focused package is ideal.

Ok, in that case let’s just leave the struct empty.

> 
>> 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.
>> 
>> [...]
>> 
>> Anyway, perhaps we can think about structure initialization, and
>> implement it after we do the basic structure /  marshalling implementaiton.
> 
> That's probably best. However, at a quick glance it seems like it
> would be pretty straight-forward to generate NewStructType functions
> analogous to libxl_struct_type_init, if that's the desired behavior.

I think we basically have three options:

1. Try to arrange it so that the “zero” values correspond to “default” values in libxl; i.e., have DevID 0 -> libxl_devid -1, DevID 1 -> libxl_devid 0, &c

2. Add NewStructType functions

3. Add .Init() methods to structs

#1 I think is probably too risky; so 2 or 3 (or maybe both) will probably be needed.  The NewStructType() seems to be more standard.  But I’m open so suggestions. :-)

 -George

Nick Rosbrook Oct. 25, 2019, 12:30 p.m. UTC | #4

> Ok, in that case let’s just leave the struct empty.

Ok, sounds like a plan.

> I think we basically have three options:
>
> 1. Try to arrange it so that the “zero” values correspond to “default” values in libxl; i.e., have DevID 0 -> libxl_devid -1, DevID 1 -> libxl_devid 0, &c
>
> 2. Add NewStructType functions
>
> 3. Add .Init() methods to structs
>
> #1 I think is probably too risky; so 2 or 3 (or maybe both) will probably be needed.  The NewStructType() seems to be more standard.  But I’m open so suggestions. :-)

Option 2 is certainly the standard, and best to avoid confusing
.Init() functions with the special function init(). I'll work on the
implementation as soon as we get this series done :)

-NR

Patch

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])
+	}
+
+	return nil
+}
+
+func (kvl KeyValueList) toC() (C.libxl_key_value_list, error) {
+	// Add extra slot for sentinel
+	var char *C.char
+	csize := 2*len(kvl) + 1
+	ckvl := (C.libxl_key_value_list)(C.malloc(C.ulong(csize) * C.ulong(unsafe.Sizeof(char))))
+	clist := (*[1 << 31]*C.char)(unsafe.Pointer(ckvl))[:csize:csize]
+
+	i := 0
+	for k, v := range kvl {
+		clist[i] = C.CString(k)
+		clist[i+1] = C.CString(v)
+		i += 2
+	}
+	clist[len(clist)-1] = nil
+
+	return ckvl, nil
+}
+
 // typedef struct {
 //     uint32_t size;          /* number of bytes in map */
 //     uint8_t *map;
 // } libxl_bitmap;
-
 // Implement the Go bitmap type such that the underlying data can
 // easily be copied in and out.  NB that we still have to do copies
 // both directions, because cgo runtime restrictions forbid passing to