Dave Allan <dallan@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list