Skip to contentSkip to conversation

tools/libxl/xl_cmdimpl.c : Calling init function

PATCH In review Uma Sharma 3 participants 3 messages Oct 15, 2014 – Oct 16, 2014

lore
Share & tools
lore

Uma Sharma

author
#0 lore raw

PATCH 1/1 tools/libxl/xl_cmdimpl.c : Calling init function

This patch calls init function before using libxl_domain_sched_params in sched_domain_get().

Signed-off-by Uma Sharma <[email protected]>

---

tools/libxl/xl_cmdimpl.c +1 -0
index c734f79..5b4de28 100644
@@ -5157,6 +5157,7 @@ static int sched_domain_get(libxl_scheduler sched, int domid,
 {
     int rc;
 
+    libxl_domain_sched_params_init(scinfo);
     rc = libxl_domain_sched_params_get(ctx, domid, scinfo);
     if (rc) {
         fprintf(stderr, "libxl_domain_sched_params_get failed.\n");
#1 lore raw
Quoted 3 lines · Uma Sharma

This patch calls init function before using libxl_domain_sched_params in sched_domain_get().

Signed-off-by Uma Sharma <[email protected]>

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.

tools/libxl/xl_cmdimpl.c static int sched_domain_get(libxl_scheduler sched, int domid, · line 5160

Quoted 10 lines

---

tools/libxl/xl_cmdimpl.c +1 -0
index c734f79..5b4de28 100644
@@ -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

#2 lore raw

On Thu, Oct 16, 2014 at 04:13:30PM +0100, George Dunlap wrote:
[...]

Quoted 9 lines
Quoted 9 lines

---

tools/libxl/xl_cmdimpl.c +0 -0
index c734f79..5b4de28 100644
@@ -5157,6 +5157,7 @@ static int sched_domain_get(libxl_scheduler sched, int domid,
 {
     int rc;
Quoted 11 lines

tools/libxl/xl_cmdimpl.c static int sched_domain_get(libxl_scheduler sched, int domid, · line 5160

Quoted 1 line
tools/libxl/xl_cmdimpl.c +1 -0
+    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]

Agreed. _init and _dispose need to stay in the same function as variable declaration.

Quoted 4 lines

(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).)

No, please don't. I asked Meng to construct his function like that, which is consistent with the aforementioned pattern. :-)

Wei.

Quoted 5 lines

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

Thanks!
-George