[v2,02/22] golang/xenlight: define Defbool builtin type

Commit Message

Nick Rosbrook Nov. 15, 2019, 7:44 p.m. UTC

From: Nick Rosbrook <[e-mail]>

Define Defbool as struct analagous to the C type, and define the type
'defboolVal' that represent true, false, and default defbool values.

Implement Set, Unset, SetIfDefault, IsDefault, Val, and String functions
on Defbool so that the type can be used in Go analagously to how its
used in C.

Finally, implement fromC and toC functions.

Signed-off-by: Nick Rosbrook <[e-mail]>
Reviewed-by: George Dunlap <[e-mail]>
---
 tools/golang/xenlight/xenlight.go | 93 +++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

Comments

George Dunlap Dec. 4, 2019, 3:50 p.m. UTC | #1

On 11/15/19 7:44 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <[e-mail]>
> 
> Define Defbool as struct analagous to the C type, and define the type
> 'defboolVal' that represent true, false, and default defbool values.
> 
> Implement Set, Unset, SetIfDefault, IsDefault, Val, and String functions
> on Defbool so that the type can be used in Go analagously to how its
> used in C.
> 
> Finally, implement fromC and toC functions.

The R-b stands, just adding in a comment:

I implemented some tests for these methods to make sure everything
worked as expected (they did); but there's an unexpected side-effect:

-  *_test.go files cannot `import "C"`
- The fromC / toC methods aren't exported

So it's not possible to do the following check:

  var b Defbool

  b.Set(true)
  cb, err := b.toC()
  if ( !C.libxl_defbool_val(cb) ) {
    // report an error
  }

defbool_test.go can't `import "C"`, so it can't call
C.libxl_defbool_val().  We could make an external xenlighttest package,
but that wouldn't be able to call toC().

(I suppose we could write "proxy" functions for every such function we
might want to check, but that seems excessive.)

 -George

Nick Rosbrook Dec. 5, 2019, 3:23 p.m. UTC | #2

> I implemented some tests for these methods to make sure everything
> worked as expected (they did); but there's an unexpected side-effect:
>
> -  *_test.go files cannot `import "C"`

Yeah, this is unfortunate.

> - The fromC / toC methods aren't exported
>
> So it's not possible to do the following check:
>
>   var b Defbool
>
>   b.Set(true)
>   cb, err := b.toC()
>   if ( !C.libxl_defbool_val(cb) ) {
>     // report an error
>   }
>
> defbool_test.go can't `import "C"`, so it can't call
> C.libxl_defbool_val().  We could make an external xenlighttest package,
> but that wouldn't be able to call toC().
>
> (I suppose we could write "proxy" functions for every such function we
> might want to check, but that seems excessive.)

If by "proxy" functions you mean something like:

func libxlDefboolVal(db Defbool) bool {
        return C.libxl_defbool_val(C.libxl_defbool(db))
}

I agree it could be a bit excessive. But, it might be necessary in
order to leverage go test. And, we could make sure that those extra
"proxy" functions are only included in test builds (maybe by making
internal package to house them).

Maybe there is a better solution to this. I'll need to think about it some more.

-NR

George Dunlap Dec. 5, 2019, 3:27 p.m. UTC | #3

On 12/5/19 3:23 PM, Nick Rosbrook wrote:
>> I implemented some tests for these methods to make sure everything
>> worked as expected (they did); but there's an unexpected side-effect:
>>
>> -  *_test.go files cannot `import "C"`
> 
> Yeah, this is unfortunate.
> 
>> - The fromC / toC methods aren't exported
>>
>> So it's not possible to do the following check:
>>
>>   var b Defbool
>>
>>   b.Set(true)
>>   cb, err := b.toC()
>>   if ( !C.libxl_defbool_val(cb) ) {
>>     // report an error
>>   }
>>
>> defbool_test.go can't `import "C"`, so it can't call
>> C.libxl_defbool_val().  We could make an external xenlighttest package,
>> but that wouldn't be able to call toC().
>>
>> (I suppose we could write "proxy" functions for every such function we
>> might want to check, but that seems excessive.)
> 
> If by "proxy" functions you mean something like:
> 
> func libxlDefboolVal(db Defbool) bool {
>         return C.libxl_defbool_val(C.libxl_defbool(db))
> }
> 
> I agree it could be a bit excessive. But, it might be necessary in
> order to leverage go test. And, we could make sure that those extra
> "proxy" functions are only included in test builds (maybe by making
> internal package to house them).
> 
> Maybe there is a better solution to this. I'll need to think about it some more.

I mean, we already copy the source files over somewhere else.  We could
generate ctestproxies.go, and make a xenlighttestable package which
includes that file (and the _test.go files), and a xenlight package
which doesn't.

But I'd say that's a lower priority at this point.

 -George

Patch

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 89ed439fd0..640d82f35f 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -85,6 +85,99 @@  type MemKB uint64
 
 type Uuid C.libxl_uuid
 
+// defboolVal represents a defbool value.
+type defboolVal int
+
+const (
+	defboolDefault defboolVal = 0
+	defboolFalse   defboolVal = -1
+	defboolTrue    defboolVal = 1
+)
+
+// Defbool represents a libxl_defbool.
+type Defbool struct {
+	val defboolVal
+}
+
+func (d Defbool) String() string {
+	switch d.val {
+	case defboolDefault:
+		return "<default>"
+	case defboolFalse:
+		return "False"
+	case defboolTrue:
+		return "True"
+	}
+
+	return ""
+}
+
+// Set sets the value of the Defbool.
+func (d *Defbool) Set(b bool) {
+	if b {
+		d.val = defboolTrue
+		return
+	}
+	d.val = defboolFalse
+}
+
+// Unset resets the Defbool to default value.
+func (d *Defbool) Unset() {
+	d.val = defboolDefault
+}
+
+// SetIfDefault sets the value of Defbool only if
+// its current value is default.
+func (d *Defbool) SetIfDefault(b bool) {
+	if d.IsDefault() {
+		d.Set(b)
+	}
+}
+
+// IsDefault returns true if the value of Defbool
+// is default, returns false otherwise.
+func (d *Defbool) IsDefault() bool {
+	return d.val == defboolDefault
+}
+
+// Val returns the boolean value associated with the
+// Defbool value. An error is returned if the value
+// is default.
+func (d *Defbool) Val() (bool, error) {
+	if d.IsDefault() {
+		return false, fmt.Errorf("%v: cannot take value of default defbool", ErrorInval)
+	}
+
+	return (d.val > 0), nil
+}
+
+func (d *Defbool) fromC(c *C.libxl_defbool) error {
+	if C.libxl_defbool_is_default(*c) {
+		d.val = defboolDefault
+		return nil
+	}
+
+	if C.libxl_defbool_val(*c) {
+		d.val = defboolTrue
+		return nil
+	}
+
+	d.val = defboolFalse
+
+	return nil
+}
+
+func (d *Defbool) toC() (C.libxl_defbool, error) {
+	var c C.libxl_defbool
+
+	if !d.IsDefault() {
+		val, _ := d.Val()
+		C.libxl_defbool_set(&c, C.bool(val))
+	}
+
+	return c, nil
+}
+
 type Context struct {
 	ctx    *C.libxl_ctx
 	logger *C.xentoollog_logger_stdiostream