Re: [PATCH]: An implementation of HyperV KVP functionality

[Greg KH <greg@kroah.com>]

On Fri, Nov 12, 2010 at 11:29:58AM -0700, Ky Srinivasan wrote:
> 
> 
> >>> On 11/11/2010 at  4:19 PM, in message <20101111211904.GB31373@kroah.com>, Greg
> KH <greg@kroah.com> wrote: 
> > On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
> >> +/*
> >> + * Array of keys we support in Linux.
> > 
> > Not really, you can support "any" number of keys as the kernel shouldn't
> > care, or did I get it wrong?
> We currently support only the keys that have been specified in the KVP specification. I have a more detailed response on the core KVP protocol in response to your other email on this topic.
> > 
> >> + *
> >> + */
> >> +#define KVP_MAX_KEY	10
> >> +#define KVP_LIC_VERSION 1
> > 
> > Um, this is a nice magic number, care to explain it a bit more?
> As I noted in an earlier email, the KVP specification currently requires that we support 10 keys and it also specifies the ordering of these keys.  The information for the key "IntegrationServicesVersion", is only available in the kernel (one of the other LIC drivers defines  this information). 
> 
> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> +				"IntegrationServicesVersion",
> > 
> > Looks like it matches up with this, right?  You might want to make that
> > a bit more "tied" together.
> > 
> Yes; I will fix this.
> >> +			case (KVP_LIC_VERSION):
> >> +				kvp_transaction_active = true;
> >> +				kvp_respond_to_host(kvp_data->index,
> >> +						HV_DRV_VERSION);
> > 
> > Why are you doing this in the kernel?  Why not do it from userspace like
> > all other messages?
> This information is only available in the kernel (defined by another LIC driver).

I thought it was exported through a modinfo parameter?  If not, you
could set the MODULE_VERSION field be the as this which then is exported
to userspace so you would not have to do any thing like this in the
kernel.

thanks,

greg k-h