[Qemu-devel] [PATCH] vl.c: move "if (fd < 0)" into "if (fd <= STDERR_FILENO)"

[Qemu-devel] [PATCH] vl.c: move "if (fd < 0)" into "if (fd <= STDERR_FILENO)"

[Chen Gang]

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.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/vl.c b/vl.c
index 2160933..fbea535 100644
--- a/vl.c
+++ b/vl.c
@@ -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;
     }
 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH] vl.c: move "if (fd < 0)" into "if (fd <= STDERR_FILENO)"

[Peter Maydell]

On 28 December 2013 08:52, Chen Gang <gang.chen.5i5j@gmail.com> 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).
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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] vl.c: move "if (fd < 0)" into "if (fd <= STDERR_FILENO)"

[Chen Gang]

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 <gang.chen.5i5j@gmail.com> 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