[OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Uma Sharma]

This patch calls init function for libxl_domain_sched_params before
passing it as reference to sched_domain_get() function in
tools/libxl/xl_cmdimpl.c
IDL generated libxl types should be used only after calling the init
function even if the variable is simply being passed by reference as
an output parameter to a libxl function

Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
--
Changed since v1:
-Calling _init before using libxl_dominfo type and _dispose after
using in function main_list
-Calling _init for type libxl_bitmap in vcpuset() for cpumap
-Calling _init for type libxl_bitmap in main_cpupoolnumasplit() for cpumap 
Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
--
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c734f79..057033a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4330,6 +4330,8 @@ int main_list(int argc, char **argv)
     libxl_dominfo *info, *info_free=0;
     int nb_domain, rc;
 
+    libxl_dominfo_init(&info_buf);
+    
     SWITCH_FOREACH_OPT(opt, "lvhZn", opts, "list", 0) {
     case 'l':
         details = 1;
@@ -4381,6 +4383,7 @@ int main_list(int argc, char **argv)
     else
         libxl_dominfo_dispose(info);
 
+    libxl_dominfo_dispose(&info_buf);
     return 0;
 }
 
@@ -4844,6 +4847,8 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     unsigned int max_vcpus, i;
     libxl_bitmap cpumap;
 
+    libxl_bitmap_init(&cpumap);
+    
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
     if (nr_vcpus == endptr) {
         fprintf(stderr, "Error: Invalid argument.\n");
@@ -5215,6 +5220,8 @@ static int sched_credit_domain_output(int domid)
         printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight", "Cap");
         return 0;
     }
+    
+    libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
     if (rc)
         return rc;
@@ -5261,6 +5268,8 @@ static int sched_credit2_domain_output(
         printf("%-33s %4s %6s\n", "Name", "ID", "Weight");
         return 0;
     }
+    
+    libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT2, domid, &scinfo);
     if (rc)
         return rc;
@@ -5286,6 +5295,8 @@ static int sched_sedf_domain_output(
                "Slice", "Latency", "Extra", "Weight");
         return 0;
     }
+    
+    libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_SEDF, domid, &scinfo);
     if (rc)
         return rc;
@@ -7253,6 +7264,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
         /* No options */
     }
 
+    libxl_bitmap_init(&cpumap);    
     ret = 0;
 
     poolinfo = libxl_list_cpupool(ctx, &n_pools);

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Wei Liu]

On Sun, Oct 19, 2014 at 06:05:01PM +0530, Uma Sharma wrote:
> This patch calls init function for libxl_domain_sched_params before
> passing it as reference to sched_domain_get() function in
> tools/libxl/xl_cmdimpl.c
> IDL generated libxl types should be used only after calling the init
> function even if the variable is simply being passed by reference as
> an output parameter to a libxl function
> 
> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

It's confusing that you sent this v3 patch with my ack on it while at
the same time changed a bunch of other places.

If you have other places to fix, you can send separate patch(es) to fix
them. If you want to squash these changes into one patch, then please
drop my ack (not that I'm against these changes, just that you've
substantially changed the scope of this patch we agreed upon) and write
proper change log.

Hope the above clarification help you understand the process better, and
please don't hesitate to ask any question.

Wei.

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Uma Sharma]

Actually I looked at the patch sending documentation it stated that if
some changes are already acknowledged then we have to write it.
Should I write a new patch with all these changes ? Or I can make the
changes in different patches?

Regards,
Uma Sharma

On Sun, Oct 19, 2014 at 6:25 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Sun, Oct 19, 2014 at 06:05:01PM +0530, Uma Sharma wrote:
>> This patch calls init function for libxl_domain_sched_params before
>> passing it as reference to sched_domain_get() function in
>> tools/libxl/xl_cmdimpl.c
>> IDL generated libxl types should be used only after calling the init
>> function even if the variable is simply being passed by reference as
>> an output parameter to a libxl function
>>
>> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> It's confusing that you sent this v3 patch with my ack on it while at
> the same time changed a bunch of other places.
>
> If you have other places to fix, you can send separate patch(es) to fix
> them. If you want to squash these changes into one patch, then please
> drop my ack (not that I'm against these changes, just that you've
> substantially changed the scope of this patch we agreed upon) and write
> proper change log.
>
> Hope the above clarification help you understand the process better, and
> please don't hesitate to ask any question.
>
> Wei.

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Wei Liu]

On Sun, Oct 19, 2014 at 06:45:11PM +0530, Uma Sharma wrote:
> Actually I looked at the patch sending documentation it stated that if
> some changes are already acknowledged then we have to write it.
> Should I write a new patch with all these changes ? Or I can make the
> changes in different patches?
> 

QUOTE
If someone replies to your patch with a tag of the form "Acked-by:
<Developer>", "Reviewed-by:", "Tested-by:" etc then, assuming you have
not completely reworked the patch, you should include these tags in any
reposting after your own Signed-off-by line.  This lets people know that
the patch has been seen and that someone approves of it and also serves
to prevent reviewers wasting time re-reviewing a patch which is not
significantly different to last time.
/QUOTE

An important point of carrying tags is to save reviewers' time, not just
for the sake of carrying.

Your new patch is significantly different to last time, because you've
expanded the scope of your work, which would mean you changes need
reviewing.

A better approach would be to send new patch(es) for your new changes.

Wei.

> Regards,
> Uma Sharma
> 
> On Sun, Oct 19, 2014 at 6:25 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Sun, Oct 19, 2014 at 06:05:01PM +0530, Uma Sharma wrote:
> >> This patch calls init function for libxl_domain_sched_params before
> >> passing it as reference to sched_domain_get() function in
> >> tools/libxl/xl_cmdimpl.c
> >> IDL generated libxl types should be used only after calling the init
> >> function even if the variable is simply being passed by reference as
> >> an output parameter to a libxl function
> >>
> >> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
> >> Acked-by: Wei Liu <wei.liu2@citrix.com>
> >
> > It's confusing that you sent this v3 patch with my ack on it while at
> > the same time changed a bunch of other places.
> >
> > If you have other places to fix, you can send separate patch(es) to fix
> > them. If you want to squash these changes into one patch, then please
> > drop my ack (not that I'm against these changes, just that you've
> > substantially changed the scope of this patch we agreed upon) and write
> > proper change log.
> >
> > Hope the above clarification help you understand the process better, and
> > please don't hesitate to ask any question.
> >
> > Wei.

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[George Dunlap]

On 10/19/2014 02:15 PM, Uma Sharma wrote:
> Actually I looked at the patch sending documentation it stated that if
> some changes are already acknowledged then we have to write it.
> Should I write a new patch with all these changes ? Or I can make the
> changes in different patches?

"Acked-by: Wei Liu <...>" means, "Wei Liu has looked at everything in 
this patch and doesn't have any objections to it being committed."  That 
way, Ian J can just take a quick look and check it in, trusing Wei's 
judgement.

But in this case, Wei hasn't looked at the whole patch, but just half of 
it.  So Ian J might end up checking in code that hasn't been reviewed.

Usually, if you change the patch at all (apart from trivial things like 
whitespace or fixing clear violation of coding conventions) you have to 
drop the ack.

Since the code you're adding isn't necessarily connected to the code 
that was already acked, just making a separate patch would have been the 
best idea in this case.

  -George

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Uma Sharma]

Thank you for the help.
I created a new patch for the new changes that I did afterwards.

Regards,
Uma Sharma

On Mon, Oct 20, 2014 at 4:31 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 10/19/2014 02:15 PM, Uma Sharma wrote:
>>
>> Actually I looked at the patch sending documentation it stated that if
>> some changes are already acknowledged then we have to write it.
>> Should I write a new patch with all these changes ? Or I can make the
>> changes in different patches?
>
>
> "Acked-by: Wei Liu <...>" means, "Wei Liu has looked at everything in this
> patch and doesn't have any objections to it being committed."  That way, Ian
> J can just take a quick look and check it in, trusing Wei's judgement.
>
> But in this case, Wei hasn't looked at the whole patch, but just half of it.
> So Ian J might end up checking in code that hasn't been reviewed.
>
> Usually, if you change the patch at all (apart from trivial things like
> whitespace or fixing clear violation of coding conventions) you have to drop
> the ack.
>
> Since the code you're adding isn't necessarily connected to the code that
> was already acked, just making a separate patch would have been the best
> idea in this case.
>
>  -George
>

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Ian Campbell]

On Mon, 2014-10-20 at 16:59 +0530, Uma Sharma wrote:
> I created a new patch for the new changes that I did afterwards.

I'm not sure which of the patches I have in my queue I should be
considering applying. Please can you tell me the message-ids of the set
of patches which are currently intended for application.

Or if you prefer you could resend the whole lot as a small series, e.g.
using git send-email as described in
http://wiki.xen.org/wiki/Submitting_Xen_Patches so they are all threaded
together in an easy to deal with way.

Thanks,

Ian.

> 
> Regards,
> Uma Sharma
> 
> On Mon, Oct 20, 2014 at 4:31 PM, George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
> > On 10/19/2014 02:15 PM, Uma Sharma wrote:
> >>
> >> Actually I looked at the patch sending documentation it stated that if
> >> some changes are already acknowledged then we have to write it.
> >> Should I write a new patch with all these changes ? Or I can make the
> >> changes in different patches?
> >
> >
> > "Acked-by: Wei Liu <...>" means, "Wei Liu has looked at everything in this
> > patch and doesn't have any objections to it being committed."  That way, Ian
> > J can just take a quick look and check it in, trusing Wei's judgement.
> >
> > But in this case, Wei hasn't looked at the whole patch, but just half of it.
> > So Ian J might end up checking in code that hasn't been reviewed.
> >
> > Usually, if you change the patch at all (apart from trivial things like
> > whitespace or fixing clear violation of coding conventions) you have to drop
> > the ack.
> >
> > Since the code you're adding isn't necessarily connected to the code that
> > was already acked, just making a separate patch would have been the best
> > idea in this case.
> >
> >  -George
> >

Re: [OPW PATCH V3] tools/xl: Call init function for libxl defined datatypes

[Uma Sharma]

Ok , I will resend the whole lot as a series.

Regards,
Uma Sharma

On Mon, Oct 20, 2014 at 7:01 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-10-20 at 16:59 +0530, Uma Sharma wrote:
>> I created a new patch for the new changes that I did afterwards.
>
> I'm not sure which of the patches I have in my queue I should be
> considering applying. Please can you tell me the message-ids of the set
> of patches which are currently intended for application.
>
> Or if you prefer you could resend the whole lot as a small series, e.g.
> using git send-email as described in
> http://wiki.xen.org/wiki/Submitting_Xen_Patches so they are all threaded
> together in an easy to deal with way.
>
> Thanks,
>
> Ian.
>
>>
>> Regards,
>> Uma Sharma
>>
>> On Mon, Oct 20, 2014 at 4:31 PM, George Dunlap
>> <george.dunlap@eu.citrix.com> wrote:
>> > On 10/19/2014 02:15 PM, Uma Sharma wrote:
>> >>
>> >> Actually I looked at the patch sending documentation it stated that if
>> >> some changes are already acknowledged then we have to write it.
>> >> Should I write a new patch with all these changes ? Or I can make the
>> >> changes in different patches?
>> >
>> >
>> > "Acked-by: Wei Liu <...>" means, "Wei Liu has looked at everything in this
>> > patch and doesn't have any objections to it being committed."  That way, Ian
>> > J can just take a quick look and check it in, trusing Wei's judgement.
>> >
>> > But in this case, Wei hasn't looked at the whole patch, but just half of it.
>> > So Ian J might end up checking in code that hasn't been reviewed.
>> >
>> > Usually, if you change the patch at all (apart from trivial things like
>> > whitespace or fixing clear violation of coding conventions) you have to drop
>> > the ack.
>> >
>> > Since the code you're adding isn't necessarily connected to the code that
>> > was already acked, just making a separate patch would have been the best
>> > idea in this case.
>> >
>> >  -George
>> >
>
>