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.5...@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
