From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47377) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VxEO7-0006bs-89 for qemu-devel@nongnu.org; Sun, 29 Dec 2013 06:18:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VxENy-0000GE-GS for qemu-devel@nongnu.org; Sun, 29 Dec 2013 06:18:31 -0500 Received: from mail-pd0-x236.google.com ([2607:f8b0:400e:c02::236]:64815) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VxENy-0000G3-8v for qemu-devel@nongnu.org; Sun, 29 Dec 2013 06:18:22 -0500 Received: by mail-pd0-f182.google.com with SMTP id v10so10383932pde.41 for ; Sun, 29 Dec 2013 03:18:20 -0800 (PST) Message-ID: <52C004F7.7020003@gmail.com> Date: Sun, 29 Dec 2013 19:18:15 +0800 From: Chen Gang MIME-Version: 1.0 References: <52BE915D.7090202@gmail.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vl.c: move "if (fd < 0)" into "if (fd <= STDERR_FILENO)" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: aliguori@us.ibm.com, QEMU Developers Firstly, thank you very much for your reply, this is my first patch for qemu. Next year (2014), as a volunteer, I will try to make a patch for qemu in each month. :-) On 12/29/2013 07:43 AM, Peter Maydell wrote: > On 28 December 2013 08:52, Chen Gang wrote: >> For valid 'fd' (in most cases), it is enough to only check whether it >> is larger than STDERR_FILENO, so recommend to move "if (fd < 0)" into >> failure processing block. > >> @@ -1064,15 +1064,10 @@ static int parse_add_fd(QemuOpts *opts, void *opaque) >> fdset_id = qemu_opt_get_number(opts, "set", -1); >> fd_opaque = qemu_opt_get(opts, "opaque"); >> >> - if (fd < 0) { >> - qerror_report(ERROR_CLASS_GENERIC_ERROR, >> - "fd option is required and must be non-negative"); >> - return -1; >> - } >> - >> if (fd <= STDERR_FILENO) { >> qerror_report(ERROR_CLASS_GENERIC_ERROR, >> - "fd cannot be a standard I/O stream"); >> + fd < 0 ? "fd option is required and must be non-negative" >> + : "fd cannot be a standard I/O stream"); >> return -1; >> } > > This patch doesn't change the behaviour, but I think it > makes the code less clear to read (because we've > folded two different error cases into one and then split > them out again with a ternary operator on the string). When we check the variable (e.g. 'fd') whether valid or not, we often try to use one code block ("if () {...}") or a function (when it is complex) to perform it. For readers, firstly, they mainly focus on the variable (e.g. 'fd') whether valid or not, they don't mainly focus on the variable failure details. > This isn't performance critical code (it's run only a few > times and only at startup), so I think we should favour > clarity and ease-of-reading, and I think the existing code > is better here. > Yeah. Thanks. -- Chen Gang Open, share and attitude like air, water and life which God blessed