tools/xl: Call init function for libxl defined datatypes

PATCH In review Uma Sharma 4 participants 8 messages Oct 19, 2014 – Oct 20, 2014

Share & tools

Uma Sharma

author

2014-10-19 12:35 UTC #0 lore raw

PATCH v3 1/1 tools/xl: Call init function for libxl defined datatypes

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 <[email protected]>

Acked-by Wei Liu <[email protected]>

--
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 <[email protected]>

tools/libxl/xl_cmdimpl.c +12 -0

index c734f79..057033a 100644
@@ -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);

Wei Liu

2014-10-19 12:55 UTC #1 lore raw

Quoted 9 lines · 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 <[email protected]>
Acked-by Wei Liu <[email protected]>

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.

Uma Sharma

author

2014-10-19 13:15 UTC #2 lore raw

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

Quoted 24 lines · Wei Liu

Quoted 9 lines · 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 <[email protected]>
Acked-by Wei Liu <[email protected]>
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.

Wei Liu

2014-10-19 13:47 UTC #3 lore raw

Quoted 5 lines · 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?

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.

Quoted 28 lines

Regards,
Uma Sharma
Quoted 24 lines · Wei Liu
Quoted 9 lines · 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 <[email protected]>
Acked-by Wei Liu <[email protected]>
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.

George Dunlap

2014-10-20 11:01 UTC #4 lore raw

In reply to Uma Sharma

Quoted 4 lines · 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?

"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

Uma Sharma

author

2014-10-20 11:29 UTC #5 lore raw

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

Regards,
Uma Sharma

Quoted 25 lines · George Dunlap

Quoted 5 lines · 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?
"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

Ian Campbell

2014-10-20 13:31 UTC #6 lore raw

Quoted 1 line · Uma Sharma

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.

Quoted 31 lines

Regards,
Uma Sharma
Quoted 25 lines · George Dunlap
Quoted 5 lines · 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?
"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

Uma Sharma

author

2014-10-20 13:36 UTC #7 lore raw

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

Regards,
Uma Sharma

Quoted 49 lines · Ian Campbell

Quoted 1 line · Uma Sharma
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.
Quoted 31 lines
Regards,
Uma Sharma
Quoted 25 lines · George Dunlap
Quoted 5 lines · 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?
"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