Re: [PATCH] add scheduler API(take 3?)

["Richard W.M. Jones" <rjones@xxxxxxxxxx>]

Atsushi SAKAI wrote:
> This patch is based on today's libvirt.
> Changes are two points.
> 1)Sunou's memory leak fixes (I removed free(ipt) from this patch) 
> 2)Valgrind detected bug fixes 
>   in params[i].type & params[i].fields src/virsh.c)
>
> line 467 of this patch.
> +    for (i = 0; i < nparams; i++){
> +        params[i].type = VIR_DOMAIN_SCHED_FIELD_INIT;
> +        strncpy(params[i].field,"              ",15);
> +    }

Just some very minor points:

 /**
+ * virDomainSchedParameterType:
+ *
+ * A scheduler parameter field type
+ */
+typedef enum {
+    VIR_DOMAIN_SCHED_FIELD_INT     = 0,	/* integer case */
+    VIR_DOMAIN_SCHED_FIELD_UINT    = 1,	/* unsigned integer case */
+    VIR_DOMAIN_SCHED_FIELD_LLONG   = 2,	/* long long case */
+    VIR_DOMAIN_SCHED_FIELD_ULLONG  = 3,	/* unsigned long long case */
+    VIR_DOMAIN_SCHED_FIELD_DOUBLE  = 4,	/* double case */
+    VIR_DOMAIN_SCHED_FIELD_BOOLEAN = 5, /* boolean(character) case */
+    VIR_DOMAIN_SCHED_FIELD_INIT    = 6	/* for valgrind check */
+} virSchedParameterType;

I would renumber these from 1 and omit the _INIT field altogether, then ...

+    int type;	/* Format type should use enum from virSchedParameterType */

... here turn this into 'virSchedParameterType type' (that's what it is, 
after all), and ...

+    for (i = 0; i < nparams; i++){
+        params[i].type = VIR_DOMAIN_SCHED_FIELD_INIT;
+        strncpy(params[i].field,"              ",15);
+    }

... here change this to:

  for (i = 0; i < nparams; ++i) {
    params[i].type = 0;
    memset (params[i].field, 0, sizeof params[i].field);
  }

I'm going to apply this patch to my private copy & try and break it 
further, so stay tuned ...

Rich.

--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
England and Wales under Company Registration No. 03798903

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature