From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54282) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TK1M8-0004DO-3F for qemu-devel@nongnu.org; Fri, 05 Oct 2012 02:25:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TK1M7-00028n-37 for qemu-devel@nongnu.org; Fri, 05 Oct 2012 02:25:52 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:56368) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TK1M6-00028S-TX for qemu-devel@nongnu.org; Fri, 05 Oct 2012 02:25:51 -0400 Received: by mail-wi0-f181.google.com with SMTP id hq12so95870wib.10 for ; Thu, 04 Oct 2012 23:25:50 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <506E7D6A.3040206@redhat.com> Date: Fri, 05 Oct 2012 08:25:46 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1349275025-5093-1-git-send-email-pbonzini@redhat.com> <1349275025-5093-9-git-send-email-pbonzini@redhat.com> <20121004152435.475f879a@doriath.home> In-Reply-To: <20121004152435.475f879a@doriath.home> Content-Type: text/plain; charset=ISO-8859-1 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: qemu-devel , Luiz Capitulino 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. >> > 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. >> > goto err_after_open; >> > } >> > >> > @@ -85,12 +83,12 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command) >> > s->write = file_write; >> > >> > migrate_fd_connect(s); >> > - return 0; >> > + return; >> > >> > err_after_open: >> > pclose(f); >> > err_after_popen: >> > - return -1; >> > + error_setg_errno(errp, errno, "failed to popen the migration target"); > The pclose() call will override errno. Paolo