Re: [Xen-devel] [PATCH v4 05/14] golang/xenlight: Add tests host related functionality functions

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Thu, Mar 16, 2017 at 7:08 PM, Ronald Rojas <ronladred@xxxxxxxxx> wrote:
> Create tests for the following functions:
> - GetVersionInfo
> - GetPhysinfo
> - GetDominfo
> - GetMaxCpus
> - GetOnlineCpus
> - GetMaxNodes
> - GetFreeMemory
>
> Signed-off-by: Ronald Rojas <ronladred@xxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>
> ---
> changes since last version
>
> - created CFLAGS and LDLIBS variables to build test C
> files with required dependencies.
>
> - created create_context and destroy_context function
> for tests to create/destroy libxl_ctx and xenlogger
>
> - Formating changes
>
> - Removed stale comments
>
> - Removed redundant error checks in Golang tests
>
> - Negated printed error code in freememory.go

Looks a lot better, thanks!  Still some details that need to be corrected.

> diff --git a/tools/golang/xenlight/test/xeninfo/dominfo.go 
> b/tools/golang/xenlight/test/xeninfo/dominfo.go
> new file mode 100644
> index 0000000..bb9257b
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/dominfo.go
> @@ -0,0 +1,31 @@
> +package main
> +
> +import (
> +       "fmt"
> +       "os"
> +       "xenproject.org/xenlight"

This needs to be changed to use the new name:
golang.xenproject.org/xenlight (in all the .go files).

Also, please do at least a build test for each patch before you send it.

> +)
> +
> +func main() {
> +       ctx := xenlight.Ctx
> +       err := ctx.Open()
> +       if err != nil {
> +               os.Exit(-1)
> +       }
> +       defer ctx.Close()
> +       info, err := ctx.DomainInfo(0)

Just noticed -- in this case, if DomainInfo fails, you simply exit
without printing anything (both for golang and the C versions); but in
most of the other cases, you actually print the return value.

Is there a reason not to print the error value here, but to print it
for the others?

> +       if err != nil {
> +               os.Exit(-1)
> +       }
> +
> +       fmt.Printf("%d\n%d\n", info.Domid, info.Ssidref)
> +       //fmt.Printf("%s\n", info.SsidLabel)

Again, why is there a line here that's commented out?  (I asked this
last time and I didn't see a response.)


> diff --git a/tools/golang/xenlight/test/xeninfo/freememory.go 
> b/tools/golang/xenlight/test/xeninfo/freememory.go
> new file mode 100644
> index 0000000..2752bd3
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/freememory.go
> @@ -0,0 +1,25 @@
> +package main
> +
> +import (
> +       "fmt"
> +       "os"
> +       "xenproject.org/xenlight"
> +)
> +
> +func main() {
> +       ctx := xenlight.Ctx
> +       err := ctx.Open()
> +       if err != nil {
> +               os.Exit(-1)
> +       }
> +
> +       defer ctx.Close()
> +
> +       free_memory, err := ctx.GetFreeMemory()
> +       if err != nil {
> +               fmt.Printf("-%d\n", err)

Hmm, so this isn't quite right.  First of all, normally you'd try to
negate the actual value, rather than just adding a '-' in front of it;
but of course you can't do that simply here because err is an
interface of type 'error'; but that shows the other way this code is a
bit fragile, in that it assumes that GetFreeMemory() will return an
error of type xenlight.Error, and not (say) some other kind of error
-- even though we actually return the results of fmt.Errorf()
occasionally (at least for some methods).

I think even for this kind of testing code, it would be better to do a
type assertion, thus:

    if err != nil {
        val, ok := err.(xenlight.Error)
        if ! ok {
            fmt.Printf("Error %v\n", err)
        } else {
            fmt.Printf("%d\n", -val)
        }
    }

Also, you need to do this for all the tests which attempt to print the
return value of the error.

Alternately, you could make all the unit tests behave like dominfo.*,
and simply exit without comment on an error.

> diff --git a/tools/golang/xenlight/test/xeninfo/print.h 
> b/tools/golang/xenlight/test/xeninfo/print.h
> new file mode 100644
> index 0000000..dd6c987
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/print.h
> @@ -0,0 +1,22 @@
> +xentoollog_logger_stdiostream *logger;

So here you define (rather than "declare") a global variable in a
header file.  This is generally frowned upon, and usually indicates
that you need to restructure the code somewhat.

I had a chat with Ian Jackson, and we agreed that it would be better
to create a file, maybe "test-common.c", that would contain this
variable, as well as the three functions below.

Then the header ("test-common.h") would contain *declarations* of the
variable (i.e., "extern xentoollog_logger_stdiostream *logger;") and
function prototypes.

The test-common.c file is small now, but it may grow as additional
functionality is needed.

The other thing you might consider, to further reduce the boilerplate
you have in each unit test file, is to also include a libxl_ctx
pointer in test-common; and have create_context() simply return an int
(0 for success, -1 for failure).

> +
> +static inline char *bool_to_string(bool a){
> +    return (a ? "true" : "false");
> +}
> +
> +static inline libxl_ctx *create_context(void){
> +    libxl_ctx *context;
> +    logger = xtl_createlogger_stdiostream(stderr,
> +            XTL_ERROR, 0);
> +    libxl_ctx_alloc(&context, LIBXL_VERSION, 0 , (xentoollog_logger*)logger);
> +    return context;
> +}
> +
> +static inline int destroy_context(libxl_ctx *context){
> +    int err = libxl_ctx_free(context);
> +    if (err != 0)
> +        return err;
> +    xtl_logger_destroy((xentoollog_logger*)logger);
> +    return err;
> +
> +}

> diff --git a/tools/golang/xenlight/test/xeninfo/xenlight.go 
> b/tools/golang/xenlight/test/xeninfo/xenlight.go
> new file mode 120000
> index 0000000..693da7b
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/xenlight.go
> @@ -0,0 +1 @@
> +../../xenlight.go/usr/local/go/src/xenproject.org/xenlight/xenlight.go

What's this file for?

Thanks,
 -George

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