From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46619) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNmO8-0007JT-Pd for qemu-devel@nongnu.org; Tue, 17 Feb 2015 12:56:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNmO5-0003zc-HF for qemu-devel@nongnu.org; Tue, 17 Feb 2015 12:56:48 -0500 Received: from e8.ny.us.ibm.com ([32.97.182.138]:34694) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNmO5-0003zQ-DT for qemu-devel@nongnu.org; Tue, 17 Feb 2015 12:56:45 -0500 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 17 Feb 2015 12:56:44 -0500 Received: from b01cxnp23032.gho.pok.ibm.com (b01cxnp23032.gho.pok.ibm.com [9.57.198.27]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 999F738C805E for ; Tue, 17 Feb 2015 12:52:48 -0500 (EST) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by b01cxnp23032.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t1HHugdW29491216 for ; Tue, 17 Feb 2015 17:56:42 GMT Received: from d01av04.pok.ibm.com (localhost [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t1HHugsM015981 for ; Tue, 17 Feb 2015 12:56:42 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <54E36719.8040900@openvz.org> References: <1424142892-7275-1-git-send-email-mdroth@linux.vnet.ibm.com> <1424142892-7275-4-git-send-email-mdroth@linux.vnet.ibm.com> <54E35DEF.3020309@redhat.com> <54E36719.8040900@openvz.org> Message-ID: <20150217175635.13315.80188@loki> Date: Tue, 17 Feb 2015 11:56:35 -0600 Subject: Re: [Qemu-devel] [PATCH 03/10] guest agent: guest-file-open: refactoring List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , Eric Blake , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, Simon Zolin Quoting Denis V. Lunev (2015-02-17 10:06:49) > On 17/02/15 18:27, Eric Blake wrote: > > On 02/16/2015 08:14 PM, Michael Roth wrote: > >> From: Simon Zolin > >> > >> Moved the code that sets non-blocking flag on fd into a separate funct= ion. > >> > >> Signed-off-by: Simon Zolin > >> Reviewed-by: Roman Kagan > >> Signed-off-by: Denis V. Lunev > >> CC: Michael Roth > >> CC: Eric Blake > >> Signed-off-by: Michael Roth > >> --- > >> qga/commands-posix.c | 31 +++++++++++++++++++++++-------- > >> 1 file changed, 23 insertions(+), 8 deletions(-) > >> > >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c > >> index 57409d0..ed527a3 100644 > >> --- a/qga/commands-posix.c > >> +++ b/qga/commands-posix.c > >> @@ -376,13 +376,33 @@ safe_open_or_create(const char *path, const char= *mode, Error **errp) > >> return NULL; > >> } > >> = > >> +static int guest_file_toggle_flags(int fd, int flags, bool set, Error= **err) > >> +{ > > Why are you reinventing qemu_set_nonblock()? > > > because we are uneducated :) > = > Anyway, qemu_set_nonblock() does not handle error > and resides in a strange header aka "include/qemu/sockets.h" > Technically I can switch to it immediately. Though error > check condition will be lost. > = > What is better at your opinion? > = > a) return error from qemu_set_nonblock()/qemu_set_block() I think making the existing functions a non-error-checking wrapper around qemu_set_{block,nonblock}_error or something would be best. These are meant to be os-agnostic utility functions though, but in the case of qemu-ga the win32 variant won't work as expected, so I'm not sure it's a good idea to rely on them. If the lack of it's usage in net/tap* compared to other parts of QEMU that build on w32 is any indication, that seems to be the pattern followed by other users. In any case, since I was actually the one who re-invented it, and this code just moves it to another function, I think we can address it as a seperate patch and leave the PULL intact (unless there are other objections). > b) drop error check here. The descriptor is just opened > and we know that it is valid. I could not imagine real > error other than broken descriptor for this exact fcntl. > = > Regards, > Den