Re: [libvirt] [PATCH v3 9/8] hellolibvirt: Adjust some comments and condition usage

2013-03-04 Thread Eric Blake 
On 02/27/2013 05:28 AM, John Ferlan wrote:
 On 02/26/2013 07:54 PM, Eric Blake wrote:
 On 02/26/2013 07:20 AM, John Ferlan wrote:

 I would have put '---' here, since...

 
 The information was only in the email not part of the git history. 
 In the future I'll remember to put that after the '---'.

It makes the most difference on projects where you don't have push
rights, because the maintainer with push rights will use 'git am' to
snarf in your email message, which discards comments after ---.  Once
you have push rights yourself, 'git am' is no longer involved in the
loop, so there is nothing in the workflow to strip the comments, at
which point it doesn't matter where you injected the comments into your
email, other than the consistency factor.  At any rate, it's always nice
to know how to make the most use of a workflow that simplifies review
time, which includes being consistent by injecting your email-only
comments after ---.

 
 
 Here's the differences from the v3 to what seems to be a happy medium.  
 
 The virConnectNum* functions are still used just to show they exist 
 and how to use them.  There's a comment before the usage.
 
 
 
 diff --git a/examples/hellolibvirt/hellolibvirt.c 
 b/examples/hellolibvirt/hellol
 index 335a75e..26dd67f 100644
 --- a/examples/hellolibvirt/hellolibvirt.c
 +++ b/examples/hellolibvirt/hellolibvirt.c
 @@ -91,8 +91,14 @@ static int
  showDomains(virConnectPtr conn)
  {
  int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
 -char **nameList = NULL;
 -
 +int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
 +VIR_CONNECT_LIST_DOMAINS_INACTIVE;

Technically, a flags of 0 will give the same result (as the combination
of ACTIVE|INACTIVE covers all domains); but this is reasonable for the
example program (easier to see what to modify to get just half the set).

 +virDomainPtr *nameList = NULL;
 + * the current call.  A domain could be started or stopped and any
 + * assumptions made purely on these return values could result in
 + * unexpected results */
  numActiveDomains = virConnectNumOfDomains(conn);
  if (numActiveDomains == -1) {
  ret = 1;

 +/* Return a list of all active and inactive domains. Using this API
 + * instead of virConnectListDomains() and virConnectListDefinedDomains()
 + * is preferred since it solves an inherit race between separated API
 + * calls if domains are started or stopped between calls */
 +numNames = virConnectListAllDomains(conn,
 +nameList,
 +flags);
 +for (i = 0; i  numNames; i++) {
 +int active = virDomainIsActive(nameList[i]);
 +printf(  %8s (%s)\n,
 +   virDomainGetName(nameList[i]),
 +   (active == 1 ? active : non-active));
  /* must free the returned named per the API documentation */
 -free(*(nameList + i));
 +virDomainFree(nameList[i]);
  }
 +free(nameList);
  
  out:
 -free(nameList);
  return ret;

ACK.

 
 This changes the output from:
 
 Attempting to connect to hypervisor
 Connected to hypervisor at qemu:///system
 Hypervisor: QEMU version: 0.32.656
 There are 0 active and 2 inactive domains
 Inactive domains:
   foo
   bar
 Disconnected from hypervisor
 
 
 to
 
 Attempting to connect to hypervisor
 Connected to hypervisor at qemu:///system
 Hypervisor: QEMU version: 0.32.656
 There are 0 active and 2 inactive domains
foo (non-active)
bar (non-active)
 Disconnected from hypervisor

This might be worth putting in the commit message.

At any rate, since this is a doc-only change, I'm comfortable with you
pushing it before the 1.0.3 release.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 9/8] hellolibvirt: Adjust some comments and condition usage

2013-02-27 Thread John Ferlan 
On 02/26/2013 07:54 PM, Eric Blake wrote:
 On 02/26/2013 07:20 AM, John Ferlan wrote:
 
 I would have put '---' here, since...
 

The information was only in the email not part of the git history. 
In the future I'll remember to put that after the '---'.

  examples/hellolibvirt/hellolibvirt.c | 26 ++
  1 file changed, 14 insertions(+), 12 deletions(-)

 
 @@ -93,7 +94,7 @@ showDomains(virConnectPtr conn)
  char **nameList = NULL;
  
  numActiveDomains = virConnectNumOfDomains(conn);
 -if (-1 == numActiveDomains) {
 +if (numActiveDomains == -1) {
  ret = 1;
  printf(Failed to get number of active domains\n);
  showError(conn);
 
 virConnectNumOfDomains is inherently racy.  Wouldn't it be better to
 just drop this section of our example?
 

Considering the back and forth I decided to make less change because 
it's just an example, but I suppose I took too much off the top. 

So below are proposed adjustments.

 @@ -101,7 +102,7 @@ showDomains(virConnectPtr conn)
  }
  
  numInactiveDomains = virConnectNumOfDefinedDomains(conn);
 -if (-1 == numInactiveDomains) {
 +if (numInactiveDomains == -1) {
  ret = 1;
  printf(Failed to get number of inactive domains\n);
  showError(conn);
 
 Same question.
 
 @@ -113,17 +114,20 @@ showDomains(virConnectPtr conn)
  
  nameList = malloc(sizeof(*nameList) * numInactiveDomains);
  
 -if (NULL == nameList) {
 +if (!nameList) {
  ret = 1;
  printf(Could not allocate memory for list of inactive domains\n);
  goto out;
  }
  
 +/* The virConnectListDomains() will return a list of the active domains.
 + * Alternatively the virConnectListAllDomains() API would return a list
 + * of both active and inactive domains */
  numNames = virConnectListDefinedDomains(conn,
  nameList,
  numInactiveDomains);
 
 I think it would be better to update the example to JUST use
 virConnectListAllDomains(), and completely avoid
 virConnectListDefinedDomains.
 
  
 -if (-1 == numNames) {
 +if (numNames == -1) {
  ret = 1;
  printf(Could not get list of defined domains from hypervisor\n);
  showError(conn);
 @@ -136,9 +140,7 @@ showDomains(virConnectPtr conn)
  
  for (i = 0 ; i  numNames ; i++) {
  printf(  %s\n, *(nameList + i));
 -/* The API documentation doesn't say so, but the names
 - * returned by virConnectListDefinedDomains are strdup'd and
 - * must be freed here.  */
 +/* must free the returned named per the API documentation */
  free(*(nameList + i));
 
 Pre-existing, but '*(nameList + i)' looks odd; 'nameList[i]' looks nicer.
 


Here's the differences from the v3 to what seems to be a happy medium.  

The virConnectNum* functions are still used just to show they exist 
and how to use them.  There's a comment before the usage.



diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellol
index 335a75e..26dd67f 100644
--- a/examples/hellolibvirt/hellolibvirt.c
+++ b/examples/hellolibvirt/hellolibvirt.c
@@ -91,8 +91,14 @@ static int
 showDomains(virConnectPtr conn)
 {
 int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
-char **nameList = NULL;
-
+int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE;
+virDomainPtr *nameList = NULL;
+ * the current call.  A domain could be started or stopped and any
+ * assumptions made purely on these return values could result in
+ * unexpected results */
 numActiveDomains = virConnectNumOfDomains(conn);
 if (numActiveDomains == -1) {
 ret = 1;
@@ -112,40 +118,24 @@ showDomains(virConnectPtr conn)
 printf(There are %d active and %d inactive domains\n,
numActiveDomains, numInactiveDomains);
 
-nameList = malloc(sizeof(*nameList) * numInactiveDomains);
-
-if (!nameList) {
-ret = 1;
-printf(Could not allocate memory for list of inactive domains\n);
-goto out;
-}
-
-/* The virConnectListDomains() will return a list of the active domains.
- * Alternatively the virConnectListAllDomains() API would return a list
- * of both active and inactive domains */
-numNames = virConnectListDefinedDomains(conn,
-nameList,
-numInactiveDomains);
-
-if (numNames == -1) {
-ret = 1;
-printf(Could not get list of defined domains from hypervisor\n);
-showError(conn);
-goto out;
-}
-
-if (numNames  0) {
-printf(Inactive domains:\n);
-}
-
-for (i = 0 ; i  numNames ; i++) {
-printf(  %s\n, *(nameList + i));
+/* Return a list of all active and inactive domains. Using this API
+ * instead of

Re: [libvirt] [PATCH v3 9/8] hellolibvirt: Adjust some comments and condition usage

2013-02-26 Thread Eric Blake 
On 02/26/2013 07:20 AM, John Ferlan wrote:

I would have put '---' here, since...

  v3-v2 difference: Reduced the amount of change to a few comments and
 adjusting the (NULL == xxx) and (-1 == xxx) checks
 
 Since these are just documentation changes - once ACK'd is it OK to push
 now or should I wait for after the freeze?
 
 Tks,

...this information isn't useful in the final git log, but makes sense
in reviewing.  This patch is safe for 1.0.3 (as you point out, it is a
doc improvement, and can't cause any bugs in libvirtd)

 
 John
 ---
  examples/hellolibvirt/hellolibvirt.c | 26 ++
  1 file changed, 14 insertions(+), 12 deletions(-)
 

 @@ -93,7 +94,7 @@ showDomains(virConnectPtr conn)
  char **nameList = NULL;
  
  numActiveDomains = virConnectNumOfDomains(conn);
 -if (-1 == numActiveDomains) {
 +if (numActiveDomains == -1) {
  ret = 1;
  printf(Failed to get number of active domains\n);
  showError(conn);

virConnectNumOfDomains is inherently racy.  Wouldn't it be better to
just drop this section of our example?

 @@ -101,7 +102,7 @@ showDomains(virConnectPtr conn)
  }
  
  numInactiveDomains = virConnectNumOfDefinedDomains(conn);
 -if (-1 == numInactiveDomains) {
 +if (numInactiveDomains == -1) {
  ret = 1;
  printf(Failed to get number of inactive domains\n);
  showError(conn);

Same question.

 @@ -113,17 +114,20 @@ showDomains(virConnectPtr conn)
  
  nameList = malloc(sizeof(*nameList) * numInactiveDomains);
  
 -if (NULL == nameList) {
 +if (!nameList) {
  ret = 1;
  printf(Could not allocate memory for list of inactive domains\n);
  goto out;
  }
  
 +/* The virConnectListDomains() will return a list of the active domains.
 + * Alternatively the virConnectListAllDomains() API would return a list
 + * of both active and inactive domains */
  numNames = virConnectListDefinedDomains(conn,
  nameList,
  numInactiveDomains);

I think it would be better to update the example to JUST use
virConnectListAllDomains(), and completely avoid
virConnectListDefinedDomains.

  
 -if (-1 == numNames) {
 +if (numNames == -1) {
  ret = 1;
  printf(Could not get list of defined domains from hypervisor\n);
  showError(conn);
 @@ -136,9 +140,7 @@ showDomains(virConnectPtr conn)
  
  for (i = 0 ; i  numNames ; i++) {
  printf(  %s\n, *(nameList + i));
 -/* The API documentation doesn't say so, but the names
 - * returned by virConnectListDefinedDomains are strdup'd and
 - * must be freed here.  */
 +/* must free the returned named per the API documentation */
  free(*(nameList + i));

Pre-existing, but '*(nameList + i)' looks odd; 'nameList[i]' looks nicer.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list