[PATCH] libxl: correct libxl to obey libxl's programming paramdigm

[PATCH] libxl: correct libxl to obey libxl's programming paramdigm

[Meng Xu]

When sched_domain_get() return rc != 0, scinfo will not be disposed.
This violates the libxl programming paradigm:
272  * The user must always calls "dispose" exactly
273  * once afterwards, to clean up, regardless of whether operations
on
274  * this object succeeded or failed.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
---
 tools/libxl/xl_cmdimpl.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 01bce2f..d1efdd7 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5014,7 +5014,7 @@ static int sched_credit_domain_output(int domid)
     }
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
     if (rc)
-        return rc;
+        goto out;
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d %4d\n",
         domname,
@@ -5022,6 +5022,7 @@ static int sched_credit_domain_output(int domid)
         scinfo.weight,
         scinfo.cap);
     free(domname);
+out:
     libxl_domain_sched_params_dispose(&scinfo);
     return 0;
 }
@@ -5060,13 +5061,14 @@ static int sched_credit2_domain_output(
     }
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT2, domid, &scinfo);
     if (rc)
-        return rc;
+        goto out;
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d\n",
         domname,
         domid,
         scinfo.weight);
     free(domname);
+out:
     libxl_domain_sched_params_dispose(&scinfo);
     return 0;
 }
@@ -5085,7 +5087,7 @@ static int sched_sedf_domain_output(
     }
     rc = sched_domain_get(LIBXL_SCHEDULER_SEDF, domid, &scinfo);
     if (rc)
-        return rc;
+        goto out;
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d %6d %7d %5d %6d\n",
         domname,
@@ -5096,6 +5098,7 @@ static int sched_sedf_domain_output(
         scinfo.extratime,
         scinfo.weight);
     free(domname);
+out:
     libxl_domain_sched_params_dispose(&scinfo);
     return 0;
 }
-- 
1.7.9.5

Re: [PATCH] libxl: correct libxl to obey libxl's programming paramdigm

[Wei Liu]

On Wed, Jul 30, 2014 at 12:18:55PM -0400, Meng Xu wrote:
> When sched_domain_get() return rc != 0, scinfo will not be disposed.
> This violates the libxl programming paradigm:
> 272  * The user must always calls "dispose" exactly
> 273  * once afterwards, to clean up, regardless of whether operations
> on
> 274  * this object succeeded or failed.
> 

I would remove the line number because the header file changes as much
sutffs added in.

> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> ---
>  tools/libxl/xl_cmdimpl.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 01bce2f..d1efdd7 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5014,7 +5014,7 @@ static int sched_credit_domain_output(int domid)
>      }
>      rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
>      if (rc)
> -        return rc;
> +        goto out;
>      domname = libxl_domid_to_name(ctx, domid);
>      printf("%-33s %4d %6d %4d\n",
>          domname,
> @@ -5022,6 +5022,7 @@ static int sched_credit_domain_output(int domid)
>          scinfo.weight,
>          scinfo.cap);
>      free(domname);
> +out:
>      libxl_domain_sched_params_dispose(&scinfo);

And you seem to miss this part of that paradigm:

* IDL-generated libxl types should be used as follows: the user must           
* always call the "init" function before using a type, even if the             
* variable is simply being passed by reference as an out parameter             
* to a libxl function.

So calling dispose in the end while not calling init at the beginning is
still bogus.

Wei.

Re: [PATCH] libxl: correct libxl to obey libxl's programming paramdigm

[Meng Xu]

[-- Attachment #1.1: Type: text/plain, Size: 2360 bytes --]

Hi Wei,

2014-07-31 22:11 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:

> On Wed, Jul 30, 2014 at 12:18:55PM -0400, Meng Xu wrote:
> > When sched_domain_get() return rc != 0, scinfo will not be disposed.
> > This violates the libxl programming paradigm:
> > 272  * The user must always calls "dispose" exactly
> > 273  * once afterwards, to clean up, regardless of whether operations
> > on
> > 274  * this object succeeded or failed.
> >
>
> I would remove the line number because the header file changes as much
> sutffs added in.
>
>
​Agree and will remove it.​



> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> > ---
> >  tools/libxl/xl_cmdimpl.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 01bce2f..d1efdd7 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5014,7 +5014,7 @@ static int sched_credit_domain_output(int domid)
> >      }
> >      rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
> >      if (rc)
> > -        return rc;
> > +        goto out;
> >      domname = libxl_domid_to_name(ctx, domid);
> >      printf("%-33s %4d %6d %4d\n",
> >          domname,
> > @@ -5022,6 +5022,7 @@ static int sched_credit_domain_output(int domid)
> >          scinfo.weight,
> >          scinfo.cap);
> >      free(domname);
> > +out:
> >      libxl_domain_sched_params_dispose(&scinfo);
>
> And you seem to miss this part of that paradigm:
>
> * IDL-generated libxl types should be used as follows: the user must
> * always call the "init" function before using a type, even if the
> * variable is simply being passed by reference as an out parameter
> * to a libxl function.
>
> So calling dispose in the end while not calling init at the beginning is
> still bogus.
>

​I didn't call the init function at the beginning because the init function
is called in the function ​libxl_domain_sched_params_get() at file
tools/libxl/libxl.c.

My question is: should I call the init function again in xl_cmdimpl.c?
(Although calling it twice seems causing no damage, I'm not sure if it's a
good idea.)

Thanks,

Meng


-- 


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

[-- Attachment #1.2: Type: text/html, Size: 3834 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxl: correct libxl to obey libxl's programming paramdigm

[Wei Liu]

On Sat, Aug 02, 2014 at 12:37:21AM +0800, Meng Xu wrote:
[...]
> > * to a libxl function.
> >
> > So calling dispose in the end while not calling init at the beginning is
> > still bogus.
> >
> 
> ​I didn't call the init function at the beginning because the init function
> is called in the function ​libxl_domain_sched_params_get() at file
> tools/libxl/libxl.c.
> 
> My question is: should I call the init function again in xl_cmdimpl.c?
> (Although calling it twice seems causing no damage, I'm not sure if it's a
> good idea.)
> 

I think _init and _dispose should be placed in same place. I would vote
for moving that _init from libxl to xl. You might want to get some input
from maintainers before submitting another version.

Wei.

> Thanks,
> 
> Meng
> 
> 
> -- 
> 
> 
> -----------
> Meng Xu
> PhD Student in Computer and Information Science
> University of Pennsylvania

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxl: correct libxl to obey libxl's programming paramdigm

[Ian Campbell]

On Fri, 2014-08-01 at 18:02 +0100, Wei Liu wrote:
> On Sat, Aug 02, 2014 at 12:37:21AM +0800, Meng Xu wrote:
> [...]
> > > * to a libxl function.
> > >
> > > So calling dispose in the end while not calling init at the beginning is
> > > still bogus.
> > >
> > 
> > ​I didn't call the init function at the beginning because the init function
> > is called in the function ​libxl_domain_sched_params_get() at file
> > tools/libxl/libxl.c.
> > 
> > My question is: should I call the init function again in xl_cmdimpl.c?
> > (Although calling it twice seems causing no damage, I'm not sure if it's a
> > good idea.)
> > 
> 
> I think _init and _dispose should be placed in same place. I would vote
> for moving that _init from libxl to xl. You might want to get some input
> from maintainers before submitting another version.

Since _init is idempotent for structs which are out parameters it is
fine to call _init from both from the application and libxl.

The application *should* be calling _init, but it is a good idea if
libxl does it too, just in case etc etc.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel