> -----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
