Re: [libvirt] [RFC PATCH 2/2] Routines to parse numa ... /numa

2011-10-14 Thread Eric Blake 

On 10/13/2011 10:10 PM, Bharata B Rao wrote:

Using VIR_ALLOC_N + snprintf here is not desirable, when you
already have a nice  virBufferPtr object in the call that you
could use.  Just pass the virBufferPtr straight into this
method.


Wanted to user virBufferPtr, but I realized that I need to remove
the last comma from the string and coudn't find an easy way to
do that. Hence resorted to this method. But I think I can still
achive this by not appending a comma to the next part of the agrument
(,mems). Let me see if I can do this cleanly in the next post.


Yeah, it might make sense to first add a helper function to 
util/buf.[ch] that allows one to truncate previously appended bytes, 
since sometimes it is easier logic to say always emit a trailing comma 
then trim than it is to say emit a leading comma if I'm not first. 
Nothing wrong with adding helper functions to make life easier, if 
virBuffer doesn't already meet your needs.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 2/2] Routines to parse numa ... /numa

2011-10-13 Thread Daniel P. Berrange 
On Mon, Oct 03, 2011 at 03:31:35PM +0530, Bharata B Rao wrote:
 Routines to parse numa ... /numa
 
 From: Bharata B Rao bhar...@linux.vnet.ibm.com
 
 This patch adds routines to parse guest numa
 XML configuration for qemu.
 
 Signed-off-by: Bharata B Rao bhar...@linux.vnet.ibm.com
 ---
 
  src/conf/cpu_conf.c |   48 
  src/conf/cpu_conf.h |   11 ++
  src/qemu/qemu_command.c |   94 
 +++
  3 files changed, 153 insertions(+), 0 deletions(-)
 
 
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index a13ba71..5b4345e 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -3153,6 +3153,97 @@ qemuBuildSmpArgStr(const virDomainDefPtr def,
  return virBufferContentAndReset(buf);
  }
  
 +static char *
 +virParseNodeCPUs(char *cpumask)

This is a rather misleading name, since it is really formatting
the argument value. Can you rename this to qemuBuildNumaCPUArgStr

 +{
 +int i, first, last, ret;
 +char *cpus, *ptr;
 +int cpuSet = 0;
 +int remaining = 128;
 +
 +if (VIR_ALLOC_N(cpus, remaining)  0)
 +return NULL;
 +
 +ptr = cpus;
 +
 +for (i = 0; i  VIR_DOMAIN_CPUMASK_LEN; i++) {
 +if (cpumask[i]) {
 +if (cpuSet)
 +last = i;
 +else {
 +first = last = i;
 +cpuSet = 1;
 +}
 +} else {
 +if (!cpuSet)
 +continue;
 +if (first == last)
 +ret = snprintf(ptr, remaining, %d,, first);
 +else
 +ret = snprintf(ptr, remaining, %d-%d,, first, last);
 +if (ret  remaining)
 +goto error;
 +ptr += ret;
 +remaining -= ret;
 +cpuSet = 0;
 + }
 +}
 +
 +if (cpuSet) {
 +if (first == last)
 +ret = snprintf(ptr, remaining, %d,, first);
 +else
 +ret = snprintf(ptr, remaining, %d-%d,, first, last);
 +if (ret  remaining)
 +goto error;
 +}
 +
 +/* Remove the trailing comma */
 +*(--ptr) = '\0';
 +return cpus;
 +
 +error:
 +VIR_FREE(cpus);
 +return NULL;

Using VIR_ALLOC_N + snprintf here is not desirable, when you
already have a nice  virBufferPtr object in the call that you
could use.  Just pass the virBufferPtr straight into this
method.

 +}
 +
 +static int
 +qemuBuildNumaArgStr(const virDomainDefPtr def, virCommandPtr cmd)
 +{
 +int i;
 +char *cpus, *node;
 +virBuffer buf = VIR_BUFFER_INITIALIZER;
 +
 +for (i = 0; i  def-cpu-nnodes; i++) {
 +virCommandAddArg(cmd, -numa);
 +virBufferAsprintf(buf, %s, node);
 +virBufferAsprintf(buf, ,nodeid=%d, def-cpu-nodes[i].nodeid);
 +
 +cpus = virParseNodeCPUs(def-cpu-nodes[i].cpumask);
 +if (!cpus)
 +goto error;
 +
 +virBufferAsprintf(buf, ,cpus=%s, cpus);
 +virBufferAsprintf(buf, ,mems=%d, def-cpu-nodes[i].mem);
 +
 +if (virBufferError(buf)) {
 +VIR_FREE(cpus);
 +goto error;
 +}
 +
 +node = virBufferContentAndReset(buf);
 +virCommandAddArg(cmd, node);
 +
 +VIR_FREE(cpus);
 +VIR_FREE(node);
 +}
 +return 0;
 +
 +error:
 +virBufferFreeAndReset(buf);
 +virReportOOMError();
 +return -1;
 +}
  
  /*
   * Constructs a argv suitable for launching qemu with config defined
 @@ -3319,6 +3410,9 @@ qemuBuildCommandLine(virConnectPtr conn,
  virCommandAddArg(cmd, smp);
  VIR_FREE(smp);
  
 +if (def-cpu-nnodes  qemuBuildNumaArgStr(def, cmd))
 +goto error;
 +
  if (qemuCapsGet(qemuCaps, QEMU_CAPS_NAME)) {
  virCommandAddArg(cmd, -name);
  if (driver-setProcessName 

Looks fine code wise. For the future iterations, it would be good to
change the split of patches slightly

  - Patch 1 for XML bits:  cpu_conf.c, cpu_conf.h, and docs/schemas/domain.rn
  - Patch 2 for qemu_command.c, tests/qemuxml2argvtest.c

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 2/2] Routines to parse numa ... /numa

2011-10-13 Thread Bharata B Rao 
On Thu, Oct 13, 2011 at 12:50:00PM +0100, Daniel P. Berrange wrote:
 On Mon, Oct 03, 2011 at 03:31:35PM +0530, Bharata B Rao wrote:
  Routines to parse numa ... /numa
  
  From: Bharata B Rao bhar...@linux.vnet.ibm.com
  
  This patch adds routines to parse guest numa
  XML configuration for qemu.
  
  Signed-off-by: Bharata B Rao bhar...@linux.vnet.ibm.com
  ---
  
   src/conf/cpu_conf.c |   48 
   src/conf/cpu_conf.h |   11 ++
   src/qemu/qemu_command.c |   94 
  +++
   3 files changed, 153 insertions(+), 0 deletions(-)
  
  
  diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
  index a13ba71..5b4345e 100644
  --- a/src/qemu/qemu_command.c
  +++ b/src/qemu/qemu_command.c
  @@ -3153,6 +3153,97 @@ qemuBuildSmpArgStr(const virDomainDefPtr def,
   return virBufferContentAndReset(buf);
   }
   
  +static char *
  +virParseNodeCPUs(char *cpumask)
 
 This is a rather misleading name, since it is really formatting
 the argument value. Can you rename this to qemuBuildNumaCPUArgStr

Sure will do.

 
  +{
  +int i, first, last, ret;
  +char *cpus, *ptr;
  +int cpuSet = 0;
  +int remaining = 128;
  +
  +if (VIR_ALLOC_N(cpus, remaining)  0)
  +return NULL;
  +
  +ptr = cpus;
  +
  +for (i = 0; i  VIR_DOMAIN_CPUMASK_LEN; i++) {
  +if (cpumask[i]) {
  +if (cpuSet)
  +last = i;
  +else {
  +first = last = i;
  +cpuSet = 1;
  +}
  +} else {
  +if (!cpuSet)
  +continue;
  +if (first == last)
  +ret = snprintf(ptr, remaining, %d,, first);
  +else
  +ret = snprintf(ptr, remaining, %d-%d,, first, last);
  +if (ret  remaining)
  +goto error;
  +ptr += ret;
  +remaining -= ret;
  +cpuSet = 0;
  +   }
  +}
  +
  +if (cpuSet) {
  +if (first == last)
  +ret = snprintf(ptr, remaining, %d,, first);
  +else
  +ret = snprintf(ptr, remaining, %d-%d,, first, last);
  +if (ret  remaining)
  +goto error;
  +}
  +
  +/* Remove the trailing comma */
  +*(--ptr) = '\0';
  +return cpus;
  +
  +error:
  +VIR_FREE(cpus);
  +return NULL;
 
 Using VIR_ALLOC_N + snprintf here is not desirable, when you
 already have a nice  virBufferPtr object in the call that you
 could use.  Just pass the virBufferPtr straight into this
 method.

Wanted to user virBufferPtr, but I realized that I need to remove
the last comma from the string and coudn't find an easy way to
do that. Hence resorted to this method. But I think I can still
achive this by not appending a comma to the next part of the agrument
(,mems). Let me see if I can do this cleanly in the next post.

 
  +}
  +
  +static int
  +qemuBuildNumaArgStr(const virDomainDefPtr def, virCommandPtr cmd)
  +{
  +int i;
  +char *cpus, *node;
  +virBuffer buf = VIR_BUFFER_INITIALIZER;
  +
  +for (i = 0; i  def-cpu-nnodes; i++) {
  +virCommandAddArg(cmd, -numa);
  +virBufferAsprintf(buf, %s, node);
  +virBufferAsprintf(buf, ,nodeid=%d, def-cpu-nodes[i].nodeid);
  +
  +cpus = virParseNodeCPUs(def-cpu-nodes[i].cpumask);
  +if (!cpus)
  +goto error;
  +
  +virBufferAsprintf(buf, ,cpus=%s, cpus);
  +virBufferAsprintf(buf, ,mems=%d, def-cpu-nodes[i].mem);
  +
  +if (virBufferError(buf)) {
  +VIR_FREE(cpus);
  +goto error;
  +}
  +
  +node = virBufferContentAndReset(buf);
  +virCommandAddArg(cmd, node);
  +
  +VIR_FREE(cpus);
  +VIR_FREE(node);
  +}
  +return 0;
  +
  +error:
  +virBufferFreeAndReset(buf);
  +virReportOOMError();
  +return -1;
  +}
   
   /*
* Constructs a argv suitable for launching qemu with config defined
  @@ -3319,6 +3410,9 @@ qemuBuildCommandLine(virConnectPtr conn,
   virCommandAddArg(cmd, smp);
   VIR_FREE(smp);
   
  +if (def-cpu-nnodes  qemuBuildNumaArgStr(def, cmd))
  +goto error;
  +
   if (qemuCapsGet(qemuCaps, QEMU_CAPS_NAME)) {
   virCommandAddArg(cmd, -name);
   if (driver-setProcessName 
 
 Looks fine code wise. For the future iterations, it would be good to
 change the split of patches slightly
 
   - Patch 1 for XML bits:  cpu_conf.c, cpu_conf.h, and docs/schemas/domain.rn
   - Patch 2 for qemu_command.c, tests/qemuxml2argvtest.c

Sure will rearrange in the next iteration.

Thanks for your review.

Regards,
Bharata.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list