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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

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.

> +
> +/*
> + * 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").

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

OK, there's more to look at but I think that's enough for now.

 -George


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