[libvirt] [PATCH] trivial libvirt example code

[Jim Meyering]

Dave Allan <dallan at redhat.com> wrote:
...
>>> +static int
>>> +showDomains(virConnectPtr conn)
>>> +{
>>> +    int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
>>> +    char **nameList = NULL;
>>> +
>>> +    numActiveDomains = virConnectNumOfDomains(conn);
>>> +    numInactiveDomains = virConnectNumOfDefinedDomains(conn);
>>
>> It'd be good to handle numInactiveDomains < 0 differently.
>> Currently it'll probably provoke a failed malloc, below.
>
> Doh--thanks.  I missed that those calls could fail.

The warning sign for me was that they were declared to be
of type "int".  I asked myself "why?": for something that's
supposed to count, why allow negative numbers.

>>> +    printf("There are %d active and %d inactive domains\n",
>>> +           numActiveDomains, numInactiveDomains);
>>> +
>>> +    nameList = malloc(sizeof(char *) * (unsigned int)numInactiveDomains);
...
>>> +    if (NULL != nameList) {
>>> +        free(nameList);
>>
>> The test for non-NULL-before-free is unnecessary,
>> since free is guaranteed to handle NULL properly.
>> So just call free:
>>
>>        free(nameList);
>>
>> In fact, if you run "make syntax-check" before making the
>> suggested change, it should detect and complain about this code.
>
> Removed.  (make syntax-check does not complain, btw)

Ah. thanks for mentioning that.
The script that detects those detects "if (expr != NULL) free(expr)",
but didn't bother to match the less common case where NULL is first:
"if (NULL != expr) free(expr)".  I've made the upstream source of
that script detect your syntax, too:

  http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=471cbb075f7

so none of those will slip into libvirt either, once we do the
next gnulib->libvirt sync.

...
>> I noticed that you're using the git mirror.  Good!  ;-)
>> When posting patches, please use "git format-patch".
>
> Will do.
>
>> That would have made it easier for me to apply and test
>> your patches.  As it is, I didn't do either because
>> "git am FILE" didn't work:
>>
>>     $ git am dallan.patch
>>     Applying: trivial libvirt example code
>>     warning: examples/hellolibvirt/hellolibvirt.c has type 100755, expected 100644
>>     error: patch failed: examples/hellolibvirt/hellolibvirt.c:97
>>     error: examples/hellolibvirt/hellolibvirt.c: patch does not apply
>>     Patch failed at 0001 trivial libvirt example code
>>     When you have resolved this problem run "git am --resolved".
>>     If you would prefer to skip this patch, instead run "git am --skip".
>>     To restore the original branch and stop patching run "git am --abort".
>>
>> Note the warning about permissions on hellolibvirt.c.
>> You can correct that by running "chmod a-x hellolibvirt.c".
>
> The permissions problem is strange--it's 644 in my development tree, and
> the patch I sent has:
> diff --git a/examples/hellolibvirt/hellolibvirt.c
> b/examples/hellolibvirt/hellolibvirt.c
> new file mode 100644
>
> What would cause git-am to think it was 755?

I'll investigate if it happens again.

>> Here are some contribution guidelines that generally make it
>> easier for maintainers/committers to deal with contributed patches,
>> (though some parts are specific to git-managed projects):
>>
>>     http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=HACKING;hb=HEAD
>
> Good stuff.
>
> When I have a patch like this that people have commented on and I've
> modified it slightly in response, what's the best way to re-submit it?
> When Rich responded, I submitted both the entire patch with the changes
> as well as the changes separately.

Personally I find 2nd-iteration reviews to work best when the
incremental patch is posted with either 1) the preceding
or 2) the squashed/final patch.
Otherwise, I have to apply the preceding patch and the new patch
on separate git branches, then diff those branches to see
the incremental.  That's not frustrating and inefficient.