Date:	Mon, 5 May 2014 10:29:48 +0200
From:	Levente Kurusa <levex@...ux.com>
To:	Dominique van den Broeck <domdevlin@...e.fr>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect
 declarations

Hi,

On Mon, May 05, 2014 at 01:59:49AM +0200, Dominique van den Broeck wrote:
> 
> Good evening,
> 
> Forgive my mistakes, I sent only a few patches yet and I'm still learning.
> Nevertheless:

Not a problem at all, since that is what the challenge is for.

> 
> > What is that period in the commit message? And the semicolon?
> 
> Semicolons is what one use to ponctuate an enumerated list (at least
> in french). In fact, it was their primary use before computer era.
> 
> Is there something wrong with it ? I check all my patches with
> checkpatch.pl --strict before sending them and it didn't seem to
> complain...

Oh, I see now! Well I guess it's better to have a commit message
that says WHY the change is good and WHAT did it change, not just
a list of what you did.

i.e. in this case you could also inline the sparse warning or at least
a part of it.

> 
> > You should also be a bit more specific. Also, the Subject line is
> > very bad. Better go with something like this:
> > 
> > staging: rtl8192e: fix userspace pointer dereference
> 
> Right. I used the slash as a subpart of the tree. Didn't know what
> was the best for this situation.

That is good as well, I just prefer the latter one, but feel
free to use whichever you feel better. :-)

> 
> > And when you resend a patchset, please resend the full patchset.
> 
> I usually do but I've got an acceptation message for the first one
> (see below).
> 

In that case, that's good.

> > This is totally unneccessary.
> 
> Should at least have gone in the body below indeed.
> 
> > When you cite a commit please don't include the full hash, that is
> > non informational. Better put the first 7 characters of the hash and
> > the first line of the commit message as well in parantheses, like so:
> >
> > 5169af2 ("Staging: rtl8192e: Fix declaration of symbols")
> > (I even have a command for this in vim :-) )
> 
> I'm interested. I use vim too.
> 

I have this:
----------%<-----------
function! GetGitCommit(commit)
    exec ":.!git log --oneline --pretty=\"format:\\%h ('\\%s')\" ".a:commit." -1"
endfunction
---------->%-----------

> > Are you sure that 1/2 was applied to the staging tree?
> > It's unlikely that 1/2 is applied while 2/2 is left alone.
> 
> As stated before, I received the common automatic mail from
> Greg KH regarding this one. So I'll now wait before I do a v3
> for this issue. If so, I'll resent the complete set if required.

No, if that was applied it's unneccessary to resend the full set.

> 
> > Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
> > In which tree is it?
> 
> It's linux-next. If I quote a commit, I should the tree as well,
> indeed. But since the v1 was performed partially for the Eudyptula
> Project and since it was a response to a modification request, 
> I though it was implicit.

No, the tree's name is not needed. In fact, I should have checked it
in linux-next, but I only checked Linus', and staging-next thinking,
since you said it was applied, it was applied to staging-next. :-)
> 
> > Could you please as well remove that empty line in the declarations?
> 
> I'll do.
> Cheers.

Regards,
Levente Kurusa

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)

Powered by blists - more mailing lists