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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6E21E5352C11B742B20C142EB499E04801661A@TK5EX14MBXC128.redmond.corp.microsoft.com>
Date:	Wed, 2 Mar 2011 01:43:13 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	Greg KH <greg@...ah.com>
CC:	"gregkh@...e.de" <gregkh@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"virtualization@...ts.osdl.org" <virtualization@...ts.osdl.org>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	"Hank Janssen" <hjanssen@...rosoft.com>
Subject: RE: [PATCH 4/6] Staging: hv:  Unify the hyperv driver abstractions



> -----Original Message-----
> From: Greg KH [mailto:greg@...ah.com]
> Sent: Monday, February 28, 2011 9:53 PM
> To: KY Srinivasan
> Cc: gregkh@...e.de; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; virtualization@...ts.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
> 
> On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote:
> > This patch combines the two driver abstractions into
> > a single driver abstraction.
> 
> Ah, how sweet.  Unfortunatly you don't say "how" you did this.
> 
> Nor do you describe _what_ those two driver abstractions were.  Are we
> talking i2c and usb abstractions?  gpio and spi?  Driver core and
> platform?  We want to know exactly what is going on here.

My mistake; I will have a more descriptive comment.

> 
> Think of writing something that when you look back, in 3 years, while
> staring at a Linux hyperv driver originally written for the 2.6.9
> kernel, that somehow never got forward ported and you are tasked with
> doing this, that you can just do a simple 'git log drivers/staging/hv/'
> and instantly know just from the changelog comments exactly what you
> need to do to your driver to clean it up and properly get it to work on
> the new 8.2.2 kernel release.
> 
> This changelog entry, would require you to go and dig through the guts
> of the patch itself, trying to figure out what abstractions you are
> talking about, and exactly how they were combined, all the while
> wondering _why_ they were combined.
> 
> Please, think of your future self, you will thank him in the years to
> come by doing this properly.  Not to mention making other's lives easier
> if you happen to have escaped this dire task by then.
> 
> Oh, you have an extra space up there in the subject, please fix it next
> time.
> 
> > -int blk_vsc_initialize(struct hv_driver *driver)
> > +int blk_vsc_initialize(struct driver_context *driver)
> 
> "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.
 
> 
> 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.
 
> 
> And what happens if your future patch is rejected?  You are stuck with a
> "driver_context" structure in a subsystem?  That's a pretty big abuse of
> the global namespace, don't you think?  It sounds like something that
> should go into include/linux/device.h
> 
> Please be careful about names, they mean things, even when you think
> they don't.

Greg, would it be better if I just had one patch for dealing with all device related issues and a
Separate patch for dealing all driver related issues?

Regards,

K. Y
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/