Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[George Dunlap]

On 02/03/17 16:07, Ronald Rojas wrote:
> Create tests for the following functions:
> - GetVersionInfo
> - GetPhysinfo
> - GetDominfo
> - GetMaxCpus
> - GetOnlineCpus
> - GetMaxNodes
> - GetFreeMemory
> 
> Signed-off-by: Ronald Rojas 
> Signed-off-by: George Dunlap 
> 
> ---
> 
> - Removed individual Makefiles for each test for a single
> Makefile to test all files at once
> 
> - Changed all tests to xeninfo directory
> 
> - Applied libxl__{init/dispose} for IDL types in tests
> 
> - Applied formating fixes

Looks a lot better!  Still some more things to improve...

> 
> CC: xen-devel@lists.xen.org
> CC: george.dun...@citrix.com
> CC: ian.jack...@eu.citrix.com
> CC: wei.l...@citrix.com
> ---
> ---
>  tools/golang/xenlight/test/xeninfo/Makefile   | 34 
> +++
>  tools/golang/xenlight/test/xeninfo/dominfo.c  | 31 +
>  tools/golang/xenlight/test/xeninfo/dominfo.go | 31 +
>  tools/golang/xenlight/test/xeninfo/freememory.c   | 24 
>  tools/golang/xenlight/test/xeninfo/freememory.go  | 28 +++
>  tools/golang/xenlight/test/xeninfo/maxcpu.c   | 16 +++
>  tools/golang/xenlight/test/xeninfo/maxcpu.go  | 27 ++
>  tools/golang/xenlight/test/xeninfo/maxnodes.c | 16 +++
>  tools/golang/xenlight/test/xeninfo/maxnodes.go| 27 ++
>  tools/golang/xenlight/test/xeninfo/onlinecpu.c| 16 +++
>  tools/golang/xenlight/test/xeninfo/onlinecpu.go   | 27 ++
>  tools/golang/xenlight/test/xeninfo/physinfo.c | 31 +
>  tools/golang/xenlight/test/xeninfo/physinfo.go| 32 +
>  tools/golang/xenlight/test/xeninfo/print.h|  3 ++
>  tools/golang/xenlight/test/xeninfo/versioninfo.c  | 20 +
>  tools/golang/xenlight/test/xeninfo/versioninfo.go | 28 +++
>  16 files changed, 391 insertions(+)
>  create mode 100644 tools/golang/xenlight/test/xeninfo/Makefile
>  create mode 100644 tools/golang/xenlight/test/xeninfo/dominfo.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/dominfo.go
>  create mode 100644 tools/golang/xenlight/test/xeninfo/freememory.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/freememory.go
>  create mode 100644 tools/golang/xenlight/test/xeninfo/maxcpu.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/maxcpu.go
>  create mode 100644 tools/golang/xenlight/test/xeninfo/maxnodes.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/maxnodes.go
>  create mode 100644 tools/golang/xenlight/test/xeninfo/onlinecpu.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/onlinecpu.go
>  create mode 100644 tools/golang/xenlight/test/xeninfo/physinfo.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/physinfo.go
>  create mode 100644 tools/golang/xenlight/test/xeninfo/print.h
>  create mode 100644 tools/golang/xenlight/test/xeninfo/versioninfo.c
>  create mode 100644 tools/golang/xenlight/test/xeninfo/versioninfo.go
> 
> diff --git a/tools/golang/xenlight/test/xeninfo/Makefile 
> b/tools/golang/xenlight/test/xeninfo/Makefile
> new file mode 100644
> index 000..859e4d6
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/Makefile
> @@ -0,0 +1,34 @@
> +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)

So I think here we need to define CFLAGS and LDLIBS; and we want to add
-Werror, thus:

CFLAGS += -Werror
CFLAGS += $(CFLAGS_libxenlight)

LDLIBS += $(LDLIBS_libxenlight)

And then we want to build things like this:

$(CC) $(CFLAGS) -o $@ $< $(LDLIBS)

And when you do that, gcc will tell you a long list of improvements in
your C code. :-)  (If you run into any problems with how to fix these,
just give us a shout on the IRC channel.)

> +
> +%-go: %.go
> + GOPATH=$(XEN_ROOT)/tools/golang $(GO) build -o $@ $<
> +
> +clean:
> + rm -f *-c
> + rm -f *-go
> + rm -f *.output
> diff --git a/tools/golang/xenlight/test/xeninfo/dominfo.c 
> b/tools/golang/xenlight/test/xeninfo/dominfo.c
> new file mode 100644
> index 000..85f658d
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/dominfo.c
> @@ -0,0 +1,31 @@
> +#include 
> +#include 
> +#include 
> +#include "print.h"
> +
> +int main(){
> +
> + 

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[Ian Jackson]

George Dunlap writes ("Re: [PATCH v2 5/5] golang/xenlight: Add tests host 
related functionality functions"):
> Yes, the plan was to have it off by default at very least until we got
> configure to detect the presence of a go compiler.  Ronald has already
> spent a decent chunk of his time doing Makefile work; I didn't want to
> make him spend a big chunk of time poking autoconf as well on a project
> which was advertised as programming Go. :-)  Adding "won't break on IDL
> changes" is another criteria it would be good to add.

Right.  OK.  I'm content then.

> Once the core patches are in-tree, I could start working on some of that
> stuff if I have time.  In addition, I keep finding random improvements
> to make in Ronald's patches which aren't actually bugs; it would be
> nicer for both of us if I could just post patches for those improvements
> rather than commenting on his patches and making him re-submit them.

Sure.

FWIW I have no other comments, and on this basis I'm happy for it to
go in with your review.

Ian.

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[George Dunlap]

On 03/03/17 15:02, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH v2 5/5] golang/xenlight: Add tests host 
> related functionality functions"):
>> Right.  The purpose of hand-crafting the code was to get a feel for what
>> a good Go-like output would look like before investing in the IDL.
> 
> That makes sense.
> 
>>  It sounds like you're suggesting that having IDL support would be a
>> prerequisite to getting anything checked in?
> 
> I think it's a prerequisite for it to be hooked into the build by
> default, unless someone can convince me that foreseeable changes to
> the libxl idl will not cause the golang build to break.
> 
>> I'd definitely say havind IDL support would be a prerequisite for
>> declaring the bindings "supported".  I don't think the structures for
>> these functions change so often that it would be a hardship for Ronald
>> or I to change them whenever they broke; and so I would argue it
>> shouldn't be a blocker for getting things into the tree if the code
>> looks good.
> 
> I don't think it's sensible to ask other contributors who change the
> IDL to have to liase with golang experts in order to not break the
> build.  The whole point of the idl system is precisely to make this
> kind of thing automatic.
> 
> OTOH I don't mind it being in-tree if it's not built by default.  Of
> course then it will probably rot, so that's not a very stable
> situation, but if having it in-tree makes the development easier then
> that's fine by me.

Yes, the plan was to have it off by default at very least until we got
configure to detect the presence of a go compiler.  Ronald has already
spent a decent chunk of his time doing Makefile work; I didn't want to
make him spend a big chunk of time poking autoconf as well on a project
which was advertised as programming Go. :-)  Adding "won't break on IDL
changes" is another criteria it would be good to add.

Once the core patches are in-tree, I could start working on some of that
stuff if I have time.  In addition, I keep finding random improvements
to make in Ronald's patches which aren't actually bugs; it would be
nicer for both of us if I could just post patches for those improvements
rather than commenting on his patches and making him re-submit them.

 -George

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[Ian Jackson]

George Dunlap writes ("Re: [PATCH v2 5/5] golang/xenlight: Add tests host 
related functionality functions"):
> Right.  The purpose of hand-crafting the code was to get a feel for what
> a good Go-like output would look like before investing in the IDL.

That makes sense.

>  It sounds like you're suggesting that having IDL support would be a
> prerequisite to getting anything checked in?

I think it's a prerequisite for it to be hooked into the build by
default, unless someone can convince me that foreseeable changes to
the libxl idl will not cause the golang build to break.

> I'd definitely say havind IDL support would be a prerequisite for
> declaring the bindings "supported".  I don't think the structures for
> these functions change so often that it would be a hardship for Ronald
> or I to change them whenever they broke; and so I would argue it
> shouldn't be a blocker for getting things into the tree if the code
> looks good.

I don't think it's sensible to ask other contributors who change the
IDL to have to liase with golang experts in order to not break the
build.  The whole point of the idl system is precisely to make this
kind of thing automatic.

OTOH I don't mind it being in-tree if it's not built by default.  Of
course then it will probably rot, so that's not a very stable
situation, but if having it in-tree makes the development easier then
that's fine by me.

Ian.

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[Ronald Rojas]

On Thu, Mar 02, 2017 at 05:53:00PM +, George Dunlap wrote:
> On 02/03/17 17:36, Ian Jackson wrote:
> > Ronald Rojas writes ("[PATCH v2 5/5] golang/xenlight: Add tests host 
> > related functionality functions"):
> >> Create tests for the following functions:
> >> - GetVersionInfo
> >> - GetPhysinfo
> >> - GetDominfo
> >> - GetMaxCpus
> >> - GetOnlineCpus
> >> - GetMaxNodes
> >> - GetFreeMemory
> > 
> > I assume this whole series is RFC still ?
> 
> I think the earlier patches looked pretty close to being checked in.  I
> think having a basic chunk of functionality checked in will make it
> easier to actually collaborate on improving things.
> 
> > I don't feel competent to review the golang code.  But I did spot some
> > C to review :-)
> > 
> >> diff --git a/tools/golang/xenlight/test/xeninfo/freememory.c 
> >> b/tools/golang/xenlight/test/xeninfo/freememory.c
> >> new file mode 100644
> >> index 000..04ee90d
> >> --- /dev/null
> >> +++ b/tools/golang/xenlight/test/xeninfo/freememory.c
> >> @@ -0,0 +1,24 @@
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include "print.h"
> >> +
> >> +int main(){
> > 
> > This is an unusual definition of main.  (I think it's still legal, but
> > probably not what you meant.)
> > 

I did this because I'm not expecting any arguments  to be passed into
main. I can change it to the standard definition instead.

> >> +libxl_ctx *context;
> >> +libxl_ctx_alloc(,LIBXL_VERSION, 0, NULL);
> >> +
> >> +uint64_t free_memory;
> >> +int err = libxl_get_free_memory(context, _memory);
> >> +if (err < 0)
> >> +{
> >> +printf("%d\n", err);
> >> +}
> >> +else
> >> +{
> >> +printf("%lu\n", free_memory);
> >> +}
> > 
> > This output is ambiguous.
> > 
> >> +libxl_ctx_free(context);
> >> +
> >> +}
> > 
> > Returns from main without returning a value.  Error code is lost.
> 
> He's not testing whether the call succeeds; he's testing if the call has
> the same result as the golang function.
> 
> But this won't quite work anymore, because as of v2 the golang return
> values are positive constants (to make it easy to use them for indexes).
>  So if there were an identical error, the output would be different even
> if the error number were identical.

You are right. I need to negate the value that I print in the go file.

> 
> That needs to be fixed; but I also agree it would probably be better to
> print something that distinguishes between success and failure.

I think it's always clear whether the function failed or succeeded. The
error code will always be a negative number while free_memory can only
be non-negative because it is an unsigned long. There is no overlap
between those two values.


Ronald

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[George Dunlap]

On 02/03/17 17:55, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH v2 5/5] golang/xenlight: Add tests host 
> related functionality functions"):
>> On 02/03/17 17:36, Ian Jackson wrote:
>>> I assume this whole series is RFC still ?
>>
>> I think the earlier patches looked pretty close to being checked in.  I
>> think having a basic chunk of functionality checked in will make it
>> easier to actually collaborate on improving things.
> 
> There is a lot of hand-crafted code here, whose semantics (eg, lists
> of enum values and fields) which is copied from the libxl idl.
> 
> What this means is that the golang code may stop building, or (worse)
> start to produce broken results, when the idl is updated.

Right.  The purpose of hand-crafting the code was to get a feel for what
a good Go-like output would look like before investing in the IDL.  It
sounds like you're suggesting that having IDL support would be a
prerequisite to getting anything checked in?

I'd definitely say havind IDL support would be a prerequisite for
declaring the bindings "supported".  I don't think the structures for
these functions change so often that it would be a hardship for Ronald
or I to change them whenever they broke; and so I would argue it
shouldn't be a blocker for getting things into the tree if the code
looks good.

But in the end it's your & Wei's call. :-)

 -George

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[Ian Jackson]

George Dunlap writes ("Re: [PATCH v2 5/5] golang/xenlight: Add tests host 
related functionality functions"):
> On 02/03/17 17:36, Ian Jackson wrote:
> > I assume this whole series is RFC still ?
> 
> I think the earlier patches looked pretty close to being checked in.  I
> think having a basic chunk of functionality checked in will make it
> easier to actually collaborate on improving things.

There is a lot of hand-crafted code here, whose semantics (eg, lists
of enum values and fields) which is copied from the libxl idl.

What this means is that the golang code may stop building, or (worse)
start to produce broken results, when the idl is updated.

Ian.

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[George Dunlap]

On 02/03/17 17:36, Ian Jackson wrote:
> Ronald Rojas writes ("[PATCH v2 5/5] golang/xenlight: Add tests host related 
> functionality functions"):
>> Create tests for the following functions:
>> - GetVersionInfo
>> - GetPhysinfo
>> - GetDominfo
>> - GetMaxCpus
>> - GetOnlineCpus
>> - GetMaxNodes
>> - GetFreeMemory
> 
> I assume this whole series is RFC still ?

I think the earlier patches looked pretty close to being checked in.  I
think having a basic chunk of functionality checked in will make it
easier to actually collaborate on improving things.

> I don't feel competent to review the golang code.  But I did spot some
> C to review :-)
> 
>> diff --git a/tools/golang/xenlight/test/xeninfo/freememory.c 
>> b/tools/golang/xenlight/test/xeninfo/freememory.c
>> new file mode 100644
>> index 000..04ee90d
>> --- /dev/null
>> +++ b/tools/golang/xenlight/test/xeninfo/freememory.c
>> @@ -0,0 +1,24 @@
>> +#include 
>> +#include 
>> +#include 
>> +#include "print.h"
>> +
>> +int main(){
> 
> This is an unusual definition of main.  (I think it's still legal, but
> probably not what you meant.)
> 
>> +libxl_ctx *context;
>> +libxl_ctx_alloc(,LIBXL_VERSION, 0, NULL);
>> +
>> +uint64_t free_memory;
>> +int err = libxl_get_free_memory(context, _memory);
>> +if (err < 0)
>> +{
>> +printf("%d\n", err);
>> +}
>> +else
>> +{
>> +printf("%lu\n", free_memory);
>> +}
> 
> This output is ambiguous.
> 
>> +libxl_ctx_free(context);
>> +
>> +}
> 
> Returns from main without returning a value.  Error code is lost.

He's not testing whether the call succeeds; he's testing if the call has
the same result as the golang function.

But this won't quite work anymore, because as of v2 the golang return
values are positive constants (to make it easy to use them for indexes).
 So if there were an identical error, the output would be different even
if the error number were identical.

That needs to be fixed; but I also agree it would probably be better to
print something that distinguishes between success and failure.

 -George

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

Re: [Xen-devel] [PATCH v2 5/5] golang/xenlight: Add tests host related functionality functions

[Ian Jackson]

Ronald Rojas writes ("[PATCH v2 5/5] golang/xenlight: Add tests host related 
functionality functions"):
> Create tests for the following functions:
> - GetVersionInfo
> - GetPhysinfo
> - GetDominfo
> - GetMaxCpus
> - GetOnlineCpus
> - GetMaxNodes
> - GetFreeMemory

I assume this whole series is RFC still ?

I don't feel competent to review the golang code.  But I did spot some
C to review :-)

> diff --git a/tools/golang/xenlight/test/xeninfo/freememory.c 
> b/tools/golang/xenlight/test/xeninfo/freememory.c
> new file mode 100644
> index 000..04ee90d
> --- /dev/null
> +++ b/tools/golang/xenlight/test/xeninfo/freememory.c
> @@ -0,0 +1,24 @@
> +#include 
> +#include 
> +#include 
> +#include "print.h"
> +
> +int main(){

This is an unusual definition of main.  (I think it's still legal, but
probably not what you meant.)

> +libxl_ctx *context;
> +libxl_ctx_alloc(,LIBXL_VERSION, 0, NULL);
> +
> +uint64_t free_memory;
> +int err = libxl_get_free_memory(context, _memory);
> +if (err < 0)
> +{
> +printf("%d\n", err);
> +}
> +else
> +{
> +printf("%lu\n", free_memory);
> +}

This output is ambiguous.

> +libxl_ctx_free(context);
> +
> +}

Returns from main without returning a value.  Error code is lost.

Thanks,
Ian.

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