23.01.2015 13:01, Chen Gang S wrote:
> When failure occurs during allocating vec[i], also need free all
> allocated vec[i] in failure processing code block before return.
>
> In unlock_user(), it will check vec[i].iov_base whether is NULL, so need
> not check it again outside.
>
> If error is EFAULT when "i == 0", vec[i].iov_base is NULL, then can just
> skip it, so can still use "while (--i >= 0)" for the free looping.
Oh well. I think you need to improve your English just a little bit... ;)
First of all, the talk here is about locking and unlocking, not about
allocating and freeing. So not "free", but "unlock", or else it
becomes a bit confusing, since you're adding unlock calls, not free.
Now, the language part. Please let me show your typical examples.
There are several typical kinds of these:
When failure occurs during allocating vec[i], also need free all
allocated vec[i] in failure processing code block before return.
"need _to_ free", it is always "need TO do something", since the
verb after need is always infinitive. There are several words
like this -- need to, have to, want to, -- when used in a similar
construct. Ofcourse I don't talk about other usage -- like "I
need coffee". Another alternative is to use word "should" --
"we should free ...".
"allocating vec[i]" -- "of" is missing, "allocating OF vec[i]".
Alternative -- "vec[i] allocating", or better yet, "vec[i]
allocation". This is like making a noun from a verb -- "to
allocate" is a verb (infinitive), "allocating" is the same
verb in present continous tense, and "allocation" is a process,
like a noun. It is during this process we hit the error.
In this part of sentence, you don't have a subject. English statements
almost always have a subject. In other words, _whp_ need to free?
Here, it is possible to use the word "we", like "we need to free..".
For a subject-less construct, it is possible to use construct "there
is" -- in this case it will be "there's a need to free..", but it
has more words, and the word "we" is short and adequate, since we
are the programmers who wrote this code.
So a bit better (from English perspective anyway) version of this
your statement is:
When failure occurs during vec[i] allocation, we need to free
all allocated vec[i] in failure processing code block before return.
(I omitted "also", but that wasn't really necessary. I _think_
it referred to the unlocking/freeing of vec itself -- in other
words, we should not only free vec, but ALSO every individual
vec[i].)
It is still quite a bit "raw", and unusual usage of English,
but it is more english-like.
>From technical standpoint, I think, it is sufficient to say
something like "in failure path we forgot to unlock starting
vec[i] elements which we successfully locked" -- this should
be a (more or less) good English, short, and correct technically.
In unlock_user(), it will check vec[i].iov_base whether is NULL, so need
not check it again outside.
"In unlock_user(), it will.." -- "it" here is not like in "it rains",
which is more or less subject-less statement. Here you can use "it"
when you "named" or mentioned it previously. For example, "the table
was square, it had long curly legs" -- here, "it" refers to "table".
So the more english-correct usage is either "unlock_user() will..." --
making "unlock_user()" a subject -- not very common for a function;
or - a bit more clumsy - "Code in unlock_user(), it will.." -- making
"Code" a subject. Or combining the two, "Code in unlock_user() will.." -
this is the most correct but a bit too long. Note that if you omit
the first "In", last "outside" becomes stray.
Now, "will check THAT <.> is NULL", or "will check IF <.> is NULL",
or "will check whether <.> is NULL", or other variants. Note the
placement of words. Alternative is "will check <.> FOR NULLiness"
(I'm not really sure it is the correct form). I think it should be
clear what's the difference between two word placements.
"so need not check" -- the same 2 probs as before. No subject, and
a verb after "need" should be in infinitive form. Correct version is
"so there's no need to check", or, making "check" a subject (noun
from verb), "so [this] check is not needed".
But this whole sentense is a bit stray by itself. When reading it
I thought that it is the current code in lock_iovec() which does
something unnecessary (checking whether iov_base is NULL). But it
turned out that this way you describe why you didn't add such a
check in NEW code this patch is adding. This is a bit more difficult
to describe and to suggest a better version, because there's no
obvious grammar rule to follow. Maybe it is just better to put
this statement into code comment, not into a commit message, or
add a line in the commit message telling that the below is about
how we do what needs to be done.
So anyway, my suggested wording is something like:
Since unlock_user() checks whether iov_base is NULL, there's no
need to do it before calling that function.
If error is EFAULT when "i == 0", vec[i].iov_base is NULL, then can just
skip it, so can still use "while (--i >= 0)" for the free looping.
"then can skip" -- again, no subject. "Then we can skip it", "Then it
is okay to skip it" and so on. "So can still use" - the same.
This is a good candidate for code comment actually, not for a commit
message. Again, I thought it was about existing code, but in fact
it describes the code you're adding, "proving" your code.
Maybe something like this:
In a corner case, if error occurs on first iteration (i == 0),
vec[i].iov_base will be NULL and there's no need to unlock it,
so it is still okay to use (--i >= 0) condition in the loop.
But actually, both these statements aren't really necessary,
I think.
And one more thing: The subject line. You jumped from a filename
(quite large!) right to a local variable. So one may think that
this whole file contains just one small function with a variable
which needs to be freed... ;) I'd use something like this:
linux-user/syscall.c: unlock vec[i] in failure processing code in lock_iovec()
or
linux-user/syscall.c: lock_iovec: unlock vec[i] in failure processing code
that's if you really want to go to this level of details up to
variable name which is being unlocked/freed -- this way you
cover a gap between high-level (file) and too low-level (variable
name) which you're jumping over. Alternatively, less details
is possible too, like:
linux-user/syscall.c: lock_iovec: fix error path resource leak
Please don't get me wrong -- I'm not saying "you're bad" or
anything like that, I'm trying to help -- or else I'd not write
so much text (it takes some time too ;). I didn't want to
offend you in any way. And more: I'm not a native English
speaker myself, my English is _far_ from perfect, I make
many mistakes too.. So maybe I don't even have a right to
correct someone else's mistakes... ;)
Anyway. I applied the patch, keeping your semantical wording
but fixing the obvious grammar problems. Thank you for the
work!
/mjt
> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com>
> ---
> linux-user/syscall.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 290fdea..a66c2ae 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1873,6 +1873,11 @@ static struct iovec *lock_iovec(int type, abi_ulong
> target_addr,
> return vec;
>
> fail:
> + while (--i >= 0) {
> + if (tswapal(target_vec[i].iov_len) > 0) {
> + unlock_user(vec[i].iov_base, tswapal(target_vec[i].iov_base), 0);
> + }
> + }
> unlock_user(target_vec, target_addr, 0);
> fail2:
> free(vec);
>
