Re: [Xen-devel] [PATCH RFC 59/59] tools/xenlight: Create interface for xenlight info

[Ronald Rojas <ronladred@xxxxxxxxx>]

On Thu, Dec 29, 2016 at 01:45:54PM +0000, George Dunlap wrote:
> Hey Ronald!  Looks like you're getting going pretty well here.
> 
> At a high level: I think I began by saying that this would probably all
> be a single patch.  But I think it would probably be easier to review
> the different decisions made at each point by filling out the file bit
> by bit, and explaining what's going on at each point.
> 
> I think also it would be helpful to have somewhere -- either in a
> comment or in a text file -- a description of the general approach to
> converting the libxl C style into xenlight golang style.  For instance,
> saying that for functions, we'll take the function name
> (libxl_cpupool_remove), remove the libxl_ prefix (since the packages are
> namespaced already) and convert it to CamelCase by capitalizing the first
> 
> I'll take a stab at breaking it down in an example order that makes some
> sense to me, and then you can see what you think.

I made some guidelines that I think would nicely adjust the naming convention
from C to Go. I'll add it as a file below.
> 
> Futher comments...
> 
> On 29/12/16 01:14, Ronald Rojas wrote:
> > diff --git a/tools/golang/xenlight/xenlight.go 
> > b/tools/golang/xenlight/xenlight.go
> > new file mode 100644
> > index 0000000..b0eb6f8
> > --- /dev/null
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -0,0 +1,1000 @@
> > +/*
> > + * Copyright (C) 2016 George W. Dunlap, Citrix Systems UK Ltd
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; version 2 of the
> > + * License only.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > + * 02110-1301, USA.
> > + */
> > +package xenlight
> > +
> > +/*
> > +#cgo LDFLAGS: -lxenlight -lyajl
> > +#include <stdlib.h>
> > +#include <libxl.h>
> > +*/
> > +import "C"
> 
> I see you switched back to dynamic linking -- any particular reason?
> 
> We probably need to put a "// FIXME" here saying that we need to
> generate these compile-time dependencies using pkg-config.

We discussed this on IRC but I'll just restate here that I was not able 
to compile the program through static linking and it would be fine to 
just use dynamic linking for now.
Since we are doing dynamic linking do we still need to use pkg-config?
> 
> > +
> > +/*
> > + * Other flags that may be needed at some point:
> > + *  -lnl-route-3 -lnl-3
> > +#cgo LDFLAGS: -lxenlight -lyajl_s -lxengnttab -lxenstore -lxenguest 
> > -lxentoollog -lxenevtchn -lxenctrl -lblktapctl -lxenforeignmemory -lxencall 
> > -lz -luuid -lutil
> > + *
> > + * To get back to simple dynamic linking:
> > +*/
> 
> Comment needs updating (to "To use static linking").

Fixed.
> 
> > +
> > +import (
> > +   "fmt"
> > +   "time"
> > +   "unsafe"
> > +)
> > +
> > +/*
> > + * Errors
> > + */
> > +const (
> > +   ErrorNonspecific                  = int(C.ERROR_NONSPECIFIC)
> > +   ErrorVersion                      = int(C.ERROR_VERSION)
> > +   ErrorFail                         = int(C.ERROR_FAIL)
> > +   ErrorNi                           = int(C.ERROR_NI)
> > +   ErrorNomem                        = int(C.ERROR_NOMEM)
> > +   ErrorInval                        = int(C.ERROR_INVAL)
> > +   ErrorBadfail                      = int(C.ERROR_BADFAIL)
> > +   ErrorGuestTimedout                = int(C.ERROR_GUEST_TIMEDOUT)
> > +   ErrorTimedout                     = int(C.ERROR_TIMEDOUT)
> > +   ErrorNoparavirt                   = int(C.ERROR_NOPARAVIRT)
> > +   ErrorNotReady                     = int(C.ERROR_NOT_READY)
> > +   ErrorOseventRegFail               = int(C.ERROR_OSEVENT_REG_FAIL)
> > +   ErrorBufferfull                   = int(C.ERROR_BUFFERFULL)
> > +   ErrorUnknownChild                 = int(C.ERROR_UNKNOWN_CHILD)
> > +   ErrorLockFail                     = int(C.ERROR_LOCK_FAIL)
> > +   ErrorJsonConfigEmpty              = int(C.ERROR_JSON_CONFIG_EMPTY)
> > +   ErrorDeviceExists                 = int(C.ERROR_DEVICE_EXISTS)
> > +   ErrorCheckpointDevopsDoesNotMatch = 
> > int(C.ERROR_CHECKPOINT_DEVOPS_DOES_NOT_MATCH)
> > +   ErrorCheckpointDeviceNotSupported = 
> > int(C.ERROR_CHECKPOINT_DEVICE_NOT_SUPPORTED)
> > +   ErrorVnumaConfigInvalid           = int(C.ERROR_VNUMA_CONFIG_INVALID)
> > +   ErrorDomainNotfound               = int(C.ERROR_DOMAIN_NOTFOUND)
> > +   ErrorAborted                      = int(C.ERROR_ABORTED)
> > +   ErrorNotfound                     = int(C.ERROR_NOTFOUND)
> > +   ErrorDomainDestroyed              = int(C.ERROR_DOMAIN_DESTROYED)
> > +   ErrorFeatureRemoved               = int(C.ERROR_FEATURE_REMOVED)
> > +)
> 
> The problem with this at the moment is that we don't actually return any
> of these errors -- instead we return a newly-generated error which
> contains text.
> 
> This pair of blog posts describes some approaches taken by different
> kinds of libraries to returning errors, and why:
> 
> https://www.goinggo.net/2014/10/error-handling-in-go-part-i.html
> https://www.goinggo.net/2014/11/error-handling-in-go-part-ii.html
> 
> Another approach is taken by the go syscall package:
> 
> https://golang.org/pkg/syscall/#Errno
> 
> (See also https://golang.org/src/syscall/syscall_unix.go, lines 93-100,
> and https://golang.org/src/syscall/zerrors_linux_amd64.go, at lines 1204
> and 1381.)
> 
> The approach taken by libraries in the second blog allows you to pass
> back a lot more information about what happened; but I think at the
> moment, given the limited number of things that libxl returns, that's
> probably overkill.
> 
> Which leaves us either taking the approach of something like bufio, and
> making things like this:
> 
> const (
>    ErrorNonspecific = errors.New("xenlight: Non-specific error")
>    ...
> )
> 
> Or taking the approach of the syscall package, and defining a type:
> 
> type xlerror int
> 
> func (e xlerror) Error() {
> 
> }
> 
> And making a (private) errors[] array, and re-impleminting Error() from
> the syscall package.
> 
> I think I would probably go with the syscall library's approach, since
> (like it) we are handed a set of pre-existing error numbers.
> 
> What do you think?

I was actually thinking that taking the approach similiar to bufio would
be better. It would not be difficult to create individual errors for each
error number and creating variables for each error would allow users to 
easily compare the error that's returned against specific errors. 

I've also been thinking about how to do the testing and I think it would
be good to seperate testing into two parts, testing functions that get
infomation about the system and functions that change the state of the 
system. 

I haven't been able to figure out how to use osstest to use the
Golang libxl but I think it would still be fine to do unit testing by
writing C and Go code that get information from a specific fuction(like
physinfo) and seeing if they produce the same output.

For functions that change the state of the system (like create/destroy
guests) I think your idea of using a chaos monkey would be good. Maybe
the general procedure would be something like this:
1. Get the complete system information through the functions tested
in the first part.
2. Pick a random operation from a set implemented functions. 
3. Make changes to the copy of the systems information that we have
4. Get the system information again through the golang functions and
see if this copy of the system information matches the copy we already
have.
5. Repeat as necessary

It will probably take some time to finalize the procedure so I will
plan to submit it as a seperate patch or with a later patch.
> 
> OK, there's more to look at but I think that's enough for now.
> 
>  -George
> 

thanks,
Ronald

Attachment: rules
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel