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

[Ronald Rojas <ronladred@gmail.com>]

On Thu, Mar 02, 2017 at 05:53:00PM +0000, 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 0000000..04ee90d
> >> --- /dev/null
> >> +++ b/tools/golang/xenlight/test/xeninfo/freememory.c
> >> @@ -0,0 +1,24 @@
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <libxl.h>
> >> +#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(&context,LIBXL_VERSION, 0, NULL);
> >> +
> >> +    uint64_t free_memory;
> >> +    int err = libxl_get_free_memory(context, &free_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