Dave Allan <dallan@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.