[08/24] golang/xenlight: define Mac builtin type

Commit Message

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

From: Nick Rosbrook <[e-mail]>

Define Mac as [6]byte and implement fromC, toC, and String 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 | 35 +++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

George Dunlap Nov. 13, 2019, 3:51 p.m. UTC | #1

On 10/7/19 4:12 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <[e-mail]>
> 
> Define Mac as [6]byte and implement fromC, toC, and String 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 | 35 +++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index a3a1836d31..3b7824b284 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -181,6 +181,41 @@ func (d *Defbool) toC() (C.libxl_defbool, error) {
>  	return c, nil
>  }
>  
> +// Mac represents a libxl_mac, or simply a MAC address.
> +type Mac [6]byte
> +
> +// String formats a Mac address to string representation.
> +func (mac Mac) String() string {
> +	s := "%x:%x:%x:%x:%x:%x"
> +	opts := make([]interface{}, 6)
> +
> +	for i, v := range mac {
> +		opts[i] = v
> +	}

What's the point of this?

I realize it's slightly annoying to have to type `mac[0], mac[1], ...`,
but I'd rather do that once than make the runtime copy everything over
into a slice of interfaces every String() call.

Also, I guess the format should be "%02x".

> +
> +	return fmt.Sprintf(s, opts...)
> +}
> +
> +func (mac *Mac) fromC(cmac *C.libxl_mac) error {
> +	b := (*[6]C.uint8_t)(unsafe.Pointer(cmac))
> +
> +	for i, v := range b {
> +		mac[i] = byte(v)
> +	}
> +
> +	return nil
> +}
> +
> +func (mac *Mac) toC() (C.libxl_mac, error) {

Conversely, shouldn't this be a value receiver, since we're don't want
this function to change the contents of mac?

Thanks,
 -George

Nick Rosbrook Nov. 13, 2019, 9:50 p.m. UTC | #2

> What's the point of this?
>
> I realize it's slightly annoying to have to type `mac[0], mac[1], ...`,
> but I'd rather do that once than make the runtime copy everything over
> into a slice of interfaces every String() call.

As I think you realized by looking at subsequent patches, this is to
get around the fact that "an array of an interface type != an array of
type that implements that interface." Since this is a small array, I'm
fine with explicitly passing each element of the array to fmt.Sprintf.

> Also, I guess the format should be "%02x".

Yeah, thanks.

> Conversely, shouldn't this be a value receiver, since we're don't want
> this function to change the contents of mac?

Conventionally receivers are kept consistent between methods of a
type, unless it's implementing some interface like Stringer. And, it's
consistent with the other toC functions which are defined with
pointers in the generated functions since there are some large
structs.

But, yes this could just be a value receiver. I don't have a strong
opinion to keep it as is, so I'll change it.

-NR

George Dunlap Nov. 14, 2019, 12:27 p.m. UTC | #3

On 11/13/19 9:50 PM, Nick Rosbrook wrote:
>> What's the point of this?
>>
>> I realize it's slightly annoying to have to type `mac[0], mac[1], ...`,
>> but I'd rather do that once than make the runtime copy everything over
>> into a slice of interfaces every String() call.
> 
> As I think you realized by looking at subsequent patches, this is to
> get around the fact that "an array of an interface type != an array of
> type that implements that interface." Since this is a small array, I'm
> fine with explicitly passing each element of the array to fmt.Sprintf.

Using `mac[0], mac[1], ...` for this one is kind of just on the border
of reasonable, but when I got to a later patch, I felt like `uuid[0],
uuid[1], ... uuid[15]` was a bit much.  And on further reflection, the
fact is that all of these will be changed to interface{} under the hood
by the function call and then reflect'ed to determine their type anyway;
i.e., I'm pretty sure:

    fmt.Sprintf(s, mac[0], mac[1], /* &c */)

from a compiler perspective is basically identical to:

    fmt.Sprintf(s, []interface{}{mac[0], mac[1], /* &c */}...)

So the code you have is probably going to be about equally efficient anyway.

So I guess, unless you feel strongly that using `mac[0], mac[1], ...` is
a better way to go, maybe just leave it the way it is for now.

 -George

Nick Rosbrook Nov. 14, 2019, 3:34 p.m. UTC | #4

> So the code you have is probably going to be about equally efficient anyway.

A quick benchmark [1] shows:

goos: linux
goarch: amd64
BenchmarkString1-8        5000000           251 ns/op
BenchmarkString2-8        5000000           247 ns/op

So yes, they're about the same :)

I'll leave it as is.

-NR

[1] https://play.golang.org/p/2cOzBpoTfgE

Patch

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index a3a1836d31..3b7824b284 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -181,6 +181,41 @@  func (d *Defbool) toC() (C.libxl_defbool, error) {
 	return c, nil
 }
 
+// Mac represents a libxl_mac, or simply a MAC address.
+type Mac [6]byte
+
+// String formats a Mac address to string representation.
+func (mac Mac) String() string {
+	s := "%x:%x:%x:%x:%x:%x"
+	opts := make([]interface{}, 6)
+
+	for i, v := range mac {
+		opts[i] = v
+	}
+
+	return fmt.Sprintf(s, opts...)
+}
+
+func (mac *Mac) fromC(cmac *C.libxl_mac) error {
+	b := (*[6]C.uint8_t)(unsafe.Pointer(cmac))
+
+	for i, v := range b {
+		mac[i] = byte(v)
+	}
+
+	return nil
+}
+
+func (mac *Mac) toC() (C.libxl_mac, error) {
+	var cmac C.libxl_mac
+
+	for i, v := range mac {
+		cmac[i] = C.uint8_t(v)
+	}
+
+	return cmac, nil
+}
+
 type Context struct {
 	ctx    *C.libxl_ctx
 	logger *C.xentoollog_logger_stdiostream