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 <ronlad...@gmail.com>
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
XEN_ROOT = $(CURDIR)/../../../../.. include $(XEN_ROOT)/tools/Rules.mk GO ?= go TESTS = dominfo freememory maxcpu onlinecpu physinfo versioninfo CBINARIES = $(TESTS:%=%-c) GOBINARIES = $(TESTS:%=%-go) all: build test: clean build for test in $(TESTS) ; do \ ./$$test-c >> c.output ; \ ./$$test-go >> go.output ; \ if cmp -s "c.output" "go.output"; then\ echo "$$test PASSED";\ else \ echo "$$test FAILED";\ fi ; \ done build: $(CBINARIES) $(GOBINARIES) %-c: %.c gcc $(CFLAGS_libxenlight) -o $@ $< $(LDLIBS_libxenlight) %-go: %.go GOPATH=$(XEN_ROOT)/tools/golang $(GO) build -o $@ $< clean: rm -f *-c rm -f *-go rm -f *.output
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel