[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] linux-user/syscall.c: Free the vec[i] in fai
From: |
Chen Gang S |
Subject: |
Re: [Qemu-devel] [PATCH v2] linux-user/syscall.c: Free the vec[i] in failure processing code block |
Date: |
Wed, 28 Jan 2015 10:36:38 +0800 |
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
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