From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36063) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TK7CW-0005P0-0c for qemu-devel@nongnu.org; Fri, 05 Oct 2012 08:40:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TK7CP-0005ff-NE for qemu-devel@nongnu.org; Fri, 05 Oct 2012 08:40:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14445) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TK7CP-0005ea-FI for qemu-devel@nongnu.org; Fri, 05 Oct 2012 08:40:13 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q95CeCNu027175 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 5 Oct 2012 08:40:12 -0400 Date: Fri, 5 Oct 2012 09:41:05 -0300 From: Luiz Capitulino Message-ID: <20121005094105.60818a32@doriath.home> In-Reply-To: <506E7D6A.3040206@redhat.com> References: <1349275025-5093-1-git-send-email-pbonzini@redhat.com> <1349275025-5093-9-git-send-email-pbonzini@redhat.com> <20121004152435.475f879a@doriath.home> <506E7D6A.3040206@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel On Fri, 05 Oct 2012 08:25:46 +0200 Paolo Bonzini 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?