Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

From: Luiz Capitulino <lcapitulino@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols
Date: Fri, 5 Oct 2012 09:41:05 -0300	[thread overview]
Message-ID: <20121005094105.60818a32@doriath.home> (raw)
In-Reply-To: <506E7D6A.3040206@redhat.com>

On Fri, 05 Oct 2012 08:25:46 +0200
Paolo Bonzini <pbonzini@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?