Thread (9 messages) 9 messages, 3 authors, 2011-03-04

RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions

From: KY Srinivasan <kys@microsoft.com>
Date: 2011-03-03 21:16:31
Also in: lkml

-----Original Message-----
From: Greg KH [mailto:gregkh@suse.de]
Sent: Thursday, March 03, 2011 1:10 AM
To: KY Srinivasan
Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
virtualization@lists.osdl.org; Haiyang Zhang; Hank Janssen
Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions

On Thu, Mar 03, 2011 at 02:50:00AM +0000, KY Srinivasan wrote:
quoted
quoted
quoted
quoted
"struct driver_context"?  Oh please no.
Greg; this is the patch that consolidates the state in  struct hv_driver into
struct driver_context. In the spirit of doing one thing in a patch;
other relevant changes are made in:
Patch[5/6]: Changes the name driver_context to hyperv_driver
Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver.
Yes, but on its own, this patch is wrong, that is not a valid name, even
if it is a "temporary" name.
Greg, the temporary name happens to be the name currently in use in the
code - this is not the name I introduced.
There is not a "struct driver_context" in the code that I see today, or
am I missing something?  That's my objection here, please don't use that
name, it's not valid for a subsystem to use, even for a tiny bit.
Look at the file vmbus.h  you will see struct driver_context. This has been there for as long 
as I have seen this code. 
quoted
Think of this as the surviving data structure after  the hv_driver
state is consolidated into (the existing) driver_context data
structure.  I did this in the spirit of doing one thing at a time. If
I am going to be picking a more appropriate name for the consolidated
data structure; I might as well pick the final name that we want this
unified driver abstraction to be called.
Your final name is fine, it's the intermediate one I'm objecting to.
Ok.
How about 'struct hv_driver_context' instead?
quoted
quoted
quoted
quoted
I realize that you are hopefully going to later rename this to something
else, but remember, a few patches back you thought that the "ctx" name
wasn't nice.  And here you go resuscitating it from the graveyard of
pointy bits.
As I noted in a different email, may be the granularity I chose in breaking up
these patches is causing all this confusion.
Yes, as I think you need to go much finer as you were doing more than
one thing in these patches, and not describing them properly at all.

Please try to redo them in a simpler manner, probably breaking it into
more steps, so we can properly review them.
Based on your comments on intermediate names, would you recommend that
as part  of consolidating the driver abstractions, I also rename this combined
state.
Probably, if I understand what you are referring to.  Please post code
so that I really know what you are doing :)

thanks,

greg k-h
Regards,

K. Y
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help