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.
> +/*
> + * 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.
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?
And no typedefs, you did run your code through checkpatch.pl, right?
Why ignore the stuff it spits back at you?
> +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?
> +int
> +kvp_init(void)
All of your global symbols should have "hyperv_" on the front of them.
> --- 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...)
>
> 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 :)
> --- 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?
> -#define CN_NETLINK_USERS 8
> +#define CN_NETLINK_USERS 10
Are you sure you incremented this properly?
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
