Re: [Xen-devel] [PATCH RFC 5/8] golang/xenlight: Add tests host related functinality functions

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 23/01/17 16:43, Ronald Rojas wrote:
> Create tests for the following functions:
> - GetVersionInfo
> - GetPhysinfo
> - GetDominfo
> - GetMaxCpus
> - GetOnlineCpus
> - GetMaxNodes
> - GetFreeMemory
> 
> Signed-off-by: Ronald Rojas <ronladred@xxxxxxxxx>

Hey Ronald,

Sorry this has taken a long time to get to.  I think building the test
cases has been really good for testing out the package *as a package*;
and I think the general approach is a really good one.

One big comment first: Rather than having a separate makefile for each
of these, what about putting all the tests of a similar type -- like
these, which are primarily comparing the output of the functions -- in
the same directory, and using a makefile like the attached?  (You may
have to modify the XEN_ROOT variable at the top to reflect where you end
up putting it.)

Then, as promised, a whole slew of nitpicks.

Quick heads-up: It's easy the first time you have your code nitpicked
like this to hear an angry or annoyed "tone", because the comments are
really short.  That's not what's intended; it's just to save time.

> diff --git a/tools/golang/xenlight/test/dominfo/dominfo.c 
> b/tools/golang/xenlight/test/dominfo/dominfo.c
> new file mode 100644
> index 0000000..ebe18c3
> --- /dev/null
> +++ b/tools/golang/xenlight/test/dominfo/dominfo.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <libxl.h>
> +
> +char * bool_to_string(bool b);

Need a newline here.  And I suspect you'll be using this function again
-- maybe you could put it in a .h file as a static inline?

> +int main(){
> +
> +    libxl_ctx *context;
> +    libxl_ctx_alloc(&context,LIBXL_VERSION, 0, NULL);
> +    libxl_dominfo info;
> +    int err = libxl_domain_info(context, &info, 0);

In the Xen tools style you normally declare all the variables together
at the top of the function, and then put a newline between them.  It's
probably a good idea to read tools/libxl/CODING_STYLE, particularly the
"Formating and naming" section.

Also don't forget to call libxl_dominfo_init(&info) (which will set some
defaults).

> +    if(err != 0){
> +        return err;
> +    }

The tools style says there should be spaces around the parentheses, and
that single-line blocks don't need braces; i.e.:

if (err != 0)
    return err;

> +     printf("%d\n%d\n", info.domid, info.ssidref);
> +     printf("%s\n%s\n%s\n%s\n%s\n%s\n", bool_to_string(info.running), 
> bool_to_string(info.blocked ), bool_to_string(info.paused 
> ),bool_to_string(info.shutdown ),bool_to_string(info.dying), 
> bool_to_string(info.never_stop));
> +     long cpu_time = info.cpu_time/((long)1<<35);

Why are we clipping this value?

> +     printf("%d\n%lu\n%lu\n%lu\n%lu\n%lu\n%lu\n%d\n%d\n%d\n", 
> info.shutdown_reason, info.outstanding_memkb, info.current_memkb, 
> info.shared_memkb, info.paged_memkb, info.max_memkb, cpu_time, 
> info.vcpu_max_id, info.vcpu_online, info.cpupool);
> +     printf("%d\n", info.domain_type);

These lines need to be wrapped to 80 columns or less.

> +
> +
> +    libxl_ctx_free(context);
> +
> +}
> +char * bool_to_string(bool b){
> +    return b ? "true": "false";
> +}
> diff --git a/tools/golang/xenlight/test/dominfo/dominfo.go 
> b/tools/golang/xenlight/test/dominfo/dominfo.go
> new file mode 100644
> index 0000000..e6c6a64
> --- /dev/null
> +++ b/tools/golang/xenlight/test/dominfo/dominfo.go
> @@ -0,0 +1,48 @@
> +
> +package main
> +
> +/*
> +#cgo LDFLAGS: -lxenlight -lyajl
> +#include <stdlib.h>
> +#include <libxl.h>
> +*/
> +import "C"

This stanza isn't necessary -- all that should be hidden behind the
package.  (Same for all the other .go files.)

> +import (
> +     "fmt"
> +     "xenproject.org/xenlight"
> +     "os"
> +)
> +
> +func main() {
> +     ctx := xenlight.Ctx

I think this should probably be "var ctx Xenlight.Context".  Copying a
struct which has a pointer in it is usually a bad idea; if you're going
to use your own copy, you should just use your own copy. :-)  (Same for
all the other files.)

I also think we want a newline here.

> +     err := ctx.Open()
> +     if err != nil{

Run 'go fmt' to put spaces where they need to be.  (Same for all the
other files.)

> +             os.Exit(-1)
> +     }
> +     defer ctx.Close()

Newline.

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

Why are we not printing this (both here and in the .c file)?

> +     fmt.Printf("%t\n%t\n%t\n%t\n%t\n%t\n", info.Running, info.Blocked, 
> info.Paused, info.Shutdown, info.Dying, info.NeverStop)

These lines should be wrapped to 80 characters.

> +     cpuTime := info.CpuTime/(1<<35)
> +     fmt.Printf("%d\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n", 
> info.ShutdownReason, info.OutstandingMemkb, info.CurrentMemkb, 
> info.SharedMemkb, info.PagedMemkb, info.MaxMemkb, cpuTime, info.VcpuMaxId, 
> info.VcpuOnline, info.Cpupool)
> +     fmt.Printf("%d\n", info.DomainType)
> +
> +}
> +
> +func replaceDomainType(n int32)(ret string){
> +     switch n{
> +     case -1:
> +             ret = "invalid"
> +     case 1:
> +             ret = "hvm"
> +     case 2:
> +             ret = "pv"
> +     }
> +     return

You're not using this function; it should be removed.

But if you implement the xenlight.DomainType enum in the previous patch,
and the String() method which calls libxl_domain_type_to_string(), you
can use "%v" to print the value of DomainType here.  (Of course you'll
need to call libxl_domain_type_to_string() in the .c file too.)

> diff --git a/tools/golang/xenlight/test/freememory/freememory.c 
> b/tools/golang/xenlight/test/freememory/freememory.c
> new file mode 100644
> index 0000000..9ff8444
> --- /dev/null
> +++ b/tools/golang/xenlight/test/freememory/freememory.c
> @@ -0,0 +1,25 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <libxl.h>
> +
> +int main(){
> +
> +    libxl_ctx *context;
> +    libxl_ctx_alloc(&context,LIBXL_VERSION, 0, NULL);
> +    libxl_physinfo info ;
> +    int err= libxl_get_physinfo(context,&info);
> +    if(err != 0){
> +        return err;
> +    }

I don't think you actually want to call physinfo here. :-)  (Same for
the other files.)

> +
> +   uint64_t free_memory;
> +    err = libxl_get_free_memory(context, &free_memory);
> +    if(err < 0){
> +        printf("%d\n", err);
> +    }else{
> +        printf("%lu\n", free_memory);
> +    }

Indents, spaces, and unnecessary parentheses.

> +    libxl_ctx_free(context);
> +
> +}
> +
> diff --git a/tools/golang/xenlight/test/freememory/freememory.go 
> b/tools/golang/xenlight/test/freememory/freememory.go
> new file mode 100644
> index 0000000..19501e7
> --- /dev/null
> +++ b/tools/golang/xenlight/test/freememory/freememory.go
> @@ -0,0 +1,35 @@
> +
> +package main
> +
> +/*
> +#cgo LDFLAGS: -lxenlight -lyajl
> +#include <stdlib.h>
> +#include <libxl.h>
> +*/
> +import "C"
> +import (
> +     "fmt"
> +     "xenproject.org/xenlight"
> +     "os"
> +)
> +
> +func main() {
> +     ctx := xenlight.Ctx
> +     err := ctx.Open()
> +     if err != nil{
> +             os.Exit(-1)
> +     }
> +     defer ctx.Close()
> +     if err != nil{
> +             os.Exit(-1)
> +     }

I don't think we need to check the error twice. :-)  (Same for the other
files.)

> +
> +
> +     free_memory, err := ctx.GetFreeMemory()
> +     if err != nil{
> +             fmt.Printf("%d\n", err)
> +     }else{
> +             fmt.Printf("%d\n", free_memory)
> +     }
> +
> +}

[snip]

> diff --git a/tools/golang/xenlight/test/physinfo/physinfo.c 
> b/tools/golang/xenlight/test/physinfo/physinfo.c
> new file mode 100644
> index 0000000..92c8f47
> --- /dev/null
> +++ b/tools/golang/xenlight/test/physinfo/physinfo.c
> @@ -0,0 +1,31 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <libxl.h>
> +
> +char *bool_to_string(bool b);
> +int main(){
> +
> +    libxl_ctx *context;
> +    libxl_ctx_alloc(&context,LIBXL_VERSION, 0, NULL);
> +    libxl_physinfo info ;
> +    int err= libxl_get_physinfo(context,&info);
> +    if(err != 0){
> +        return err;
> +    }
> +
> +    printf("%d\n%d\n%d\n%d\n%d\n", info.threads_per_core, 
> info.cores_per_socket, info.max_cpu_id, info.nr_cpus, info.cpu_khz);
> +    printf("%lu\n%lu\n%lu\n%lu\n%lu\n%lu\n", info.total_pages, 
> info.free_pages, info.scrub_pages, info.outstanding_pages, 
> info.sharing_freed_pages, info.sharing_used_frames);
> +    printf("%u\n",info.nr_nodes);
> +    printf("%s\n%s\n", bool_to_string(info.cap_hvm), 
> bool_to_string(info.cap_hvm_directio));
> +
> +    for(int i = 0; i < 8; i++){

So with the Xen-standard CFLAGS (which is what you'll get if you use the
makefile attached), this fails.  You should declare the int up above.

[snip]

> diff --git a/tools/golang/xenlight/test/versioninfo/versioninfo.c 
> b/tools/golang/xenlight/test/versioninfo/versioninfo.c
> new file mode 100644
> index 0000000..f40495d
> --- /dev/null
> +++ b/tools/golang/xenlight/test/versioninfo/versioninfo.c
> @@ -0,0 +1,18 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <libxl.h>
> +
> +main(){
> +
> +    libxl_ctx *context;
> +    libxl_ctx_alloc(&context,LIBXL_VERSION, 0, NULL);
> +    libxl_version_info* info = libxl_get_version_info(context);

With the Xen-supplied CFLAGS this fails because the pointer isn't
declared const.

Like I said, lots of little style points, but overall looks very good,
thanks!

 -George

Attachment: Makefile.xenlight-tests
Description: Text document

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