lkml.org 
[lkml]   [2011]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
    Date


    > -----Original Message-----
    > From: Greg KH [mailto:greg@kroah.com]
    > Sent: Monday, February 28, 2011 9:44 PM
    > To: KY Srinivasan
    > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
    > devel@linuxdriverproject.org; virtualization@lists.osdl.org; 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@microsoft.com>
    > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
    >
    > Sweet, you cloned yourself, I thought only Alan Cox had achieved that
    > goal...
    >
    > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
    > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
    > >
    > > ---
    > > 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



    \
     
     \ /
      Last update: 2011-03-02 02:45    [W:0.030 / U:4.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site