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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.)
> 
>> +    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.

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel