[v3,8/8] RFC: Sketch constructors, DomainCreateNew
diff mbox series

Message ID 20200117155734.1067550-8-george.dunlap@citrix.com
State New
Headers show
Series
  • [v3,1/8] golang/xenlight: Don't try to marshall zero-length arrays in fromC
Related show

Commit Message

George Dunlap Jan. 17, 2020, 3:57 p.m. UTC
This is a sketch of functionality suitable for creating a basic
domain, with a disk and a vif.  DomainConfig, DeviceDisk, and
DeviceNic types are all created using constructor functions, which
initialize them with libxl's defaults.

DomainCreateNew takes the config and calls without any updates.

Obviously some of these will need to be changed it we switch to
passing references to .toC() rather than passing back by value.

The main purpose of this is to allow testing of creating a hard-coded
domain.

Creating a domain would look like this:

	// type = "pv"
	dconf, err := xl.NewDomainConfig(xl.DomainTypePv)
	if err != nil {
		fmt.Printf("NewDomainConfig: %v\n", err)
		return
	}
	dconf.CInfo.Type = xl.DomainTypePv
	// name = "c6-01"
	dconf.CInfo.Name = "c6-01"
	// vcpus=4
	dconf.BInfo.MaxVcpus = 4
	// memory = "2048"
	dconf.BInfo.MaxMemkb = 2048 * 1024
	dconf.BInfo.TargetMemkb = 2048 * 1024
	// on_crash = 'destroy'
	dconf.OnCrash = xl.ActionOnShutdownDestroy
	// bootloader = "pygrub"
	dconf.BInfo.Bootloader = "pygrub"
	// disk = [ 'vdev=hda,format=raw,target=/images/c6-01.raw']
	{
		disk, err := xl.NewDeviceDisk()
		if err != nil {
			fmt.Printf("NewDeviceDisk: %v\n", err)
			return
		}
		disk.Vdev = "hda"
		//disk.DiskBackend = xl.DiskBackendPhy
		disk.Format = xl.DiskFormatRaw
		disk.Readwrite = 1
		disk.PdevPath = "/images/c6-01.raw"
		dconf.Disks = append(dconf.Disks, *disk)
	}
	// vif = [ 'mac=5a:5b:d6:f1:d6:b4' ]
	{
		vif, err := xl.NewDeviceNic()
		if err != nil {
			fmt.Printf("NewDeviceNic: %v\n", err)
			return
		}
		vif.Mac = xl.Mac{ 0x5a, 0x5b, 0xd6, 0x31, 0xd6, 0xb4 }
		dconf.Nics = append(dconf.Nics, *vif)
	}
	// serial='pty' # HVM only

	did, err := ctx.DomainCreateNew(dconf)

	if err != nil {
		fmt.Printf("Creating domain: %v\n", err)
		return
	}

	fmt.Printf("Domain %s(%d) created successfully", dconf.CInfo.Name, did)


Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/xenlight.go | 66 +++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Comments

George Dunlap Jan. 17, 2020, 6:38 p.m. UTC | #1
On 1/17/20 3:57 PM, George Dunlap wrote:
> This is a sketch of functionality suitable for creating a basic
> domain, with a disk and a vif.  DomainConfig, DeviceDisk, and
> DeviceNic types are all created using constructor functions, which
> initialize them with libxl's defaults.
> 
> DomainCreateNew takes the config and calls without any updates.
> 
> Obviously some of these will need to be changed it we switch to
> passing references to .toC() rather than passing back by value.
> 
> The main purpose of this is to allow testing of creating a hard-coded
> domain.
> 
> Creating a domain would look like this:
> 
> 	// type = "pv"
> 	dconf, err := xl.NewDomainConfig(xl.DomainTypePv)
> 	if err != nil {
> 		fmt.Printf("NewDomainConfig: %v\n", err)
> 		return
> 	}
> 	dconf.CInfo.Type = xl.DomainTypePv
> 	// name = "c6-01"
> 	dconf.CInfo.Name = "c6-01"
> 	// vcpus=4
> 	dconf.BInfo.MaxVcpus = 4
> 	// memory = "2048"
> 	dconf.BInfo.MaxMemkb = 2048 * 1024
> 	dconf.BInfo.TargetMemkb = 2048 * 1024
> 	// on_crash = 'destroy'
> 	dconf.OnCrash = xl.ActionOnShutdownDestroy
> 	// bootloader = "pygrub"
> 	dconf.BInfo.Bootloader = "pygrub"
> 	// disk = [ 'vdev=hda,format=raw,target=/images/c6-01.raw']
> 	{
> 		disk, err := xl.NewDeviceDisk()
> 		if err != nil {
> 			fmt.Printf("NewDeviceDisk: %v\n", err)
> 			return
> 		}
> 		disk.Vdev = "hda"
> 		//disk.DiskBackend = xl.DiskBackendPhy
> 		disk.Format = xl.DiskFormatRaw
> 		disk.Readwrite = 1
> 		disk.PdevPath = "/images/c6-01.raw"
> 		dconf.Disks = append(dconf.Disks, *disk)
> 	}
> 	// vif = [ 'mac=5a:5b:d6:f1:d6:b4' ]
> 	{
> 		vif, err := xl.NewDeviceNic()
> 		if err != nil {
> 			fmt.Printf("NewDeviceNic: %v\n", err)
> 			return
> 		}
> 		vif.Mac = xl.Mac{ 0x5a, 0x5b, 0xd6, 0x31, 0xd6, 0xb4 }
> 		dconf.Nics = append(dconf.Nics, *vif)
> 	}
> 	// serial='pty' # HVM only
> 
> 	did, err := ctx.DomainCreateNew(dconf)
> 
> 	if err != nil {
> 		fmt.Printf("Creating domain: %v\n", err)
> 		return
> 	}
> 
> 	fmt.Printf("Domain %s(%d) created successfully", dconf.CInfo.Name, did)
> 
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Nick Rosbrook <rosbrookn@ainfosec.com>
> ---
>  tools/golang/xenlight/xenlight.go | 66 +++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> index c462e4bb42..5a21a2b9b8 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -1113,3 +1113,69 @@ func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
>  	path = C.GoString(cpath)
>  	return
>  }
> +
> +func NewDomainConfig(t DomainType) (*DomainConfig, error) {
> +	var cconfig C.libxl_domain_config
> +
> +	C.libxl_domain_config_init(&cconfig)
> +	C.libxl_domain_build_info_init_type(&cconfig.b_info, C.libxl_domain_type(t))
> +
> +	gconfig := &DomainConfig{}
> +	err := gconfig.fromC(&cconfig)
> +	if err != nil {
> +		return nil, err
> +	}
> +
> +	return gconfig, nil
> +}
> +
> +func NewDeviceDisk() (*DeviceDisk, error) {
> +	var ctype C.libxl_device_disk
> +
> +	C.libxl_device_disk_init(&ctype)
> +
> +	gtype := &DeviceDisk{}
> +	err := gtype.fromC(&ctype)
> +
> +	if err != nil {
> +		return nil, err
> +	}
> +
> +	return gtype, nil
> +}
> +
> +func NewDeviceNic() (*DeviceNic, error) {
> +	var ctype C.libxl_device_nic
> +
> +	C.libxl_device_nic_init(&ctype)
> +
> +	gtype := &DeviceNic{}
> +	err := gtype.fromC(&ctype)
> +
> +	if err != nil {
> +		return nil, err
> +	}
> +
> +	return gtype, nil
> +}
> +
> +// int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
> +//                             uint32_t *domid,
> +//                             const libxl_asyncop_how *ao_how,
> +//                             const libxl_asyncprogress_how *aop_console_how)
> +func (Ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> +	var cdomid C.uint32_t
> +	var cconfig C.libxl_domain_config
> +	err := config.toC(&cconfig)
> +	if err != nil {
> +		return Domid(0), fmt.Errorf("converting domain config to C: %v", err)
> +	}
> +	defer C.libxl_domain_config_dispose(&cconfig)
> +
> +	ret := C.libxl_domain_create_new(Ctx.ctx, &cconfig, &cdomid, nil, nil)
> +	if ret != 0 {
> +		return Domid(0), Error(ret)
> +	}
> +
> +	return Domid(cdomid), nil
> +}

An alternate way to do this would be something like this:

---
func NewDomainConfig(t DomainType, populate func(*DomainConfig))
*DomainConfig {
	var ctype C.libxl_domain_config

	C.libxl_domain_config_init(&ctype)
	C.libxl_domain_build_info_init_type(&ctype.b_info, C.libxl_domain_type(t))

	gtype := &DomainConfig{}
	err := gtype.fromC(&ctype)
	if err != nil {
		panic("internal error: Can't convert empty DomainConfig")
	}

	if populate != nil {
		populate(gtype)
	}

	return gtype
}

func NewDeviceDisk(populate func(*DeviceDisk)) *DeviceDisk {
	var ctype C.libxl_device_disk

	C.libxl_device_disk_init(&ctype)

	gtype := &DeviceDisk{}
	err := gtype.fromC(&ctype)

	if err != nil {
		panic("internal error: Can't convert empty DeviceDisk")
	}

	if populate != nil {
		populate(gtype)
	}

	return gtype
}

func NewDeviceNic(populate func(*DeviceNic)) *DeviceNic {
	var ctype C.libxl_device_nic

	C.libxl_device_nic_init(&ctype)

	gtype := &DeviceNic{}
	err := gtype.fromC(&ctype)

	if err != nil {
		panic("internal error: Can't convert empty DeviceNic")
	}

	if populate != nil {
		populate(gtype)
	}

	return gtype
}
---

And then code to populate a domain config might look like this:

	dconf := xl.NewDomainConfig(xl.DomainTypePv, func(dconf *xl.DomainConfig) {
		dconf.CInfo.Type = xl.DomainTypePv
		// name = "c6-01"
		dconf.CInfo.Name = "c6-01"
		// vcpus=4
		dconf.BInfo.MaxVcpus = 4
		// memory = "2048"
		dconf.BInfo.MaxMemkb = 2048 * 1024
		dconf.BInfo.TargetMemkb = 2048 * 1024
		// on_crash = 'destroy'
		dconf.OnCrash = xl.ActionOnShutdownDestroy
		// bootloader = "pygrub"
		dconf.BInfo.Bootloader = "pygrub"
	})

	dconf.Disks = []xl.DeviceDisk{*xl.NewDeviceDisk(func(disk *xl.DeviceDisk) {
		disk.Vdev = "hda"
		//disk.DiskBackend = xl.DiskBackendPhy
		disk.Format = xl.DiskFormatRaw
		disk.Readwrite = 1
		disk.PdevPath = "/images/c6-01.raw"
		dconf.Disks = append(dconf.Disks, *disk)
	})}

	dconf.Nics = []xl.DeviceNic{*xl.NewDeviceNic(func(vif *xl.DeviceNic) {
		vif.Mac = xl.Mac{0x5a, 0x5b, 0xd6, 0x31, 0xd6, 0xb4}
	})}
George Dunlap Jan. 22, 2020, 10:32 a.m. UTC | #2
On 1/17/20 3:57 PM, George Dunlap wrote:
> This is a sketch of functionality suitable for creating a basic
> domain, with a disk and a vif.  DomainConfig, DeviceDisk, and
> DeviceNic types are all created using constructor functions, which
> initialize them with libxl's defaults.
> 
> DomainCreateNew takes the config and calls without any updates.
> 
> Obviously some of these will need to be changed it we switch to
> passing references to .toC() rather than passing back by value.
> 
> The main purpose of this is to allow testing of creating a hard-coded
> domain.

BTW, just in case anyone decides to try this -- this will create a
domain in the "paused" state, and it does not create a "reaper" for it
when the domain shuts down.  You have to manually `xl unpause` the
domain for it to run, and then `xl destroy` the domain after it shuts down.

The first is just an implementation detail; the second requires adding
the ability to receive events, which will be the next "level" of libxl
event handling support.

 -George
Nick Rosbrook Jan. 24, 2020, 7:32 p.m. UTC | #3
Sorry for the late reply on this one.

> +func NewDomainConfig(t DomainType) (*DomainConfig, error) {
> +       var cconfig C.libxl_domain_config
> +
> +       C.libxl_domain_config_init(&cconfig)
> +       C.libxl_domain_build_info_init_type(&cconfig.b_info, C.libxl_domain_type(t))
> +
> +       gconfig := &DomainConfig{}
> +       err := gconfig.fromC(&cconfig)
> +       if err != nil {
> +               return nil, err
> +       }
> +
> +       return gconfig, nil
> +}

I think this is sufficient for the "New" functions; simple is probably
better here. I appreciate the idea of the `populate func` approach you
mentioned in another email, but I think in practice people will want
to leverage encoding/json or otherwise to unmarshal some data into a
DomainConfig etc. Or, we would hopefully be able to unmarshal an
xl.cfg. All that is to say, I think the approach you have shown here
gives us and package users the most flexibility.

Do you have any thoughts on supporting unmarshaling json, xl.cfg, etc.?

If we stick with this outline for constructors, they will be easy to
generate. I'm happy to work on that, unless you were already planning
on it.

Thanks,
-NR
George Dunlap Jan. 27, 2020, 6:08 p.m. UTC | #4
On 1/24/20 7:32 PM, Nick Rosbrook wrote:
> Sorry for the late reply on this one.
> 
>> +func NewDomainConfig(t DomainType) (*DomainConfig, error) {
>> +       var cconfig C.libxl_domain_config
>> +
>> +       C.libxl_domain_config_init(&cconfig)
>> +       C.libxl_domain_build_info_init_type(&cconfig.b_info, C.libxl_domain_type(t))
>> +
>> +       gconfig := &DomainConfig{}
>> +       err := gconfig.fromC(&cconfig)
>> +       if err != nil {
>> +               return nil, err
>> +       }
>> +
>> +       return gconfig, nil
>> +}
> 
> I think this is sufficient for the "New" functions; simple is probably
> better here. I appreciate the idea of the `populate func` approach you
> mentioned in another email, but I think in practice people will want
> to leverage encoding/json or otherwise to unmarshal some data into a
> DomainConfig etc. Or, we would hopefully be able to unmarshal an
> xl.cfg. All that is to say, I think the approach you have shown here
> gives us and package users the most flexibility.

I think marshaling and unmarshalling should be fine, *as long as* the
structure being unmarshalled actually went through the
libxl_<type>_init() function first.

In fact, I was kicking around the idea of adding a non-exported field to
all the generated structs -- maybe "bool initalized" -- and having the
.fromC() methods set this to 'true', and the .toC() methods return an
error if it wasn't set.  But then we'd need to write custom JSON
marshallers to handle these.

I'm not personally very interested in parsing xl.cfg files, but libxl
has library functions to do that, so it should be something very easy to
add if you want.  (Although in fact, it looks like a lot of the code to
actually interpret the results of the parsing is in xl; we might want to
see about moving some of that functionality into libxlu.)

> If we stick with this outline for constructors, they will be easy to
> generate. I'm happy to work on that, unless you were already planning
> on it.

I'm afraid my 1 day a week of coding is going to have to be diverted to
something else for a month or so; so please go ahead if you have the time.

As far as further steps -- do you have a clear idea what kind of
functionality you'd like to see possible by the time of the feature
freeze (probably in May)?  Do you have plans to use these bindings
yourself, and if so, how?

For my part, I want to start and reap guests.  The latter will require
adding event callback functionality which will require more thought (and
perhaps expose more libxl issues).  But I don't yet have a clear target
beyond that.

Re function calls -- do we want to just manually duplicate them as
needed, or try to get some sort of IDL support?

Other work items include:

* modifying configure to detect the availability of go and enable the
bindings if it's available

* Enabling go build testing in the gitlab CI loop

* Making `go get` work, which might involve having code to push
post-generated code to a repo and tagging as appropriate

 -George
Nick Rosbrook Jan. 28, 2020, 8:41 p.m. UTC | #5
> I think marshaling and unmarshalling should be fine, *as long as* the
> structure being unmarshalled actually went through the
> libxl_<type>_init() function first.
>
> In fact, I was kicking around the idea of adding a non-exported field to
> all the generated structs -- maybe "bool initalized" -- and having the
> .fromC() methods set this to 'true', and the .toC() methods return an
> error if it wasn't set.  But then we'd need to write custom JSON
> marshallers to handle these.

I *think* to guarantee that libxl_<type>_init() has been called when
unmarshaling, we would need to generate UnmarshalJSON functions to
implement json.Unmarshaler. E.g.,:

func (dd *DeviceDisk) UnmarshalJSON(data []byte) error {
        if dd == nil { // Or maybe this is the 'initialized' check you mentioned
                dd, err := NewDeviceDisk()
                if err != nil {
                        return err
                }
        }

        return json.Unmarshal(data, dd)
}

AFAICT, this would be required for someone to unmarshal a complete
domain configuration in one go.

> I'm not personally very interested in parsing xl.cfg files, but libxl
> has library functions to do that, so it should be something very easy to
> add if you want.  (Although in fact, it looks like a lot of the code to
> actually interpret the results of the parsing is in xl; we might want to
> see about moving some of that functionality into libxlu.)

Okay, noted.

> > If we stick with this outline for constructors, they will be easy to
> > generate. I'm happy to work on that, unless you were already planning
> > on it.
>
> I'm afraid my 1 day a week of coding is going to have to be diverted to
> something else for a month or so; so please go ahead if you have the time.

Okay, I think I can manage this fairly easily.

> As far as further steps -- do you have a clear idea what kind of
> functionality you'd like to see possible by the time of the feature
> freeze (probably in May)?  Do you have plans to use these bindings
> yourself, and if so, how?
>
> For my part, I want to start and reap guests.  The latter will require
> adding event callback functionality which will require more thought (and
> perhaps expose more libxl issues).  But I don't yet have a clear target
> beyond that.

Yes, I plan on using these bindings in redctl (our Redfield toolstack)
[1], to replace our os/exec calls to xl. To fully make that
transition, we would need domain start/stop, PCI and network
attach/detach, as well as some utilities (most of which are either
implemented, or would be easy to do). But, making that transition is
relatively low on the priority list right now, so I can't commit to a
timeline unfortunately.

> Re function calls -- do we want to just manually duplicate them as
> needed, or try to get some sort of IDL support?

I think it will make more sense to manually duplicate them as needed.
That way, we can be more particular about function signatures etc. to
ensure they are idiomatic Go. Also, I am of the opinion that a minimal
API is a better place to start. Which brings me to another question
and potential work item:

Do we want to re-evaluate what is currently implemented in the API? Do
you have plans to use everything that is currently there? If not, it
might be nice to trim off things we don't need right now.

> Other work items include:
>
> * modifying configure to detect the availability of go and enable the
> bindings if it's available
>
> * Enabling go build testing in the gitlab CI loop
>
> * Making `go get` work, which might involve having code to push
> post-generated code to a repo and tagging as appropriate

I was going to ask about this. You had a vanity URL in place at one
point, right? Did `go get` ever work with that? In any case, pushing
to another repo might be desirable.

Thanks,
-NR

[1] https://gitlab.com/redfield/redctl
Nick Rosbrook Jan. 29, 2020, 2:17 p.m. UTC | #6
> I *think* to guarantee that libxl_<type>_init() has been called when
> unmarshaling, we would need to generate UnmarshalJSON functions to
> implement json.Unmarshaler. E.g.,:
>
> func (dd *DeviceDisk) UnmarshalJSON(data []byte) error {
>         if dd == nil { // Or maybe this is the 'initialized' check you mentioned

Err, I mean `if dd == (DeviceDisk{})`. We want to check if the value
that dd points to is the DeviceDisk zero value.

-NR
George Dunlap Jan. 29, 2020, 2:46 p.m. UTC | #7
On 1/28/20 8:41 PM, Nick Rosbrook wrote:
>> I think marshaling and unmarshalling should be fine, *as long as* the
>> structure being unmarshalled actually went through the
>> libxl_<type>_init() function first.
>>
>> In fact, I was kicking around the idea of adding a non-exported field to
>> all the generated structs -- maybe "bool initalized" -- and having the
>> .fromC() methods set this to 'true', and the .toC() methods return an
>> error if it wasn't set.  But then we'd need to write custom JSON
>> marshallers to handle these.
> 
> I *think* to guarantee that libxl_<type>_init() has been called when
> unmarshaling, we would need to generate UnmarshalJSON functions to
> implement json.Unmarshaler. E.g.,:
> 
> func (dd *DeviceDisk) UnmarshalJSON(data []byte) error {
>         if dd == nil { // Or maybe this is the 'initialized' check you mentioned
>                 dd, err := NewDeviceDisk()
>                 if err != nil {
>                         return err
>                 }
>         }
> 
>         return json.Unmarshal(data, dd)
> }
> 
> AFAICT, this would be required for someone to unmarshal a complete
> domain configuration in one go.

So the above will fix an issue like this:

Scenario A
1. Make a structure from version V by calling NewType()
2. Fill in what you want
3. Marshal it into json
4. Marshal it out of json into a structure from version V+1, with new fields

With code as above, the new elements of structure V+1 will be
initialized by the NewType() in the UnmarshalJSON() method.

But the problem I'm worried about is this:

Scenario B
1. Make an empty, uninitialized structure, without calling NewType()
2. Fill in some fields
3. Marshal it into json
4. Marshal it out of json (with the same version)

In the case above, step 3 encodes all the known fields with *golang*'s
zero values, rather than libxl's default values, and so step 4 will
clobber any defaults written by NewType() with golang zero values again.

Of course, something like scenario A might happen without the marshal /
unmarshal, which is why I thought having a private 'initialized' flag
might be helpful.  But then what you'd want to solve B by having the
unmarshaller read the initialized flag and add it to the json / read it
from the json (since the json package itself can't do it).

(Naturally people can directly modify the json to have the 'initialized'
flag set, but if you get to that level of messing around, you get to
keep all the pieces if it breaks.)

>> As far as further steps -- do you have a clear idea what kind of
>> functionality you'd like to see possible by the time of the feature
>> freeze (probably in May)?  Do you have plans to use these bindings
>> yourself, and if so, how?
>>
>> For my part, I want to start and reap guests.  The latter will require
>> adding event callback functionality which will require more thought (and
>> perhaps expose more libxl issues).  But I don't yet have a clear target
>> beyond that.
> 
> Yes, I plan on using these bindings in redctl (our Redfield toolstack)
> [1], to replace our os/exec calls to xl. To fully make that
> transition, we would need domain start/stop, PCI and network
> attach/detach, as well as some utilities (most of which are either
> implemented, or would be easy to do). But, making that transition is
> relatively low on the priority list right now, so I can't commit to a
> timeline unfortunately.

Sure, nor I; but having a goal always helps, even if it's only best-effort.

Looking at redctl, it seems like actually a pretty full-featured
toolstack -- that seems like a nice complete target to aim at. :-)

>> Re function calls -- do we want to just manually duplicate them as
>> needed, or try to get some sort of IDL support?
> 
> I think it will make more sense to manually duplicate them as needed.
> That way, we can be more particular about function signatures etc. to
> ensure they are idiomatic Go. Also, I am of the opinion that a minimal
> API is a better place to start. Which brings me to another question
> and potential work item:
> 
> Do we want to re-evaluate what is currently implemented in the API? Do
> you have plans to use everything that is currently there? If not, it
> might be nice to trim off things we don't need right now.

I think we should make sure that things actually work.  The very
original golang bindings I wrote to be able to control cpupools, so I
think those functions should stay.  But I'm not sure if anyone's ever
used ConsoleGetTty.  Like `xl.cfg` parsing, it's the sort of thing that
*somebody* will probably want eventually; so I'm inclined to say it
would be less cost to just test it and make sure it works than to remove
it and re-add it when someone decides they need it.

Did you have anything in particular in mind?

I was sort of thinking what we might do is leave `xenlight` as mostly
just a plain wrapper around the libxl C functions, as close to what
might be generated as possible; and then have another package that would
do something more useful.  For instance, having 'Vm' struct, which could
be Start()ed, shut down, and so on; and which would keep track of
if/when the domain died, &c.

>> Other work items include:
>>
>> * modifying configure to detect the availability of go and enable the
>> bindings if it's available
>>
>> * Enabling go build testing in the gitlab CI loop
>>
>> * Making `go get` work, which might involve having code to push
>> post-generated code to a repo and tagging as appropriate
> 
> I was going to ask about this. You had a vanity URL in place at one
> point, right? Did `go get` ever work with that? In any case, pushing
> to another repo might be desirable.
We never actually had a URL in place, no.  I had simply chosen the URL
based on what would be a good combination of easy to type/remember and
easy to set up effectively.  (This was after having an informal chat
with Ian Jackson, who tends to do most of this sort of technical admin
thing.)  We'd probably want to go back and figure out what kind of
"interface" is possible, how to do versioning &c, and then work
backwards from that to get a workflow from the various Xen branches into
that.

BTW do you guys have a solution to the "install new tools then reboot"
issue?  I guess if you have a daemon then it will retain the old version
of the library until after you reboot?

 -George
Nick Rosbrook Feb. 4, 2020, 7:26 p.m. UTC | #8
> But the problem I'm worried about is this:
>
> Scenario B
> 1. Make an empty, uninitialized structure, without calling NewType()
> 2. Fill in some fields
> 3. Marshal it into json
> 4. Marshal it out of json (with the same version)
>
> In the case above, step 3 encodes all the known fields with *golang*'s
> zero values, rather than libxl's default values, and so step 4 will
> clobber any defaults written by NewType() with golang zero values again.

One way to solve this would be add struct tags to generated types that
tell JSON to omit zero values when marshaling. I.e.,
`json:",omitempty"`. Then, only fields that were actually set will be
marshaled. And, for Unmarshal to overwrite a field set by NewType(),
it would have to be *explicitly* set by a user. But, that does mean if
a field is set, and happens to be a Go zero value, it will not be
shown in the JSON. That could be a problem itself.

> >> As far as further steps -- do you have a clear idea what kind of
> >> functionality you'd like to see possible by the time of the feature
> >> freeze (probably in May)?  Do you have plans to use these bindings
> >> yourself, and if so, how?
> >>
> >> For my part, I want to start and reap guests.  The latter will require
> >> adding event callback functionality which will require more thought (and
> >> perhaps expose more libxl issues).  But I don't yet have a clear target
> >> beyond that.
> >
> > Yes, I plan on using these bindings in redctl (our Redfield toolstack)
> > [1], to replace our os/exec calls to xl. To fully make that
> > transition, we would need domain start/stop, PCI and network
> > attach/detach, as well as some utilities (most of which are either
> > implemented, or would be easy to do). But, making that transition is
> > relatively low on the priority list right now, so I can't commit to a
> > timeline unfortunately.
>
> Sure, nor I; but having a goal always helps, even if it's only best-effort.
>
> Looking at redctl, it seems like actually a pretty full-featured
> toolstack -- that seems like a nice complete target to aim at. :-)

Sounds like a plan to me. :)

> I think we should make sure that things actually work.  The very
> original golang bindings I wrote to be able to control cpupools, so I
> think those functions should stay.  But I'm not sure if anyone's ever
> used ConsoleGetTty.  Like `xl.cfg` parsing, it's the sort of thing that
> *somebody* will probably want eventually; so I'm inclined to say it
> would be less cost to just test it and make sure it works than to remove
> it and re-add it when someone decides they need it.
>
> Did you have anything in particular in mind?

Okay, that's good to know. I don't have anything particular in mind, I
just wanted to pose the question. Since I wasn't around when you first
wrote the bindings, I wasn't aware of the motivations.

> I was sort of thinking what we might do is leave `xenlight` as mostly
> just a plain wrapper around the libxl C functions, as close to what
> might be generated as possible; and then have another package that would
> do something more useful.  For instance, having 'Vm' struct, which could
> be Start()ed, shut down, and so on; and which would keep track of
> if/when the domain died, &c.

That's a good point, it might be nice to create an API that is more
"Go-like" and not as tied to libxl semantically speaking.

> > I was going to ask about this. You had a vanity URL in place at one
> > point, right? Did `go get` ever work with that? In any case, pushing
> > to another repo might be desirable.
> We never actually had a URL in place, no.  I had simply chosen the URL
> based on what would be a good combination of easy to type/remember and
> easy to set up effectively.  (This was after having an informal chat
> with Ian Jackson, who tends to do most of this sort of technical admin
> thing.)  We'd probably want to go back and figure out what kind of
> "interface" is possible, how to do versioning &c, and then work
> backwards from that to get a workflow from the various Xen branches into
> that.

What do you mean by "figure out what kind of interface is possible"?
For versioning, I think it would be nice to adopt Go modules and
follow their semantic versioning recommendations. Have you considered
that route, or any others?

> BTW do you guys have a solution to the "install new tools then reboot"
> issue?  I guess if you have a daemon then it will retain the old version
> of the library until after you reboot?

At the moment, we don't have a solution to that problem as we're
simply calling xl via os/exec. But yes, when we integrate xenlight
into redctl the daemon will retain the old version until reboot.

Thanks,
-NR

Patch
diff mbox series

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index c462e4bb42..5a21a2b9b8 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -1113,3 +1113,69 @@  func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
 	path = C.GoString(cpath)
 	return
 }
+
+func NewDomainConfig(t DomainType) (*DomainConfig, error) {
+	var cconfig C.libxl_domain_config
+
+	C.libxl_domain_config_init(&cconfig)
+	C.libxl_domain_build_info_init_type(&cconfig.b_info, C.libxl_domain_type(t))
+
+	gconfig := &DomainConfig{}
+	err := gconfig.fromC(&cconfig)
+	if err != nil {
+		return nil, err
+	}
+
+	return gconfig, nil
+}
+
+func NewDeviceDisk() (*DeviceDisk, error) {
+	var ctype C.libxl_device_disk
+
+	C.libxl_device_disk_init(&ctype)
+
+	gtype := &DeviceDisk{}
+	err := gtype.fromC(&ctype)
+
+	if err != nil {
+		return nil, err
+	}
+
+	return gtype, nil
+}
+
+func NewDeviceNic() (*DeviceNic, error) {
+	var ctype C.libxl_device_nic
+
+	C.libxl_device_nic_init(&ctype)
+
+	gtype := &DeviceNic{}
+	err := gtype.fromC(&ctype)
+
+	if err != nil {
+		return nil, err
+	}
+
+	return gtype, nil
+}
+
+// int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
+//                             uint32_t *domid,
+//                             const libxl_asyncop_how *ao_how,
+//                             const libxl_asyncprogress_how *aop_console_how)
+func (Ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
+	var cdomid C.uint32_t
+	var cconfig C.libxl_domain_config
+	err := config.toC(&cconfig)
+	if err != nil {
+		return Domid(0), fmt.Errorf("converting domain config to C: %v", err)
+	}
+	defer C.libxl_domain_config_dispose(&cconfig)
+
+	ret := C.libxl_domain_create_new(Ctx.ctx, &cconfig, &cdomid, nil, nil)
+	if ret != 0 {
+		return Domid(0), Error(ret)
+	}
+
+	return Domid(cdomid), nil
+}