Re: [Xen-devel] [OPW PATCH] tools/libxl/xl_cmdimpl.c : Calling init function

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Oct 15, 2014 at 9:21 PM, Uma Sharma <uma.sharma523@xxxxxxxxx> wrote:
> This patch calls init function before using libxl_domain_sched_params in 
> sched_domain_get().
>
> Signed-off-by: Uma Sharma <uma.sharma523@xxxxxxxxx>

Hey Uma,

Thanks for the patch!  Good for a first attempt, but as promised, a
number of comments:

The patch itself looks good; but you also need to CC the tools
maintainers.  There are instructions for finding out who they are in
http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches.
(Search for "get_maintainer.pl" .)

I would probably make the summary something like "tools/xl: Call init
function for libxl_domain_sched_params".  "tools/xl" doesn't need to
be an actual path; just enough to give someone the idea the area of
code that's being touched.  "Calling init function" is too generic:
there are lots of init functions already being called.

Re the summary, you need to give an idea *why* you think this change
is necessary.  Which means pointing out, as I did, that in libxl.h
that you are required to call "_init" before using structures defined
in the IDL.

> ---
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c734f79..5b4de28 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5157,6 +5157,7 @@ static int sched_domain_get(libxl_scheduler sched, int 
> domid,
>  {
>      int rc;
>
> +    libxl_domain_sched_params_init(scinfo);

While this will technically work, I think it's probably better if this
is done closer to the place where it's allocated.  That way it's easy
to see the pattern you expect:
 [declare]
 ...
 [init]
 ...
 [dispose]

(Even if you decided to coalesce all the calls into one place,  then
you should have removed the _init() call in sched_rtds_domain_output()
(which is redundant with this patch).)

CC'ing the tools maintainers so they can contradict me if they have a
different opinion...

Thanks!
 -George

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