Re: [libvirt] [PATCH] trivial libvirt example code
Dave Allan <dallan(a)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.