On Wed, Mar 30, 2011 at 13:46:12 -0600, Eric Blake wrote:
> On 03/30/2011 06:17 AM, Jiri Denemark wrote:
> >
> > + if (virAsprintf(&debug, ": %d: debug : ", vm->pid) < 0) {
>
> That's rather hard-coded to our current output style; should we add
> comments here and to the place that outputs those lines to be careful to
> keep the two places in sync?
Yeah, it makes sense to add these comments.
> > - got += ret;
> > + got += bytes;
> > buf[got] = '\0';
> > +
> > + /* Filter out debug messages from intermediate libvirt process */
> > + while ((eol = strchr(filter_next, '\n'))) {
> > + char *p = strstr(filter_next, debug);
>
> Suppose you have five normal lines before the first debug line. Is it
> any more efficient to use use getline() to read a line at a time
Heh, the one which needs FILE *? No, thanks :-)
> Or even if you don't use getline(), can you temporarily set eol to NUL
Sure, that actually even results in a bit nicer code.
> > + if (p && p < eol) {
> > + memmove(filter_next, eol + 1, got - (eol - buf));
>
> Conversely, supposed you have five debug lines followed by five normal
> lines. Doing the memmove() on every debug line encountered results in
> moving 25 lines around, whereas if you rewrite the loop to only copy one
> line at a time and only if it passed the filter, then you only have to
> move 5 lines around.
And all this after we went through fork() and going to call exec() soon, I
don't think such optimization would make any difference. If this code had been
run on any libvirt API, it would have make sense to optimize the loop, but now
I don't think it's worth it. It looks like a premature optimization to me :-)
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list