RE: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Monday, February 28, 2011 9:44 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; Haiyang Zhang; Hank
> Janssen
> Subject: Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
> 
> On Fri, Feb 25, 2011 at 06:06:32PM -0800, K. Y. Srinivasan wrote:
> > Cleanup the names of variables that refer to the
> > hyperv_device abstraction.
> 
> Clean them up to be what?  Shorter?  Nice?  Full of rounded edges so
> that when we bump into them in the dark they don't poke us and cause us
> to shreak in pain?
> 
> >
> >
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> 
> Sweet, you cloned yourself, I thought only Alan Cox had achieved that
> goal...
> 
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > Signed-off-by: Hank Janssen <hjanssen@xxxxxxxxxxxxx>
> >
> > ---
> >  drivers/staging/hv/blkvsc_drv.c  |   12 ++--
> >  drivers/staging/hv/netvsc.c      |    4 +-
> >  drivers/staging/hv/netvsc_drv.c  |   36 ++++----
> >  drivers/staging/hv/storvsc_drv.c |   44 +++++-----
> >  drivers/staging/hv/vmbus_drv.c   |  164 +++++++++++++++++++-----------------
> --
> >  5 files changed, 130 insertions(+), 130 deletions(-)
> >
> > diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> > index 58ab0e8..305a665 100644
> > --- a/drivers/staging/hv/blkvsc_drv.c
> > +++ b/drivers/staging/hv/blkvsc_drv.c
> > @@ -95,7 +95,7 @@ struct blkvsc_request {
> >  /* Per device structure */
> >  struct block_device_context {
> >  	/* point back to our device context */
> > -	struct hyperv_device *device_ctx;
> > +	struct hyperv_device *device_obj;
> 
> Hey, I was right, it does have more rounded edges, nicely done.
> 
> 
> > -static int netvsc_device_add(struct hyperv_device *device,
> > -			void *additional_info);
> > +static int
> > +netvsc_device_add(struct hyperv_device *device, void *additional_info);
> 
> Again with the function return value hiding.  Please don't.
> 
> > --- a/drivers/staging/hv/storvsc_drv.c
> > +++ b/drivers/staging/hv/storvsc_drv.c
> > @@ -43,7 +43,7 @@ struct host_device_context {
> >  	/* must be 1st field
> >  	 * FIXME this is a bug */
> >  	/* point back to our device context */
> > -	struct hyperv_device *device_ctx;
> > +	struct hyperv_device *device_obj;
> 
> I really don't understand this change at all.  "obj" is just as vapid
> and clueless as "ctx" is, and it seems very gratuitous to change this.
> And I should know, I have made a lot of gratuitous renames in my time in
> the kernel...

Greg, there are not that many options here. As I recall there was universal
objection to the use of  *context/*ctx to refer to device or driver objects.
The name I chose is fairly descriptive of what it represents. If there is consensus 
on a better name, I will  use it. 

> 
> >  static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
> >  {
> > -	struct hyperv_device *device_ctx = device_to_hyperv_device(device);
> > +	struct hyperv_device *device_obj = device_to_hyperv_device(device);
> >  	int ret;
> >
> >  	DPRINT_INFO(VMBUS_DRV, "generating uevent -
> VMBUS_DEVICE_CLASS_GUID={"
> >  		    "%02x%02x%02x%02x-%02x%02x-%02x%02x-"
> >  		    "%02x%02x%02x%02x%02x%02x%02x%02x}",
> > -		    device_ctx->class_id.data[3], device_ctx->class_id.data[2],
> > -		    device_ctx->class_id.data[1], device_ctx->class_id.data[0],
> > -		    device_ctx->class_id.data[5], device_ctx->class_id.data[4],
> > -		    device_ctx->class_id.data[7], device_ctx->class_id.data[6],
> > -		    device_ctx->class_id.data[8], device_ctx->class_id.data[9],
> > -		    device_ctx->class_id.data[10],
> > -		    device_ctx->class_id.data[11],
> > -		    device_ctx->class_id.data[12],
> > -		    device_ctx->class_id.data[13],
> > -		    device_ctx->class_id.data[14],
> > -		    device_ctx->class_id.data[15]);
> > +		    device_obj->class_id.data[3], device_obj->class_id.data[2],
> > +		    device_obj->class_id.data[1], device_obj->class_id.data[0],
> > +		    device_obj->class_id.data[5], device_obj->class_id.data[4],
> > +		    device_obj->class_id.data[7], device_obj->class_id.data[6],
> > +		    device_obj->class_id.data[8], device_obj->class_id.data[9],
> > +		    device_obj->class_id.data[10],
> > +		    device_obj->class_id.data[11],
> > +		    device_obj->class_id.data[12],
> > +		    device_obj->class_id.data[13],
> > +		    device_obj->class_id.data[14],
> > +		    device_obj->class_id.data[15]);
> 
> After seeing this stuff so many times I'm waiting for a helper function
> to be added for it in this subsystem.  I'm sure you really don't want to
> edit GUID printk-like functions ever again, right?

I will have a separate patch for this.

> 
> >  static void vmbus_device_release(struct device *device)
> >  {
> > -	struct hyperv_device *device_ctx = device_to_hyperv_device(device);
> > +	struct hyperv_device *device_obj = device_to_hyperv_device(device);
> >
> > -	kfree(device_ctx);
> > +	kfree(device_obj);
> >
> > -	/* !!DO NOT REFERENCE device_ctx anymore at this point!! */
> > +	/* !!DO NOT REFERENCE device_obj anymore at this point!! */
> >  }
> 
> I think by virtue of the kfree() right above this comment, it should be
> deleted.  Especially as it is at the end of the function, making it
> pretty much impossible to make any sense here...

For what it is worth; this is existing code. In the spirit of one thing per patch;
there will be a patch for this.

> 
> Come on, global search-and-replace needs to be done in a sane manner,
> other wise you can just send me a vi macro to run on the code, it would
> be the same thing in the end (hint, don't do that, only one person has
> ever gotten away with doing that in the history of the kernel, in an act
> never to be ever repeated again.)

Greg, apart from your objection to the name I picked to refer to variable 
referring to struct hyperv_device; what else is the problem here. 

Regards,

K. Y 
 
> 
> thanks,
> 
> greg k-h

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux