On 04/02/2013 08:22 AM, Michal Privoznik wrote: > --- > HACKING | 24 +- > daemon/libvirtd-config.c | 29 +-- > daemon/libvirtd.c | 32 +-- > daemon/remote.c | 292 +++++++--------------- > daemon/stream.c | 4 +- > docs/hacking.html.in | 24 +-
Again, splitting this into multiple patches might ease review (if only
by making it something where people could tackle review in parallel),
even if it results in the same amount of content spread over an even
larger number of messages. I'm okay if the intermediate tree has bugs
in its quality of OOM error messages (with a double report probably
better than no error reporting), as long as it still builds and the
series end cleans it back up. If I understand correctly, the only
reason you tried to cram everything into one patch was to avoid an
intermediate temporary state.
> 185 files changed, 2130 insertions(+), 5646 deletions(-)
At any rate, this is a move in the right direction for reduced source
size :)
>
> diff --git a/HACKING b/HACKING
> index c511cab..3d8c611 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -501,16 +501,16 @@ Low level memory management
> Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt
> codebase, because they encourage a number of serious coding bugs and do not
> enable compile time verification of checks for NULL. Instead of these
> -routines, use the macros from memory.h.
> +routines, use the macros from memory.h. Note, that these functions
s/Note,/Note/
> +automatically report OOM error. If you want to suppress such behaviour, use
I couldn't quickly determine if HACKING favors US (behavior) or UK
(behaviour) spelling, so I don't know if this is creating an
inconsistent mix, or even if it is the first locale-dependent word in
the document.
grep 'iou\?r\b' docs/hacking.html.in
didn't help me, unfortunately.
> +VIR_ALLOCNOOM() and variants.
Will need to be adjusted for whatever naming we end up with.
> @@ -210,15 +208,13 @@ daemonConfigFilePath(bool privileged, char **configfile)
>
> if (virAsprintf(configfile, "%s/libvirtd.conf", configdir) < 0) {
> VIR_FREE(configdir);
> - goto no_memory;
> + goto error;
> }
> VIR_FREE(configdir);
> }
>
> return 0;
>
> -no_memory:
> - virReportOOMError();
> error:
> return -1;
For functions like this, it might even be worth:
int ret = -1;
char *configdir = NULL;
...
if (virAsprintf(configfile, ...) < 0)
goto cleanup;
ret = 0;
cleanup:
VIR_FREE(configdir);
return ret;
On the other hand, your approach of minimal changes, in order to make
the initial conversion, makes sense; and additional cleanups like I just
mentioned would best be in separate patches rather than lumped in to the
initial conversion.
> @@ -1878,10 +1812,8 @@ remoteDispatchDomainMigratePrepare2(virNetServerPtr
> server ATTRIBUTE_UNUSED,
> dname = args->dname == NULL ? NULL : *args->dname;
>
> /* Wacky world of XDR ... */
> - if (VIR_ALLOC(uri_out) < 0) {
> - virReportOOMError();
> + if (VIR_ALLOC(uri_out) < 0)
> goto cleanup;
> - }
Up to here, I looked closely at the changes to make sure you were
generally making sense. Beyond here, I'll look for key points
(src/util/virstring.h) and random spots, but don't expect that I looked
closely at all 800k of this patch :)
> +++ b/src/qemu/qemu_agent.c
> @@ -514,10 +514,8 @@ qemuAgentIORead(qemuAgentPtr mon)
>
> if (avail < 1024) {
> if (VIR_REALLOC_N(mon->buffer,
> - mon->bufferLength + 1024) < 0) {
> - virReportOOMError();
> + mon->bufferLength + 1024) < 0)
> return -1;
> - }
Another random spot where I stopped hitting delete; still looks good.
> +++ b/src/util/virstring.c
> @@ -85,8 +85,10 @@ char **virStringSplit(const char *string,
> if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0)
> goto no_memory;
>
> - if (!(tokens[ntokens] = strndup(remainder, len)))
> + if (!(tokens[ntokens] = strndup(remainder, len))) {
Again the comment I made earlier that if we add VIR_STRDUP, then we
should also add VIR_STRNDUP.
> + virReportOOMError();
> goto no_memory;
> + }
> ntokens++;
> remainder = tmp + delimlen;
> tmp = strstr(remainder, delim);
> @@ -108,7 +110,6 @@ char **virStringSplit(const char *string,
> return tokens;
>
> no_memory:
> - virReportOOMError();
> for (i = 0 ; i < ntokens ; i++)
> VIR_FREE(tokens[i]);
> VIR_FREE(tokens);
> @@ -144,12 +145,8 @@ char *virStringJoin(const char **strings,
> return NULL;
> }
> ret = virBufferContentAndReset(&buf);
> - if (!ret) {
> - if (!(ret = VIR_STRDUP(""))) {
> - virReportOOMError();
> - return NULL;
> - }
> - }
> + if (!ret)
> + ret = VIR_STRDUP("");
> return ret;
> }
This is reasonable for the changes to virstring.c, but where are the
changes to virstring.h that turn on OOM reporting in the first place (or
was it viralloc.[ch] that needed the tweak)? If you apply this patch as
is, then the 'report' variable will still be false in the bulk of the
call sites.
> +++ b/tests/commandtest.c
> @@ -820,10 +820,8 @@ static int test20(const void *unused ATTRIBUTE_UNUSED)
>
> sigaction(SIGPIPE, &sig_action, NULL);
>
> - if (virAsprintf(&buf, "1\n%100000d\n", 2) < 0) {
> - virReportOOMError();
> + if (virAsprintf(&buf, "1\n%100000d\n", 2) < 0)
> goto cleanup;
> - }
Another random spot - still looks good. Maybe not sed'able, but
certainly mechanical. I've heard about coccinelle that supposedly makes
it easier to do syntax-based transformations, but haven't used it myself
to know if it would make this patch any easier to write.
Looking forward to v2 [is that the right thing to say, or did I just
volunteer myself to another megabyte of skimming?]
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
