On Fri, 05 Oct 2012 08:25:46 +0200
Paolo Bonzini <pbonz...@redhat.com> wrote:
> Il 04/10/2012 20:24, Luiz Capitulino ha scritto:
> > That DPRINTF() usage is really bizarre, it seems its purpose is to report
> > an error to the user, but that's a debugging call.
> >
> > I'd let it there and replace it later with proper tracing code, but that's
> > quite minor for me. Please, at least mention you're dropping it in the log.
>
> This one is not dropped, it becomes the reported error message.
What I meant is that the error/debug message won't be printed the same way
it was before. This is an improvement, but it's a good idea to mention it.
> >> > goto err_after_popen;
> >> > }
> >> >
> >> > s->fd = fileno(f);
> >> > if (s->fd == -1) {
> >> > - DPRINTF("Unable to retrieve file descriptor for popen'd
> >> > handle\n");
>
> This one is dropped, but I wanted to delete the check altogether.
> fileno() should only fail if it detects somehow that its argument is not
> a valid stream, which is obviously not the case.
>
> Would that be better? It would also fix the clobbering of errno.
I guess so. Is it possible for popen() to return success but then set the
FILE's fd to -1? Maybe change to an assert() instead?
