| Message ID | 52BE915D.7090202@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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; }
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(-)