Firstly, thank you for your work about the related patches and replying so much details.
On 1/27/15 19:04, Michael Tokarev wrote: > 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... ;) > 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. > 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. > OK, what you said above sounds reasonable to me (in the unlock function, the code really free resources, though). > 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 ...". > 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". > "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. > OK, thanks, what you said above sounds reasonable to me. I shall notice about infinitive verb, next time. > 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. > OK, thanks, what you said above sounds reasonable to me, I shall notice about "do not miss subject in formal using". > 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. > OK. > > 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. > OK, We need the subject in formal using. > 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. > 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. > "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". > It is the same discussion in some area above of this thread. > 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. > 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. > > > 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. > OK, missing subject in formal using. > 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. > 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. > > 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 > OK, thanks, what you said above sounds reasonable to me, I shall notice about it, next time. > > 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... ;) > 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. > Anyway. I applied the patch, keeping your semantical wording > but fixing the obvious grammar problems. Thank you for the > work! > OK, thank you too. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
