[PATCH 0000/0003] drivers: hv

[PATCH 0000/0003] drivers: hv

[K. Y. Srinivasan]

This patch-set further enhances the KVP functionality for Linux
guests:

	1. Supports most of the Win8 KVP protocol.
	2. Supports operations on all the pools.

This is a re-send of the series sent on March10. I have made 
changes to address comments Dan Carpenter had posted.

Regards,

K. Y

[PATCH 0000/0003] drivers: hv

[K. Y. Srinivasan]

This patch-set further enhances the KVP functionality for Linux
guests:

	1. Supports most of the Win8 KVP protocol.
	2. Supports operations on all the pools.

This is a re-send of the series sent on March10. I have made 
changes to address comments Dan Carpenter had posted.

Regards,

K. Y

[PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[K. Y. Srinivasan]

Support the newly defined KVP message types. It turns out that the host
pushes a set of standard key value pairs as soon as the guest opens the KVP channel.
Since we cannot handle these tuples until the user level daemon loads up, defer
reading the KVP channel until the user level daemon is launched.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/hv_kvp.c      |  200 +++++++++++++++++++++++++++++++++++-----------
 include/linux/hyperv.h   |    2 +
 tools/hv/hv_kvp_daemon.c |    7 ++
 3 files changed, 162 insertions(+), 47 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 779109b..3485dee 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -42,9 +42,10 @@
 static struct {
 	bool active; /* transaction status - active or not */
 	int recv_len; /* number of bytes received. */
-	int index; /* current index */
+	struct hv_kvp_msg  *kvp_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
+	void *kvp_context; /* for the channel callback */
 } kvp_transaction;
 
 static void kvp_send_key(struct work_struct *dummy);
@@ -110,12 +111,15 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 	struct hv_kvp_msg_enumerate *data;
 
 	message = (struct hv_kvp_msg *)msg->data;
-	if (message->kvp_hdr.operation == KVP_OP_REGISTER) {
+	switch (message->kvp_hdr.operation) {
+	case KVP_OP_REGISTER:
 		pr_info("KVP: user-mode registering done.\n");
 		kvp_register();
-	}
+		kvp_transaction.active = false;
+		hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+		break;
 
-	if (message->kvp_hdr.operation == KVP_OP_ENUMERATE) {
+	default:
 		data = &message->body.kvp_enum_data;
 		/*
 		 * Complete the transaction by forwarding the key value
@@ -133,21 +137,104 @@ kvp_send_key(struct work_struct *dummy)
 {
 	struct cn_msg *msg;
 	struct hv_kvp_msg *message;
-	int index = kvp_transaction.index;
+	struct hv_kvp_msg *in_msg;
+	__u8 operation = kvp_transaction.kvp_msg->kvp_hdr.operation;
+	__u8 pool = kvp_transaction.kvp_msg->kvp_hdr.pool;
+	__u32 val32;
+	__u64 val64;
 
 	msg = kzalloc(sizeof(*msg) + sizeof(struct hv_kvp_msg) , GFP_ATOMIC);
+	if (!msg)
+		return;
 
-	if (msg) {
-		msg->id.idx =  CN_KVP_IDX;
-		msg->id.val = CN_KVP_VAL;
+	msg->id.idx =  CN_KVP_IDX;
+	msg->id.val = CN_KVP_VAL;
 
-		message = (struct hv_kvp_msg *)msg->data;
-		message->kvp_hdr.operation = KVP_OP_ENUMERATE;
-		message->body.kvp_enum_data.index = index;
-		msg->len = sizeof(struct hv_kvp_msg);
-		cn_netlink_send(msg, 0, GFP_ATOMIC);
-		kfree(msg);
+	message = (struct hv_kvp_msg *)msg->data;
+	message->kvp_hdr.operation = operation;
+	message->kvp_hdr.pool = pool;
+	in_msg = kvp_transaction.kvp_msg;
+
+	/*
+	 * The key/value strings sent from the host are encoded in
+	 * in utf16; convert it to utf8 strings.
+	 * The host assures us that the utf16 strings will not exceed
+	 * the max lengths specified. We will however, reserve room
+	 * for the string terminating character - in the utf16s_utf8s()
+	 * function we limit the size of the buffer where the converted
+	 * string is placed to HV_KVP_EXCHANGE_MAX_*_SIZE -1 to gaurantee
+	 * that the strings can be properly terminated!
+	 */
+
+	switch (message->kvp_hdr.operation) {
+	case KVP_OP_SET:
+		switch (in_msg->body.kvp_set.data.value_type) {
+		case REG_SZ:
+			/*
+			 * The value is a string - utf16 encoding.
+			 */
+			message->body.kvp_set.data.value_size =
+				utf16s_to_utf8s(
+				(wchar_t *)in_msg->body.kvp_set.data.value,
+				in_msg->body.kvp_set.data.value_size,
+				UTF16_LITTLE_ENDIAN,
+				message->body.kvp_set.data.value,
+				HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1) + 1;
+				break;
+
+		case REG_U32:
+			/*
+			 * The value is a 32 bit scalar.
+			 * We save this as a utf8 string.
+			 */
+			val32 = in_msg->body.kvp_set.data.value_u32;
+			message->body.kvp_set.data.value_size =
+				sprintf(message->body.kvp_set.data.value,
+					"%d", val32) + 1;
+			break;
+
+		case REG_U64:
+			/*
+			 * The value is a 64 bit scalar.
+			 * We save this as a utf8 string.
+			 */
+			val64 = in_msg->body.kvp_set.data.value_u64;
+			message->body.kvp_set.data.value_size =
+				sprintf(message->body.kvp_set.data.value,
+					"%llu", val64) + 1;
+			break;
+
+		}
+	case KVP_OP_GET:
+		message->body.kvp_set.data.key_size =
+			utf16s_to_utf8s(
+			(wchar_t *)in_msg->body.kvp_set.data.key,
+			in_msg->body.kvp_set.data.key_size,
+			UTF16_LITTLE_ENDIAN,
+			message->body.kvp_set.data.key,
+			HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
+			break;
+
+	case KVP_OP_DELETE:
+		message->body.kvp_delete.key_size =
+			utf16s_to_utf8s(
+			(wchar_t *)in_msg->body.kvp_delete.key,
+			in_msg->body.kvp_delete.key_size,
+			UTF16_LITTLE_ENDIAN,
+			message->body.kvp_delete.key,
+			HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
+			break;
+
+	case KVP_OP_ENUMERATE:
+		message->body.kvp_enum_data.index =
+			in_msg->body.kvp_enum_data.index;
+			break;
 	}
+
+	msg->len = sizeof(struct hv_kvp_msg);
+	cn_netlink_send(msg, 0, GFP_ATOMIC);
+	kfree(msg);
+
 	return;
 }
 
@@ -159,7 +246,7 @@ static void
 kvp_respond_to_host(char *key, char *value, int error)
 {
 	struct hv_kvp_msg  *kvp_msg;
-	struct hv_kvp_msg_enumerate  *kvp_data;
+	struct hv_kvp_exchg_msg_value  *kvp_data;
 	char	*key_name;
 	struct icmsg_hdr *icmsghdrp;
 	int	keylen, valuelen;
@@ -189,6 +276,9 @@ kvp_respond_to_host(char *key, char *value, int error)
 
 	kvp_transaction.active = false;
 
+	icmsghdrp = (struct icmsg_hdr *)
+			&recv_buffer[sizeof(struct vmbuspipe_hdr)];
+
 	if (channel->onchannel_callback == NULL)
 		/*
 		 * We have raced with util driver being unloaded;
@@ -196,41 +286,57 @@ kvp_respond_to_host(char *key, char *value, int error)
 		 */
 		return;
 
-	icmsghdrp = (struct icmsg_hdr *)
-			&recv_buffer[sizeof(struct vmbuspipe_hdr)];
-	kvp_msg = (struct hv_kvp_msg *)
-			&recv_buffer[sizeof(struct vmbuspipe_hdr) +
-			sizeof(struct icmsg_hdr)];
-	kvp_data = &kvp_msg->body.kvp_enum_data;
-	key_name = key;
 
 	/*
 	 * If the error parameter is set, terminate the host's enumeration.
 	 */
 	if (error) {
 		/*
-		 * We don't support this index or the we have timedout;
+		 * Something failed or the we have timedout;
 		 * terminate the host-side iteration by returning an error.
 		 */
 		icmsghdrp->status = HV_E_FAIL;
 		goto response_done;
 	}
 
+	icmsghdrp->status = HV_S_OK;
+
+	kvp_msg = (struct hv_kvp_msg *)
+			&recv_buffer[sizeof(struct vmbuspipe_hdr) +
+			sizeof(struct icmsg_hdr)];
+
+	switch (kvp_transaction.kvp_msg->kvp_hdr.operation) {
+	case KVP_OP_GET:
+		kvp_data = &kvp_msg->body.kvp_get.data;
+		goto copy_value;
+
+	case KVP_OP_SET:
+	case KVP_OP_DELETE:
+		goto response_done;
+
+	default:
+		break;
+	}
+
+	kvp_data = &kvp_msg->body.kvp_enum_data.data;
+	key_name = key;
+
 	/*
 	 * The windows host expects the key/value pair to be encoded
 	 * in utf16.
 	 */
 	keylen = utf8s_to_utf16s(key_name, strlen(key_name), UTF16_HOST_ENDIAN,
-				(wchar_t *) kvp_data->data.key,
+				(wchar_t *) kvp_data->key,
 				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
-	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
+	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
+
+copy_value:
 	valuelen = utf8s_to_utf16s(value, strlen(value), UTF16_HOST_ENDIAN,
-				(wchar_t *) kvp_data->data.value,
+				(wchar_t *) kvp_data->value,
 				HV_KVP_EXCHANGE_MAX_VALUE_SIZE / 2);
-	kvp_data->data.value_size = 2*(valuelen + 1); /* utf16 encoding */
+	kvp_data->value_size = 2*(valuelen + 1); /* utf16 encoding */
 
-	kvp_data->data.value_type = REG_SZ; /* all our values are strings */
-	icmsghdrp->status = HV_S_OK;
+	kvp_data->value_type = REG_SZ; /* all our values are strings */
 
 response_done:
 	icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
@@ -257,11 +363,18 @@ void hv_kvp_onchannelcallback(void *context)
 	u64 requestid;
 
 	struct hv_kvp_msg *kvp_msg;
-	struct hv_kvp_msg_enumerate *kvp_data;
 
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
+	if (kvp_transaction.active) {
+		/*
+		 * We will defer processing this callback once
+		 * the current transaction is complete.
+		 */
+		kvp_transaction.kvp_context = context;
+		return;
+	}
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE, &recvlen, &requestid);
 
@@ -276,29 +389,16 @@ void hv_kvp_onchannelcallback(void *context)
 				sizeof(struct vmbuspipe_hdr) +
 				sizeof(struct icmsg_hdr)];
 
-			kvp_data = &kvp_msg->body.kvp_enum_data;
-
-			/*
-			 * We only support the "get" operation on
-			 * "KVP_POOL_AUTO" pool.
-			 */
-
-			if ((kvp_msg->kvp_hdr.pool != KVP_POOL_AUTO) ||
-				(kvp_msg->kvp_hdr.operation !=
-				KVP_OP_ENUMERATE)) {
-				icmsghdrp->status = HV_E_FAIL;
-				goto callback_done;
-			}
-
 			/*
 			 * Stash away this global state for completing the
 			 * transaction; note transactions are serialized.
 			 */
+
 			kvp_transaction.recv_len = recvlen;
 			kvp_transaction.recv_channel = channel;
 			kvp_transaction.recv_req_id = requestid;
 			kvp_transaction.active = true;
-			kvp_transaction.index = kvp_data->index;
+			kvp_transaction.kvp_msg = kvp_msg;
 
 			/*
 			 * Get the information from the
@@ -316,8 +416,6 @@ void hv_kvp_onchannelcallback(void *context)
 
 		}
 
-callback_done:
-
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
@@ -338,6 +436,14 @@ hv_kvp_init(struct hv_util_service *srv)
 		return err;
 	recv_buffer = srv->recv_buffer;
 
+	/*
+	 * When this driver loads, the user level daemon that
+	 * processes the host requests may not yet be running.
+	 * Defer processing channel callbacks until the daemon
+	 * has registered.
+	 */
+	kvp_transaction.active = true;
+
 	return 0;
 }
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index a2d8c54..e88a979 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -119,6 +119,8 @@
  */
 
 #define REG_SZ 1
+#define REG_U32 4
+#define REG_U64 8
 
 enum hv_kvp_exchg_op {
 	KVP_OP_GET = 0,
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 00d3f7c..a98878c 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -389,10 +389,16 @@ int main(void)
 			}
 			continue;
 
+		case KVP_OP_SET:
+		case KVP_OP_GET:
+		case KVP_OP_DELETE:
 		default:
 			break;
 		}
 
+		if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
+			goto kvp_done;
+
 		hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
 		key_name = (char *)hv_msg->body.kvp_enum_data.data.key;
 		key_value = (char *)hv_msg->body.kvp_enum_data.data.value;
@@ -454,6 +460,7 @@ int main(void)
 		 * already in the receive buffer. Update the cn_msg header to
 		 * reflect the key value that has been added to the message
 		 */
+kvp_done:
 
 		incoming_cn_msg->id.idx = CN_KVP_IDX;
 		incoming_cn_msg->id.val = CN_KVP_VAL;
-- 
1.7.4.1

[PATCH 2/3] Tools: hv: Fully support the new KVP verbs in the user level daemon

[K. Y. Srinivasan]

Now fully support the new KVP messages in the user level daemon. Hyper-V defines
multiple persistent pools to which the host can write/read/modify KVP tuples.
In this patch we implement a file for each specified pool, where the KVP tuples
will be stored in the guest.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 tools/hv/hv_kvp_daemon.c |  281 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 280 insertions(+), 1 deletions(-)

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index a98878c..2fb9c3d 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -39,7 +39,8 @@
 #include <ifaddrs.h>
 #include <netdb.h>
 #include <syslog.h>
-
+#include <sys/stat.h>
+#include <fcntl.h>
 
 /*
  * KVP protocol: The user mode component first registers with the
@@ -79,6 +80,250 @@ static char *os_build;
 static char *lic_version;
 static struct utsname uts_buf;
 
+
+#define MAX_FILE_NAME 100
+#define ENTRIES_PER_BLOCK 50
+
+struct kvp_record {
+	__u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+	__u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+};
+
+struct kvp_file_state {
+	int fd;
+	int num_blocks;
+	struct kvp_record *records;
+	int num_records;
+	__u8 fname[MAX_FILE_NAME];
+};
+
+static struct kvp_file_state kvp_file_info[KVP_POOL_COUNT];
+
+static void kvp_acquire_lock(int pool)
+{
+	struct flock fl = {F_WRLCK, SEEK_SET, 0, 0, 0};
+	fl.l_pid = getpid();
+
+	if (fcntl(kvp_file_info[pool].fd, F_SETLKW, &fl) == -1) {
+		syslog(LOG_ERR, "Failed to acquire the lock pool: %d", pool);
+		exit(-1);
+	}
+}
+
+static void kvp_release_lock(int pool)
+{
+	struct flock fl = {F_UNLCK, SEEK_SET, 0, 0, 0};
+	fl.l_pid = getpid();
+
+	if (fcntl(kvp_file_info[pool].fd, F_SETLK, &fl) == -1) {
+		perror("fcntl");
+		syslog(LOG_ERR, "Failed to release the lock pool: %d", pool);
+		exit(-1);
+	}
+}
+
+static void kvp_update_file(int pool)
+{
+	FILE *filep;
+	size_t bytes_written;
+
+	/*
+	 * We are going to write our in-memory registry out to
+	 * disk; acquire the lock first.
+	 */
+	kvp_acquire_lock(pool);
+
+	filep = fopen(kvp_file_info[pool].fname, "w");
+	if (!filep) {
+		kvp_release_lock(pool);
+		syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+		exit(-1);
+	}
+
+	bytes_written = fwrite(kvp_file_info[pool].records,
+				sizeof(struct kvp_record),
+				kvp_file_info[pool].num_records, filep);
+
+	fflush(filep);
+	kvp_release_lock(pool);
+}
+
+static int kvp_file_init(void)
+{
+	int ret, fd;
+	FILE *filep;
+	size_t records_read;
+	__u8 *fname;
+	struct kvp_record *record;
+	struct kvp_record *readp;
+	int num_blocks;
+	int i;
+	int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+	if (access("/var/opt/hyperv", F_OK)) {
+		if (mkdir("/var/opt/hyperv", S_IRUSR | S_IWUSR | S_IROTH)) {
+			syslog(LOG_ERR, " Failed to create /var/opt/hyperv");
+			exit(-1);
+		}
+	}
+
+	for (i = 0; i < KVP_POOL_COUNT; i++) {
+		fname = kvp_file_info[i].fname;
+		records_read = 0;
+		num_blocks = 1;
+		sprintf(fname, "/var/opt/hyperv/.kvp_pool_%d", i);
+		fd = open(fname, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR | S_IROTH);
+
+		if (fd == -1)
+			return 1;
+
+
+		filep = fopen(fname, "r");
+		if (!filep)
+			return 1;
+
+		record = malloc(alloc_unit * num_blocks);
+		if (record == NULL) {
+			fclose(filep);
+			return 1;
+		}
+		while (!feof(filep)) {
+			readp = &record[records_read];
+			records_read += fread(readp, sizeof(struct kvp_record),
+					ENTRIES_PER_BLOCK,
+					filep);
+
+			if (!feof(filep)) {
+				/*
+				 * We have more data to read.
+				 */
+				num_blocks++;
+				record = realloc(record, alloc_unit *
+						num_blocks);
+				if (record == NULL) {
+					fclose(filep);
+					return 1;
+				}
+				continue;
+			}
+			break;
+		}
+		kvp_file_info[i].fd = fd;
+		kvp_file_info[i].num_blocks = num_blocks;
+		kvp_file_info[i].records = record;
+		kvp_file_info[i].num_records = records_read;
+		fclose(filep);
+
+	}
+
+	return 0;
+}
+
+static int kvp_key_delete(int pool, __u8 *key, int key_size)
+{
+	int i;
+	int j, k;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just move the remaining
+		 * entries up.
+		 */
+		if (i == num_records) {
+			kvp_file_info[pool].num_records--;
+			kvp_update_file(pool);
+			return 0;
+		}
+
+		j = i;
+		k = j + 1;
+		for (; k < num_records; k++) {
+			strcpy(record[j].key, record[k].key);
+			strcpy(record[j].value, record[k].value);
+			j++;
+		}
+
+		kvp_file_info[pool].num_records--;
+		kvp_update_file(pool);
+		return 0;
+	}
+	return 1;
+}
+
+static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
+			int value_size)
+{
+	int i;
+	int j, k;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_blocks = kvp_file_info[pool].num_blocks;
+
+	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+		return 1;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just update the value -
+		 * this is the modify case.
+		 */
+		memcpy(record[i].value, value, value_size);
+		kvp_update_file(pool);
+		return 0;
+	}
+
+	/*
+	 * Need to add a new entry;
+	 */
+	if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
+		/* Need to allocate a larger array for reg entries. */
+		record = realloc(record, sizeof(struct kvp_record) *
+			 ENTRIES_PER_BLOCK * (num_blocks + 1));
+
+		if (record == NULL)
+			return 1;
+		kvp_file_info[pool].num_blocks++;
+
+	}
+	memcpy(record[i].value, value, value_size);
+	memcpy(record[i].key, key, key_size);
+	kvp_file_info[pool].records = record;
+	kvp_file_info[pool].num_records++;
+	kvp_update_file(pool);
+	return 0;
+}
+
+static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
+			int value_size)
+{
+	int i;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+
+	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+		return 1;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just copy the value out.
+		 */
+		memcpy(value, record[i].value, value_size);
+		return 0;
+	}
+
+	return 1;
+}
+
 void kvp_get_os_info(void)
 {
 	FILE	*file;
@@ -315,6 +560,11 @@ int main(void)
 	 */
 	kvp_get_os_info();
 
+	if (kvp_file_init()) {
+		syslog(LOG_ERR, "Failed to initialize the pools");
+		exit(-1);
+	}
+
 	fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
 	if (fd < 0) {
 		syslog(LOG_ERR, "netlink socket creation failed; error:%d", fd);
@@ -389,9 +639,38 @@ int main(void)
 			}
 			continue;
 
+		/*
+		 * The current protocol with the kernel component uses a
+		 * NULL key name to pass an error condition.
+		 * For the SET, GET and DELETE operations,
+		 * use the existing protocol to pass back error.
+		 */
+
 		case KVP_OP_SET:
+			if (kvp_key_add_or_modify(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_set.data.key,
+					hv_msg->body.kvp_set.data.key_size,
+					hv_msg->body.kvp_set.data.value,
+					hv_msg->body.kvp_set.data.value_size))
+				strcpy(hv_msg->body.kvp_set.data.key, "");
+			break;
+
 		case KVP_OP_GET:
+			if (kvp_get_value(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_set.data.key,
+					hv_msg->body.kvp_set.data.key_size,
+					hv_msg->body.kvp_set.data.value,
+					hv_msg->body.kvp_set.data.value_size))
+				strcpy(hv_msg->body.kvp_set.data.key, "");
+			break;
+
 		case KVP_OP_DELETE:
+			if (kvp_key_delete(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_delete.key,
+					hv_msg->body.kvp_delete.key_size))
+				strcpy(hv_msg->body.kvp_delete.key, "");
+			break;
+
 		default:
 			break;
 		}
-- 
1.7.4.1

[PATCH 2/3] Tools: hv: Fully support the new KVP verbs in the user level daemon

[K. Y. Srinivasan]

Now fully support the new KVP messages in the user level daemon. Hyper-V defines
multiple persistent pools to which the host can write/read/modify KVP tuples.
In this patch we implement a file for each specified pool, where the KVP tuples
will be stored in the guest.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 tools/hv/hv_kvp_daemon.c |  281 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 280 insertions(+), 1 deletions(-)

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index a98878c..2fb9c3d 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -39,7 +39,8 @@
 #include <ifaddrs.h>
 #include <netdb.h>
 #include <syslog.h>
-
+#include <sys/stat.h>
+#include <fcntl.h>
 
 /*
  * KVP protocol: The user mode component first registers with the
@@ -79,6 +80,250 @@ static char *os_build;
 static char *lic_version;
 static struct utsname uts_buf;
 
+
+#define MAX_FILE_NAME 100
+#define ENTRIES_PER_BLOCK 50
+
+struct kvp_record {
+	__u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+	__u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+};
+
+struct kvp_file_state {
+	int fd;
+	int num_blocks;
+	struct kvp_record *records;
+	int num_records;
+	__u8 fname[MAX_FILE_NAME];
+};
+
+static struct kvp_file_state kvp_file_info[KVP_POOL_COUNT];
+
+static void kvp_acquire_lock(int pool)
+{
+	struct flock fl = {F_WRLCK, SEEK_SET, 0, 0, 0};
+	fl.l_pid = getpid();
+
+	if (fcntl(kvp_file_info[pool].fd, F_SETLKW, &fl) == -1) {
+		syslog(LOG_ERR, "Failed to acquire the lock pool: %d", pool);
+		exit(-1);
+	}
+}
+
+static void kvp_release_lock(int pool)
+{
+	struct flock fl = {F_UNLCK, SEEK_SET, 0, 0, 0};
+	fl.l_pid = getpid();
+
+	if (fcntl(kvp_file_info[pool].fd, F_SETLK, &fl) == -1) {
+		perror("fcntl");
+		syslog(LOG_ERR, "Failed to release the lock pool: %d", pool);
+		exit(-1);
+	}
+}
+
+static void kvp_update_file(int pool)
+{
+	FILE *filep;
+	size_t bytes_written;
+
+	/*
+	 * We are going to write our in-memory registry out to
+	 * disk; acquire the lock first.
+	 */
+	kvp_acquire_lock(pool);
+
+	filep = fopen(kvp_file_info[pool].fname, "w");
+	if (!filep) {
+		kvp_release_lock(pool);
+		syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+		exit(-1);
+	}
+
+	bytes_written = fwrite(kvp_file_info[pool].records,
+				sizeof(struct kvp_record),
+				kvp_file_info[pool].num_records, filep);
+
+	fflush(filep);
+	kvp_release_lock(pool);
+}
+
+static int kvp_file_init(void)
+{
+	int ret, fd;
+	FILE *filep;
+	size_t records_read;
+	__u8 *fname;
+	struct kvp_record *record;
+	struct kvp_record *readp;
+	int num_blocks;
+	int i;
+	int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+	if (access("/var/opt/hyperv", F_OK)) {
+		if (mkdir("/var/opt/hyperv", S_IRUSR | S_IWUSR | S_IROTH)) {
+			syslog(LOG_ERR, " Failed to create /var/opt/hyperv");
+			exit(-1);
+		}
+	}
+
+	for (i = 0; i < KVP_POOL_COUNT; i++) {
+		fname = kvp_file_info[i].fname;
+		records_read = 0;
+		num_blocks = 1;
+		sprintf(fname, "/var/opt/hyperv/.kvp_pool_%d", i);
+		fd = open(fname, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR | S_IROTH);
+
+		if (fd == -1)
+			return 1;
+
+
+		filep = fopen(fname, "r");
+		if (!filep)
+			return 1;
+
+		record = malloc(alloc_unit * num_blocks);
+		if (record == NULL) {
+			fclose(filep);
+			return 1;
+		}
+		while (!feof(filep)) {
+			readp = &record[records_read];
+			records_read += fread(readp, sizeof(struct kvp_record),
+					ENTRIES_PER_BLOCK,
+					filep);
+
+			if (!feof(filep)) {
+				/*
+				 * We have more data to read.
+				 */
+				num_blocks++;
+				record = realloc(record, alloc_unit *
+						num_blocks);
+				if (record == NULL) {
+					fclose(filep);
+					return 1;
+				}
+				continue;
+			}
+			break;
+		}
+		kvp_file_info[i].fd = fd;
+		kvp_file_info[i].num_blocks = num_blocks;
+		kvp_file_info[i].records = record;
+		kvp_file_info[i].num_records = records_read;
+		fclose(filep);
+
+	}
+
+	return 0;
+}
+
+static int kvp_key_delete(int pool, __u8 *key, int key_size)
+{
+	int i;
+	int j, k;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just move the remaining
+		 * entries up.
+		 */
+		if (i == num_records) {
+			kvp_file_info[pool].num_records--;
+			kvp_update_file(pool);
+			return 0;
+		}
+
+		j = i;
+		k = j + 1;
+		for (; k < num_records; k++) {
+			strcpy(record[j].key, record[k].key);
+			strcpy(record[j].value, record[k].value);
+			j++;
+		}
+
+		kvp_file_info[pool].num_records--;
+		kvp_update_file(pool);
+		return 0;
+	}
+	return 1;
+}
+
+static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
+			int value_size)
+{
+	int i;
+	int j, k;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_blocks = kvp_file_info[pool].num_blocks;
+
+	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+		return 1;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just update the value -
+		 * this is the modify case.
+		 */
+		memcpy(record[i].value, value, value_size);
+		kvp_update_file(pool);
+		return 0;
+	}
+
+	/*
+	 * Need to add a new entry;
+	 */
+	if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
+		/* Need to allocate a larger array for reg entries. */
+		record = realloc(record, sizeof(struct kvp_record) *
+			 ENTRIES_PER_BLOCK * (num_blocks + 1));
+
+		if (record == NULL)
+			return 1;
+		kvp_file_info[pool].num_blocks++;
+
+	}
+	memcpy(record[i].value, value, value_size);
+	memcpy(record[i].key, key, key_size);
+	kvp_file_info[pool].records = record;
+	kvp_file_info[pool].num_records++;
+	kvp_update_file(pool);
+	return 0;
+}
+
+static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
+			int value_size)
+{
+	int i;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+
+	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+		return 1;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just copy the value out.
+		 */
+		memcpy(value, record[i].value, value_size);
+		return 0;
+	}
+
+	return 1;
+}
+
 void kvp_get_os_info(void)
 {
 	FILE	*file;
@@ -315,6 +560,11 @@ int main(void)
 	 */
 	kvp_get_os_info();
 
+	if (kvp_file_init()) {
+		syslog(LOG_ERR, "Failed to initialize the pools");
+		exit(-1);
+	}
+
 	fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
 	if (fd < 0) {
 		syslog(LOG_ERR, "netlink socket creation failed; error:%d", fd);
@@ -389,9 +639,38 @@ int main(void)
 			}
 			continue;
 
+		/*
+		 * The current protocol with the kernel component uses a
+		 * NULL key name to pass an error condition.
+		 * For the SET, GET and DELETE operations,
+		 * use the existing protocol to pass back error.
+		 */
+
 		case KVP_OP_SET:
+			if (kvp_key_add_or_modify(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_set.data.key,
+					hv_msg->body.kvp_set.data.key_size,
+					hv_msg->body.kvp_set.data.value,
+					hv_msg->body.kvp_set.data.value_size))
+				strcpy(hv_msg->body.kvp_set.data.key, "");
+			break;
+
 		case KVP_OP_GET:
+			if (kvp_get_value(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_set.data.key,
+					hv_msg->body.kvp_set.data.key_size,
+					hv_msg->body.kvp_set.data.value,
+					hv_msg->body.kvp_set.data.value_size))
+				strcpy(hv_msg->body.kvp_set.data.key, "");
+			break;
+
 		case KVP_OP_DELETE:
+			if (kvp_key_delete(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_delete.key,
+					hv_msg->body.kvp_delete.key_size))
+				strcpy(hv_msg->body.kvp_delete.key, "");
+			break;
+
 		default:
 			break;
 		}
-- 
1.7.4.1

[PATCH 3/3] Tools: hv: Support enumeration from all the pools

[K. Y. Srinivasan]

We have supported enumeration only from the AUTO pool. Now support
enumeration from all the available pools.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/hv_kvp.c      |    7 ++-
 include/linux/hyperv.h   |    1 +
 tools/hv/hv_kvp_daemon.c |  124 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 3485dee..843bfa3 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -288,14 +288,15 @@ kvp_respond_to_host(char *key, char *value, int error)
 
 
 	/*
-	 * If the error parameter is set, terminate the host's enumeration.
+	 * If the error parameter is set, terminate the host's enumeration
+	 * on this pool.
 	 */
 	if (error) {
 		/*
 		 * Something failed or the we have timedout;
-		 * terminate the host-side iteration by returning an error.
+		 * terminate the current  host-side iteration.
 		 */
-		icmsghdrp->status = HV_E_FAIL;
+		icmsghdrp->status = HV_S_CONT;
 		goto response_done;
 	}
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index e88a979..5852545 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -952,6 +952,7 @@ void vmbus_driver_unregister(struct hv_driver *hv_driver);
 
 #define HV_S_OK				0x00000000
 #define HV_E_FAIL			0x80004005
+#define HV_S_CONT			0x80070103
 #define HV_ERROR_NOT_SUPPORTED		0x80070032
 #define HV_ERROR_MACHINE_LOCKED		0x800704F7
 
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 2fb9c3d..146fd61 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -148,6 +148,51 @@ static void kvp_update_file(int pool)
 	kvp_release_lock(pool);
 }
 
+static void kvp_update_mem_state(int pool)
+{
+	FILE *filep;
+	size_t records_read = 0;
+	struct kvp_record *record = kvp_file_info[pool].records;
+	struct kvp_record *readp;
+	int num_blocks = kvp_file_info[pool].num_blocks;
+	int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+	kvp_acquire_lock(pool);
+
+	filep = fopen(kvp_file_info[pool].fname, "r");
+	if (!filep) {
+		kvp_release_lock(pool);
+		syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+		exit(-1);
+	}
+	while (!feof(filep)) {
+		readp = &record[records_read];
+		records_read += fread(readp, sizeof(struct kvp_record),
+					ENTRIES_PER_BLOCK * num_blocks,
+					filep);
+
+		if (!feof(filep)) {
+			/*
+			 * We have more data to read.
+			 */
+			num_blocks++;
+			record = realloc(record, alloc_unit * num_blocks);
+
+			if (record == NULL) {
+				syslog(LOG_ERR, "malloc failed");
+				exit(-1);
+			}
+			continue;
+		}
+		break;
+	}
+
+	kvp_file_info[pool].num_blocks = num_blocks;
+	kvp_file_info[pool].records = record;
+	kvp_file_info[pool].num_records = records_read;
+
+	kvp_release_lock(pool);
+}
 static int kvp_file_init(void)
 {
 	int ret, fd;
@@ -223,8 +268,16 @@ static int kvp_key_delete(int pool, __u8 *key, int key_size)
 {
 	int i;
 	int j, k;
-	int num_records = kvp_file_info[pool].num_records;
-	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_records;
+	struct kvp_record *record;
+
+	/*
+	 * First update the in-memory state.
+	 */
+	kvp_update_mem_state(pool);
+
+	num_records = kvp_file_info[pool].num_records;
+	record = kvp_file_info[pool].records;
 
 	for (i = 0; i < num_records; i++) {
 		if (memcmp(key, record[i].key, key_size))
@@ -259,14 +312,23 @@ static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
 {
 	int i;
 	int j, k;
-	int num_records = kvp_file_info[pool].num_records;
-	struct kvp_record *record = kvp_file_info[pool].records;
-	int num_blocks = kvp_file_info[pool].num_blocks;
+	int num_records;
+	struct kvp_record *record;
+	int num_blocks;
 
 	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
 		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
 		return 1;
 
+	/*
+	 * First update the in-memory state.
+	 */
+	kvp_update_mem_state(pool);
+
+	num_records = kvp_file_info[pool].num_records;
+	record = kvp_file_info[pool].records;
+	num_blocks = kvp_file_info[pool].num_blocks;
+
 	for (i = 0; i < num_records; i++) {
 		if (memcmp(key, record[i].key, key_size))
 			continue;
@@ -304,13 +366,21 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
 			int value_size)
 {
 	int i;
-	int num_records = kvp_file_info[pool].num_records;
-	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_records;
+	struct kvp_record *record;
 
 	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
 		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
 		return 1;
 
+	/*
+	 * First update the in-memory state.
+	 */
+	kvp_update_mem_state(pool);
+
+	num_records = kvp_file_info[pool].num_records;
+	record = kvp_file_info[pool].records;
+
 	for (i = 0; i < num_records; i++) {
 		if (memcmp(key, record[i].key, key_size))
 			continue;
@@ -324,6 +394,31 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
 	return 1;
 }
 
+static void kvp_pool_enumerate(int pool, int index, __u8 *key, int key_size,
+				__u8 *value, int value_size)
+{
+	struct kvp_record *record;
+
+	/*
+	 * First update our in-memory database.
+	 */
+	kvp_update_mem_state(pool);
+	record = kvp_file_info[pool].records;
+
+	if (index >= kvp_file_info[pool].num_records) {
+		/*
+		 * This is an invalid index; terminate enumeration;
+		 * - a NULL value will do the trick.
+		 */
+		strcpy(value, "");
+		return;
+	}
+
+	memcpy(key, record[index].key, key_size);
+	memcpy(value, record[index].value, value_size);
+}
+
+
 void kvp_get_os_info(void)
 {
 	FILE	*file;
@@ -678,6 +773,21 @@ int main(void)
 		if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
 			goto kvp_done;
 
+		/*
+		 * If the pool is KVP_POOL_AUTO, dynamically generate
+		 * both the key and the value; if not read from the
+		 * appropriate pool.
+		 */
+		if (hv_msg->kvp_hdr.pool != KVP_POOL_AUTO) {
+			kvp_pool_enumerate(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_enum_data.index,
+					hv_msg->body.kvp_enum_data.data.key,
+					HV_KVP_EXCHANGE_MAX_KEY_SIZE,
+					hv_msg->body.kvp_enum_data.data.value,
+					HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
+			goto kvp_done;
+		}
+
 		hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
 		key_name = (char *)hv_msg->body.kvp_enum_data.data.key;
 		key_value = (char *)hv_msg->body.kvp_enum_data.data.value;
-- 
1.7.4.1

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
>  	/*
>  	 * The windows host expects the key/value pair to be encoded
>  	 * in utf16.
>  	 */
>  	keylen = utf8s_to_utf16s(key_name, strlen(key_name), UTF16_HOST_ENDIAN,
> -				(wchar_t *) kvp_data->data.key,
> +				(wchar_t *) kvp_data->key,
>  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> +

I feel like a jerk for asking this, but is the output length correct
here?  It seems like we could go over again.  Also utf8s_to_utf16s()
can return negative error codes, why do we ignore those?

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 1:46 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> >  	/*
> >  	 * The windows host expects the key/value pair to be encoded
> >  	 * in utf16.
> >  	 */
> >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> UTF16_HOST_ENDIAN,
> > -				(wchar_t *) kvp_data->data.key,
> > +				(wchar_t *) kvp_data->key,
> >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > +
> 
> I feel like a jerk for asking this, but is the output length correct
> here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> can return negative error codes, why do we ignore those?

We are returning the strings back to the host here. There are checks elsewhere
in the code to ensure that all strings we return to the host can be accommodated
in the available space. For the most part these are strings that the host gave us in the 
first place that have already been validated.  Furthermore, there are checks on the 
host side to ensure that the returned size parameters are consistent with the protocol 
definitions for the key value pair. For instance let us say somehow we got into a 
situation where the converted utf16 string occupied the entire MAX sized array 
without any room for the terminating character and we set the length parameter 
to 2 more than the MAX value as this code would do. The host would simply discard the 
message as an illegal message. This would be more appropriate than sending a 
truncated key or value.

With regards to the negative values, negative values indicate a failure of some sort
in the conversion. Since the host is the recipient here, host will correctly deal with the
transaction by discarding the tuple.  

Regards,

K. Y 

RE: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 1:46 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> >  	/*
> >  	 * The windows host expects the key/value pair to be encoded
> >  	 * in utf16.
> >  	 */
> >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> UTF16_HOST_ENDIAN,
> > -				(wchar_t *) kvp_data->data.key,
> > +				(wchar_t *) kvp_data->key,
> >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > +
> 
> I feel like a jerk for asking this, but is the output length correct
> here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> can return negative error codes, why do we ignore those?

We are returning the strings back to the host here. There are checks elsewhere
in the code to ensure that all strings we return to the host can be accommodated
in the available space. For the most part these are strings that the host gave us in the 
first place that have already been validated.  Furthermore, there are checks on the 
host side to ensure that the returned size parameters are consistent with the protocol 
definitions for the key value pair. For instance let us say somehow we got into a 
situation where the converted utf16 string occupied the entire MAX sized array 
without any room for the terminating character and we set the length parameter 
to 2 more than the MAX value as this code would do. The host would simply discard the 
message as an illegal message. This would be more appropriate than sending a 
truncated key or value.

With regards to the negative values, negative values indicate a failure of some sort
in the conversion. Since the host is the recipient here, host will correctly deal with the
transaction by discarding the tuple.  

Regards,

K. Y 

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1.1: Type: text/plain, Size: 3069 bytes --]

On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Friday, March 16, 2012 1:46 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > Alan Stern
> > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > messages in the driver
> > 
> > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > >  	/*
> > >  	 * The windows host expects the key/value pair to be encoded
> > >  	 * in utf16.
> > >  	 */
> > >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > UTF16_HOST_ENDIAN,
> > > -				(wchar_t *) kvp_data->data.key,
> > > +				(wchar_t *) kvp_data->key,
> > >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +
> > 
> > I feel like a jerk for asking this, but is the output length correct
> > here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> > can return negative error codes, why do we ignore those?
> 
> We are returning the strings back to the host here. There are checks elsewhere
> in the code to ensure that all strings we return to the host can be accommodated
> in the available space. For the most part these are strings that the host gave us in the 
> first place that have already been validated.  Furthermore, there are checks on the 
> host side to ensure that the returned size parameters are consistent with the protocol 
> definitions for the key value pair. For instance let us say somehow we got into a 
> situation where the converted utf16 string occupied the entire MAX sized array 
> without any room for the terminating character and we set the length parameter 
> to 2 more than the MAX value as this code would do. The host would simply discard the 
> message as an illegal message. This would be more appropriate than sending a 
> truncated key or value.
> 

Uh...  Looking at it again, this code is clearly off by one.  If
we're not going to hit the limit, then we're not going to truncate,
so that's not a concern.  Let's just use the correct limit here.

The problem is that off-by-ones tend to reproduce by copy and paste.
It's best to never introduce any, even harmless ones.

Either that or add a comment.  /* Don't care about wrong limitter
because we trust the input. */.

> With regards to the negative values, negative values indicate a failure of some sort
> in the conversion. Since the host is the recipient here, host will correctly deal with the
> transaction by discarding the tuple.  

I'm not super familiar with this subsystem.  Where can I find code
for rejecting bad transactions?  It seems like an easy thing to
handle the error in both places.  It makes auditing the code a lot
simpler.

regards,
dan carpenter


[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Friday, March 16, 2012 1:46 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > Alan Stern
> > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > messages in the driver
> > 
> > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > >  	/*
> > >  	 * The windows host expects the key/value pair to be encoded
> > >  	 * in utf16.
> > >  	 */
> > >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > UTF16_HOST_ENDIAN,
> > > -				(wchar_t *) kvp_data->data.key,
> > > +				(wchar_t *) kvp_data->key,
> > >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +
> > 
> > I feel like a jerk for asking this, but is the output length correct
> > here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> > can return negative error codes, why do we ignore those?
> 
> We are returning the strings back to the host here. There are checks elsewhere
> in the code to ensure that all strings we return to the host can be accommodated
> in the available space. For the most part these are strings that the host gave us in the 
> first place that have already been validated.  Furthermore, there are checks on the 
> host side to ensure that the returned size parameters are consistent with the protocol 
> definitions for the key value pair. For instance let us say somehow we got into a 
> situation where the converted utf16 string occupied the entire MAX sized array 
> without any room for the terminating character and we set the length parameter 
> to 2 more than the MAX value as this code would do. The host would simply discard the 
> message as an illegal message. This would be more appropriate than sending a 
> truncated key or value.
> 

Uh...  Looking at it again, this code is clearly off by one.  If
we're not going to hit the limit, then we're not going to truncate,
so that's not a concern.  Let's just use the correct limit here.

The problem is that off-by-ones tend to reproduce by copy and paste.
It's best to never introduce any, even harmless ones.

Either that or add a comment.  /* Don't care about wrong limitter
because we trust the input. */.

> With regards to the negative values, negative values indicate a failure of some sort
> in the conversion. Since the host is the recipient here, host will correctly deal with the
> transaction by discarding the tuple.  

I'm not super familiar with this subsystem.  Where can I find code
for rejecting bad transactions?  It seems like an easy thing to
handle the error in both places.  It makes auditing the code a lot
simpler.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1.1: Type: text/plain, Size: 2599 bytes --]

On Fri, Mar 16, 2012 at 10:18:58AM +0300, Dan Carpenter wrote:
> > > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > >  	/*
> > > >  	 * The windows host expects the key/value pair to be encoded
> > > >  	 * in utf16.
> > > >  	 */
> > > >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > > UTF16_HOST_ENDIAN,
> > > > -				(wchar_t *) kvp_data->data.key,
> > > > +				(wchar_t *) kvp_data->key,
> > > >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +
> > > 
> > > I feel like a jerk for asking this, but is the output length correct
> > > here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> > > can return negative error codes, why do we ignore those?
> > 
> > We are returning the strings back to the host here. There are checks elsewhere
> > in the code to ensure that all strings we return to the host can be accommodated
> > in the available space. For the most part these are strings that the host gave us in the 
> > first place that have already been validated.  Furthermore, there are checks on the 
> > host side to ensure that the returned size parameters are consistent with the protocol 
> > definitions for the key value pair. For instance let us say somehow we got into a 
> > situation where the converted utf16 string occupied the entire MAX sized array 
> > without any room for the terminating character and we set the length parameter 
> > to 2 more than the MAX value as this code would do. The host would simply discard the 
> > message as an illegal message. This would be more appropriate than sending a 
> > truncated key or value.
> > 
> 
> Uh...  Looking at it again, this code is clearly off by one.  If
> we're not going to hit the limit, then we're not going to truncate,
> so that's not a concern.  Let's just use the correct limit here.
> 

Another option of course would be to add a test after the
conversion.

	if (keylen == HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2)
		return -EINVAL;

What I'm saying is that I audit a lot of code for buffer overflows,
and I don't want to see an off by one and then I have to audit where
the string come from and audit where it's going.

If it's corrupts memory then I fix the bug and I can list it under
my achievements in my weekly status report.  If it's wrong but it
doesn't corrupt memory, it's just a complete waste of my time and it
makes me really cross.

regards,
dan carpenter


[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]

On Fri, Mar 16, 2012 at 10:18:58AM +0300, Dan Carpenter wrote:
> > > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > >  	/*
> > > >  	 * The windows host expects the key/value pair to be encoded
> > > >  	 * in utf16.
> > > >  	 */
> > > >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > > UTF16_HOST_ENDIAN,
> > > > -				(wchar_t *) kvp_data->data.key,
> > > > +				(wchar_t *) kvp_data->key,
> > > >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +
> > > 
> > > I feel like a jerk for asking this, but is the output length correct
> > > here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> > > can return negative error codes, why do we ignore those?
> > 
> > We are returning the strings back to the host here. There are checks elsewhere
> > in the code to ensure that all strings we return to the host can be accommodated
> > in the available space. For the most part these are strings that the host gave us in the 
> > first place that have already been validated.  Furthermore, there are checks on the 
> > host side to ensure that the returned size parameters are consistent with the protocol 
> > definitions for the key value pair. For instance let us say somehow we got into a 
> > situation where the converted utf16 string occupied the entire MAX sized array 
> > without any room for the terminating character and we set the length parameter 
> > to 2 more than the MAX value as this code would do. The host would simply discard the 
> > message as an illegal message. This would be more appropriate than sending a 
> > truncated key or value.
> > 
> 
> Uh...  Looking at it again, this code is clearly off by one.  If
> we're not going to hit the limit, then we're not going to truncate,
> so that's not a concern.  Let's just use the correct limit here.
> 

Another option of course would be to add a test after the
conversion.

	if (keylen == HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2)
		return -EINVAL;

What I'm saying is that I audit a lot of code for buffer overflows,
and I don't want to see an off by one and then I have to audit where
the string come from and audit where it's going.

If it's corrupts memory then I fix the bug and I can list it under
my achievements in my weekly status report.  If it's wrong but it
doesn't corrupt memory, it's just a complete waste of my time and it
makes me really cross.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 3:19 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Friday, March 16, 2012 1:46 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > Alan Stern
> > > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > > messages in the driver
> > >
> > > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > >  	/*
> > > >  	 * The windows host expects the key/value pair to be encoded
> > > >  	 * in utf16.
> > > >  	 */
> > > >  	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > > UTF16_HOST_ENDIAN,
> > > > -				(wchar_t *) kvp_data->data.key,
> > > > +				(wchar_t *) kvp_data->key,
> > > >  				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > > -	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +
> > >
> > > I feel like a jerk for asking this, but is the output length correct
> > > here?  It seems like we could go over again.  Also utf8s_to_utf16s()
> > > can return negative error codes, why do we ignore those?
> >
> > We are returning the strings back to the host here. There are checks elsewhere
> > in the code to ensure that all strings we return to the host can be
> accommodated
> > in the available space. For the most part these are strings that the host gave us
> in the
> > first place that have already been validated.  Furthermore, there are checks on
> the
> > host side to ensure that the returned size parameters are consistent with the
> protocol
> > definitions for the key value pair. For instance let us say somehow we got into a
> > situation where the converted utf16 string occupied the entire MAX sized array
> > without any room for the terminating character and we set the length
> parameter
> > to 2 more than the MAX value as this code would do. The host would simply
> discard the
> > message as an illegal message. This would be more appropriate than sending a
> > truncated key or value.
> >
> 
> Uh...  Looking at it again, this code is clearly off by one.  If
> we're not going to hit the limit, then we're not going to truncate,
> so that's not a concern.  Let's just use the correct limit here.

Dan,
I am trying to understand your concern here:
You will agree with me that the current code does not overflow the 
buffer since the output is limited to MAX bytes and that is how 
big the output buffer is sized. Now, as I pointed out earlier, if the
string to be converted were to fully occupy the MAX bytes or even be
larger than MAX bytes (no buffer overflow here since we limit the
conversion to MAX bytes), we will get a malformed packet that will be
sent to the host since the size field of the message would exceed
the protocol specified size limit. I was merely pointing out that in
this case, I would rather have the host reject the message than send
a truncated Key/Value string (if we were to ever get invalid key or
value strings). 

> 
> The problem is that off-by-ones tend to reproduce by copy and paste.
> It's best to never introduce any, even harmless ones.
> 
> Either that or add a comment.  /* Don't care about wrong limitter
> because we trust the input. */.
> 
All I am saying if we have an invalid string we will (a) ensure that we don't overflow
the output buffer and (b) we will generate a malformed packet as we should since
the string we are handling is not valid. Furthermore, I am suggesting that it is better
to have a malformed packet in this case than to have 

> > With regards to the negative values, negative values indicate a failure of some
> sort
> > in the conversion. Since the host is the recipient here, host will correctly deal
> with the
> > transaction by discarding the tuple.
> 
> I'm not super familiar with this subsystem.  Where can I find code
> for rejecting bad transactions?  It seems like an easy thing to
> handle the error in both places.  It makes auditing the code a lot
> simpler.

The host here is a windows host and the protocol verification engine there
will reject a mal-formed message. Having said this, what you are 
proposing is reasonable, I will re-send the patches.

Regards,

K. Y

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1.1: Type: text/plain, Size: 1704 bytes --]

On Fri, Mar 16, 2012 at 01:14:07PM +0000, KY Srinivasan wrote:
> Dan,
> I am trying to understand your concern here:
> You will agree with me that the current code does not overflow the 
> buffer since the output is limited to MAX bytes and that is how 
> big the output buffer is sized. Now, as I pointed out earlier, if the
> string to be converted were to fully occupy the MAX bytes or even be
> larger than MAX bytes (no buffer overflow here since we limit the
> conversion to MAX bytes), we will get a malformed packet that will be
> sent to the host since the size field of the message would exceed
> the protocol specified size limit. I was merely pointing out that in
> this case, I would rather have the host reject the message than send
> a truncated Key/Value string (if we were to ever get invalid key or
> value strings). 
> 

Sending malformed packets is a bad idea.

Normally, if you handle the error as close to the cause of the error
as possible then it makes everything easier to read.  In this case
especially, it's so easy to catch any errors.  If you decide to go
ahead and send the malformed message, at least put a comment.

When I read code, I spend all my time looking for what it does
wrong.  So when code doesn't have any error handling and sets
keylen = -EINVAL then sure, it's fewer lines of code to read, but
it doesn't make my life easier.  Readable code has obvious error
handling.

Introducing off by one errors is an especially bad idea.  It doesn't
matter how harmless they are, because they end up getting copy and
pasted.  I don't know how to make it any clearer than that.  :/
Never never never do that!

regards,
dan carpenter


[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On Fri, Mar 16, 2012 at 01:14:07PM +0000, KY Srinivasan wrote:
> Dan,
> I am trying to understand your concern here:
> You will agree with me that the current code does not overflow the 
> buffer since the output is limited to MAX bytes and that is how 
> big the output buffer is sized. Now, as I pointed out earlier, if the
> string to be converted were to fully occupy the MAX bytes or even be
> larger than MAX bytes (no buffer overflow here since we limit the
> conversion to MAX bytes), we will get a malformed packet that will be
> sent to the host since the size field of the message would exceed
> the protocol specified size limit. I was merely pointing out that in
> this case, I would rather have the host reject the message than send
> a truncated Key/Value string (if we were to ever get invalid key or
> value strings). 
> 

Sending malformed packets is a bad idea.

Normally, if you handle the error as close to the cause of the error
as possible then it makes everything easier to read.  In this case
especially, it's so easy to catch any errors.  If you decide to go
ahead and send the malformed message, at least put a comment.

When I read code, I spend all my time looking for what it does
wrong.  So when code doesn't have any error handling and sets
keylen = -EINVAL then sure, it's fewer lines of code to read, but
it doesn't make my life easier.  Readable code has obvious error
handling.

Introducing off by one errors is an especially bad idea.  It doesn't
matter how harmless they are, because they end up getting copy and
pasted.  I don't know how to make it any clearer than that.  :/
Never never never do that!

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]