>>> On 11/11/2010 at 4:15 PM, in message <20101111211548.GA31373@xxxxxxxxx>, Greg KH <greg@xxxxxxxxx> wrote: > On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote: >> +/* >> + * An implementation of key value pair (KVP) functionality for Linux. >> + * >> + * >> + * Copyright (C) 2010, Novell, Inc. >> + * Author : K. Y. Srinivasan <ksrinivasan@xxxxxxxxxx> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation. > > This is all that is needed. > >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or >> + * NON INFRINGEMENT. See the GNU General Public License for more >> + * details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. > > You can delete this chunk. Will do. > >> +/* >> + * KVP protocol: The user mode component first registers with the >> + * the kernel component. Subsequently, the kernel component requests, data >> + * for the specified keys. In response to this message the user mode > component >> + * fills in the value corresponding to the specified key. We overload the >> + * sequence field in the cn_msg header to define our KVP message types. >> + * >> + * XXXKYS: Have a shared header file between the user and kernel (TODO) >> + */ >> + >> +enum kvp_op { >> + KVP_REGISTER = 0, /* Register the user mode component */ >> + KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/ >> + KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/ >> + KVP_USER_GET, /*User is requesting the value for the specified key*/ >> + KVP_USER_SET /*User is providing the value for the specified key*/ >> +}; > > As these values are shared between the kernel and userspace, you should > specifically define them. > > Also, your spaces with the /* stuff is incorrect. > > And, "KVP"? That's very generic, how about, "HYPERV_KVP_..." instead? > That fits the global namespace much better. I will change the names; deal with the comments etc. > > s/kvp_op/hyperv_kvp_op/ as well. > >> +#define KVP_KEY_SIZE 512 >> +#define KVP_VALUE_SIZE 2048 >> + >> + >> +typedef struct kvp_msg { >> + __u32 kvp_key; /* Key */ >> + __u8 kvp_value[0]; /* Corresponding value */ >> +} kvp_msg_t; > > I thought that kvp_value was really KVP_VALUE_SIZE? kvp_value is typed information and KVP_VALUE_SIZE specifies the maximum size of the supported value. For instance if kvp_value is a string (which is the case for all the values currently supported), KVP_VALUE_SIZE specifies the maximum size of the string that will be supported. > > And no typedefs, you did run your code through checkpatch.pl, right? > Why ignore the stuff it spits back at you? I will fix this. > > >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName", >> + "IntegrationServicesVersion", >> + "NetworkAddressIPv4", >> + "NetworkAddressIPv6", >> + "OSBuildNumber", >> + "OSName", >> + "OSMajorVersion", >> + "OSMinorVersion", >> + "OSVersion", >> + "ProcessorArchitecture", >> + }; > > Why list these at all, as more might come in the future, and the kernel > really doesn't care about them, right? The core HyperV KVP protocol is such that the host iterates through an index (starting with 0); the guest is free to associate a key with the index and return the associated value. The host side iteration is stopped when the guest returns a failure on an index. MSFT has specified the keys and their ordinal value in their KVP specification. The array you see above is part of that specification. > >> +int >> +kvp_init(void) > > All of your global symbols should have "hyperv_" on the front of them. Will do. > >> --- linux.trees.git.orig/drivers/staging/hv/Makefile 2010-11-10 14:01:55.000000000 > -0500 >> +++ linux.trees.git/drivers/staging/hv/Makefile 2010-11-11 11:24:54.000000000 > -0500 >> @@ -2,7 +2,7 @@ obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_t >> obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o >> obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o >> obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o >> -obj-$(CONFIG_HYPERV_UTILS) += hv_utils.o >> +obj-$(CONFIG_HYPERV_UTILS) += hv_util.o > > Ick, you just renamed the kernel module. Did you really mean to do > this? What tools are now going to break because you did this? (I'm > thinking installers here...) Oops! I will fix that. > >> >> hv_vmbus-y := vmbus_drv.o osd.o \ >> vmbus.o hv.o connection.o channel.o \ >> @@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \ >> hv_storvsc-y := storvsc_drv.o storvsc.o >> hv_blkvsc-y := blkvsc_drv.o blkvsc.o >> hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o >> +hv_util-y := hv_utils.o kvp.o >> Index: linux.trees.git/drivers/staging/hv/kvp.h >> =================================================================== >> --- /dev/null 1970-01-01 00:00:00.000000000 +0000 >> +++ linux.trees.git/drivers/staging/hv/kvp.h 2010-11-10 14:03:47.000000000 -0500 > > hyperv_kvp.h as this is going to be a global header file, right? > >> +typedef enum { >> + ICKvpExchangeOperationGet = 0, >> + ICKvpExchangeOperationSet, >> + ICKvpExchangeOperationDelete, >> + ICKvpExchangeOperationEnumerate, >> + ICKvpExchangeOperationCount /* Number of operations, must be last. */ >> +} IC_KVP_EXCHANGE_OPERATION; >> + >> +typedef enum { >> + ICKvpExchangePoolExternal = 0, >> + ICKvpExchangePoolGuest, >> + ICKvpExchangePoolAuto, >> + ICKvpExchangePoolAutoExternal, >> + ICKvpExchangePoolInternal, >> + ICKvpExchangePoolCount /* Number of pools, must be last. */ >> +} IC_KVP_EXCHANGE_POOL; >> + >> +typedef struct ic_kvp_hdr { >> + u8 operation; >> + u8 pool; >> +} ic_kvp_hdr_t; >> + >> +typedef struct ic_kvp_exchg_msg_value { >> + u32 value_type; >> + u32 key_size; >> + u32 value_size; >> + u8 key[IC_KVP_EXCHANGE_MAX_KEY_SIZE]; >> + u8 value[IC_KVP_EXCHANGE_MAX_VALUE_SIZE]; >> +} ic_kvp_exchg_msg_value_t; >> + >> +typedef struct ic_kvp__msg_enumerate { >> + u32 index; >> + ic_kvp_exchg_msg_value_t data; >> +} ic_kvp_msg_enumerate_t; >> + >> +typedef struct ic_kvp_msg { >> + ic_kvp_hdr_t kvp_hdr; >> + ic_kvp_msg_enumerate_t kvp_data; >> +} ic_kvp_msg_t; > > Again, no typedefs, and fix up the names of these structures to be > understandable :) Will do. With regards to making them understandable, I will try! > >> --- linux.trees.git.orig/include/linux/connector.h 2010-11-09 17:22:15.000000000 > -0500 >> +++ linux.trees.git/include/linux/connector.h 2010-11-11 13:14:52.000000000 > -0500 >> @@ -42,8 +42,9 @@ >> #define CN_VAL_DM_USERSPACE_LOG 0x1 >> #define CN_IDX_DRBD 0x8 >> #define CN_VAL_DRBD 0x1 >> +#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */ > > Did you reserve this number with anyone? Who? I sent an email to Evgeniy Polyakov (Johnpol@xxxxxxxx). The mail bounced back. I was hoping you would help me register this index. Would it make sense to have a separate patch to deal with registering this index? > >> -#define CN_NETLINK_USERS 8 >> +#define CN_NETLINK_USERS 10 > > Are you sure you incremented this properly? Oops! I will fix this. > > thanks, > > greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel