[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]