>>> On 11/11/2010 at 4:15 PM, in message <20101111211548.ga31...@kroah.com>,
>>> Greg
KH <g...@kroah.com> 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 <ksriniva...@novell.com>
>> + *
>> + * 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 (john...@2ka.mipt). 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
