linux-user/syscall.c: Free the vec[i] in failure processing code block

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.

---
 linux-user/syscall.c | 5 +++++
 1 file changed, 5 insertions(+)

index 290fdea..a66c2ae 100644
@@ -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);

1.9.3 (Apple Git-50)

thanks
-- PMM

12345678901234567890123456789012345678901234567890123456789012345678901234567890
         1         2         3         4         5         6         7         8

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.

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

Firstly, thank you for your work about the related patches and replying so much details.

Yeah, I shall always be improving my English :-).

- Communicate with open source with English.

- Listen/read English version Holy Bible when I am in subway between my home and my office.

- Sometimes can get some suggestions (e.g. you in open source), they are welcome, and they are really useful to me.

OK, what you said above sounds reasonable to me (in the unlock function, the code really free resources, though).

For me, 'need' can also be used like 'can', 'may' or 'must', e.g.:

A: "Must I do it?"
B: "No, you needn't" -- "No, you needn't do it".

So for me, "I need do it" is also correct, just like "I can do it", or "I must do it", or "I should do it".

OK, thanks, what you said above sounds reasonable to me. I shall notice about infinitive verb, next time.

OK, thanks, what you said above sounds reasonable to me, I shall notice about "do not miss subject in formal using".

OK.

OK, We need the subject in formal using.

Excuse me, I am not quite sure about it: I can not find NULLiness (nulliness) in qemu git comments or in Linux kernel git comments.

It is the same discussion in some area above of this thread.

For me, if the final code is still simple and clear enough, I try to avoid to add comments in the internal area within functions.

If there may be any doubts for the code modification, I will add related comments for the patch (commit comments), not for the code.

I guess, after modification, the code is still clear enough, do not need any code comments.

OK, missing subject in formal using.

For me, these statements are not necessary as code comments, but for sending patch, I still suggest to provide them, it will let other members more easier to review the patch.

For applying patch, the final appliers can deside whether skip these statements or not.

OK, thanks, what you said above sounds reasonable to me, I shall notice about it, next time.

I am sure, I do not misunderstand your suggestions. And still welcome any related suggestions, although I am not quite sure whether another members may be boring in qemu mailing list.

OK, thank you too.

Chen Gang

Open, share, and attitude like air, water, and life which God blessed