Re: [PATCH 00/17] drivers: hv: kvp

Re: [PATCH 00/17] drivers: hv: kvp

[Greg KH]

On Tue, Jul 24, 2012 at 09:01:12AM -0700, K. Y. Srinivasan wrote:
> This patchset expands the KVP (Key Value Pair) functionality to
> implement the mechanism to GET/SET IP addresses in the guest. This
> functionality is used in Windows Server 2012 to implement VM
> replication functionality. The way IP configuration information
> is managed is distro specific. Based on the feedback I have gotten
> from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
> distro specific code from this patch-set. Most of the GET operation
> can be implemented in a way that is completely distro independent and
> I have implemented that as such and is included in this patch-set.
> Some of the attributes that can only be fetched in a distro
> dependent way as well the mechanism for configuring an interface
> (the SET operation) that is clearly distro specific is to be
> implemented via external scripts that will be invoked via the KVP
> code. We define here the interface to these scripts.
> 
> Adding support for IP injection resulted in some changes to the
> protocol between the user level daemon and the kernel driver.
> These changes have been implemented in way that would retain
> compatibility with older daemons. I would like to thank Olaf and
> Greg for pointing out the compatibility issue.

Due to this being the middle of the merge window, I will not be able to
look at this until after 3.6-rc1 is out.

greg k-h

Re: [PATCH 09/17] Tools: hv: Represent the ipv6 mask using CIDR notation

[Borislav Petkov]

On Tue, Jul 24, 2012 at 09:01:33AM -0700, K. Y. Srinivasan wrote:
> Transform ipv6 subnet information to CIDR notation.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  tools/hv/hv_kvp_daemon.c |   45 +++++++++++++++++++++++++++++++++++----------
>  1 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 2c24ebf..007e698 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -491,6 +491,15 @@ done:
>  	return;
>  }
>  
> +static unsigned int hweight32(unsigned int *w)
> +{
> +	unsigned int res = *w - ((*w >> 1) & 0x55555555);
> +	res = (res & 0x33333333) + ((res >> 2) & 0x33333333);
> +	res = (res + (res >> 4)) & 0x0F0F0F0F;
> +	res = res + (res >> 8);
> +	return (res + (res >> 16)) & 0x000000FF;
> +}

What's wrong with the hweight32 version we have already in
<include/asm-generic/bitops/const_hweight.h> which you can include by
simply by including <asm-generic/bitops.h>?

-- 
Regards/Gruss,
Boris.

[PATCH 00/17] drivers: hv: kvp

[K. Y. Srinivasan]

This patchset expands the KVP (Key Value Pair) functionality to
implement the mechanism to GET/SET IP addresses in the guest. This
functionality is used in Windows Server 2012 to implement VM
replication functionality. The way IP configuration information
is managed is distro specific. Based on the feedback I have gotten
from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
distro specific code from this patch-set. Most of the GET operation
can be implemented in a way that is completely distro independent and
I have implemented that as such and is included in this patch-set.
Some of the attributes that can only be fetched in a distro
dependent way as well the mechanism for configuring an interface
(the SET operation) that is clearly distro specific is to be
implemented via external scripts that will be invoked via the KVP
code. We define here the interface to these scripts.

Adding support for IP injection resulted in some changes to the
protocol between the user level daemon and the kernel driver.
These changes have been implemented in way that would retain
compatibility with older daemons. I would like to thank Olaf and
Greg for pointing out the compatibility issue.

K. Y. Srinivasan (17):
  Drivers: hv: vmbus: Use the standard format string to format GUIDs
  Drivers: hv: Add KVP definitions for IP address injection
  Drivers: hv: kvp: Cleanup error handling in KVP
  Drivers: hv: kvp: Support the new IP injection messages
  Tools: hv: Prepare to expand  kvp_get_ip_address() functionality
  Tools: hv: Further refactor kvp_get_ip_address()
  Tools: hv: Gather address family information
  Tools: hv: Gather subnet information
  Tools: hv: Represent the ipv6 mask using CIDR notation
  Tools: hv: Gather ipv[4,6] gateway information
  Tools: hv: Gather DNS information
  Tools: hv: Gather DHCP information
  Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
  Tools: hv: Rename the function kvp_get_ip_address()
  Tools: hv: Implement the KVP verb - KVP_OP_GET_IP_INFO
  Tools: hv: Get rid of some unused variables
  Tools: hv: Correctly type string variables

 drivers/hv/hv_kvp.c      |  251 +++++++++++--
 drivers/hv/hv_util.c     |    4 +-
 drivers/hv/vmbus_drv.c   |   38 +--
 include/linux/hyperv.h   |   88 ++++-
 tools/hv/hv_kvp_daemon.c |  943 +++++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 1160 insertions(+), 164 deletions(-)

-- 
1.7.4.1

[PATCH 01/17] Drivers: hv: vmbus: Use the standard format string to format GUIDs

[K. Y. Srinivasan]

Format GUIDS as per MSFT standard. This makes interacting with MSFT
tool stack easier.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/vmbus_drv.c |   38 ++------------------------------------
 1 files changed, 2 insertions(+), 36 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4748086..b76e8b3 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -146,43 +146,9 @@ static ssize_t vmbus_show_device_attr(struct device *dev,
 	get_channel_info(hv_dev, device_info);
 
 	if (!strcmp(dev_attr->attr.name, "class_id")) {
-		ret = sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
-			       "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
-			       device_info->chn_type.b[3],
-			       device_info->chn_type.b[2],
-			       device_info->chn_type.b[1],
-			       device_info->chn_type.b[0],
-			       device_info->chn_type.b[5],
-			       device_info->chn_type.b[4],
-			       device_info->chn_type.b[7],
-			       device_info->chn_type.b[6],
-			       device_info->chn_type.b[8],
-			       device_info->chn_type.b[9],
-			       device_info->chn_type.b[10],
-			       device_info->chn_type.b[11],
-			       device_info->chn_type.b[12],
-			       device_info->chn_type.b[13],
-			       device_info->chn_type.b[14],
-			       device_info->chn_type.b[15]);
+		ret = sprintf(buf, "{%pUl}\n", device_info->chn_type.b);
 	} else if (!strcmp(dev_attr->attr.name, "device_id")) {
-		ret = sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
-			       "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
-			       device_info->chn_instance.b[3],
-			       device_info->chn_instance.b[2],
-			       device_info->chn_instance.b[1],
-			       device_info->chn_instance.b[0],
-			       device_info->chn_instance.b[5],
-			       device_info->chn_instance.b[4],
-			       device_info->chn_instance.b[7],
-			       device_info->chn_instance.b[6],
-			       device_info->chn_instance.b[8],
-			       device_info->chn_instance.b[9],
-			       device_info->chn_instance.b[10],
-			       device_info->chn_instance.b[11],
-			       device_info->chn_instance.b[12],
-			       device_info->chn_instance.b[13],
-			       device_info->chn_instance.b[14],
-			       device_info->chn_instance.b[15]);
+		ret = sprintf(buf, "{%pUl}\n", device_info->chn_instance.b);
 	} else if (!strcmp(dev_attr->attr.name, "modalias")) {
 		print_alias_name(hv_dev, alias_name);
 		ret = sprintf(buf, "vmbus:%s\n", alias_name);
-- 
1.7.4.1

[PATCH 02/17] Drivers: hv: Add KVP definitions for IP address injection

[K. Y. Srinivasan]

Add the necessary definitions for supporting the IP injection functionality.

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

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index d3ac6a4..a0667de 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
 		(struct hv_util_service *)dev_id->driver_data;
 	int ret;
 
-	srv->recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	srv->recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
 	if (!srv->recv_buffer)
 		return -ENOMEM;
 	if (srv->util_init) {
@@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
 		}
 	}
 
-	ret = vmbus_open(dev->channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL, 0,
+	ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0,
 			srv->util_cb, dev->channel);
 	if (ret)
 		goto error;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 68ed7f7..3c56ca7 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -122,12 +122,53 @@
 #define REG_U32 4
 #define REG_U64 8
 
+/*
+ * As we look at expanding the KVP functionality to include
+ * IP injection functionality, we need to maintain binary
+ * compatibility with older daemons.
+ *
+ * The KVP opcodes are defined by the host and it was unfortunate
+ * that I chose to treat the registration operation as part of the
+ * KVP operations defined by the host.
+ * Here is the level of compatibility
+ * (between the user level daemon and the kernel KVP driver) that we
+ * will implement:
+ *
+ * An older daemon will always be supported on a newer driver.
+ * A given user level daemon will require a minimal version of the
+ * kernel driver.
+ * If we cannot handle the version differences, we will fail gracefully
+ * (this can happen when we have a user level daemon that is more
+ * advanced than the KVP driver.
+ *
+ * We will use values used in this handshake for determining if we have
+ * workable user level daemon and the kernel driver. We begin by taking the
+ * registration opcode out of the KVP opcode namespace. We will however,
+ * maintain compatibility with the existing user-level daemon code.
+ */
+
+/*
+ * Daemon code not supporting IP injection (legacy daemon).
+ */
+
+#define KVP_OP_REGISTER	4
+
+/*
+ * Daemon code supporting IP injection.
+ * The KVP opcode field is used to communicate the
+ * registration information; so define a namespace that
+ * will be distinct from the host defined KVP opcode.
+ */
+
+#define KVP_OP_REGISTER1 100
+
 enum hv_kvp_exchg_op {
 	KVP_OP_GET = 0,
 	KVP_OP_SET,
 	KVP_OP_DELETE,
 	KVP_OP_ENUMERATE,
-	KVP_OP_REGISTER,
+	KVP_OP_GET_IP_INFO,
+	KVP_OP_SET_IP_INFO,
 	KVP_OP_COUNT /* Number of operations, must be last. */
 };
 
@@ -140,6 +181,26 @@ enum hv_kvp_exchg_pool {
 	KVP_POOL_COUNT /* Number of pools, must be last. */
 };
 
+#define ADDR_FAMILY_NONE	0x00
+#define ADDR_FAMILY_IPV4	0x01
+#define ADDR_FAMILY_IPV6	0x02
+
+#define MAX_ADAPTER_ID_SIZE	128
+#define MAX_IP_ADDR_SIZE	1024
+#define MAX_GATEWAY_SIZE	512
+
+
+struct hv_kvp_ipaddr_value {
+	__u16	adapter_id[MAX_ADAPTER_ID_SIZE];
+	__u8	addr_family;
+	__u8	dhcp_enabled;
+	__u16	ip_addr[MAX_IP_ADDR_SIZE];
+	__u16	sub_net[MAX_IP_ADDR_SIZE];
+	__u16	gate_way[MAX_GATEWAY_SIZE];
+	__u16	dns_addr[MAX_IP_ADDR_SIZE];
+} __attribute__((packed));
+
+
 struct hv_kvp_hdr {
 	__u8 operation;
 	__u8 pool;
@@ -187,10 +248,17 @@ struct hv_kvp_msg {
 		struct hv_kvp_msg_set		kvp_set;
 		struct hv_kvp_msg_delete	kvp_delete;
 		struct hv_kvp_msg_enumerate	kvp_enum_data;
+		struct hv_kvp_ipaddr_value      kvp_ip_val;
 		struct hv_kvp_register		kvp_register;
 	} body;
 } __attribute__((packed));
 
+struct hv_kvp_ip_msg {
+	__u8 operation;
+	__u8 pool;
+	struct hv_kvp_ipaddr_value      kvp_ip_val;
+} __attribute__((packed));
+
 #ifdef __KERNEL__
 #include <linux/scatterlist.h>
 #include <linux/list.h>
@@ -982,6 +1050,7 @@ void vmbus_driver_unregister(struct hv_driver *hv_driver);
 #define HV_S_CONT			0x80070103
 #define HV_ERROR_NOT_SUPPORTED		0x80070032
 #define HV_ERROR_MACHINE_LOCKED		0x800704F7
+#define HV_ERROR_DEVICE_NOT_CONNECTED	0x8007048F
 
 /*
  * While we want to handle util services as regular devices,
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index d9834b3..8fbcf7b 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -69,7 +69,7 @@ enum key_index {
 };
 
 static char kvp_send_buffer[4096];
-static char kvp_recv_buffer[4096];
+static char kvp_recv_buffer[4096 * 2];
 static struct sockaddr_nl addr;
 
 static char *os_name = "";
-- 
1.7.4.1

[PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP

[K. Y. Srinivasan]

In preparation to implementing IP injection, cleanup the way we propagate
and handle errors both in the driver as well as in the user level daemon.

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

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 0012eed..9b7fc4a 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -48,13 +48,24 @@ static struct {
 	void *kvp_context; /* for the channel callback */
 } kvp_transaction;
 
+/*
+ * Before we can accept KVP messages from the host, we need
+ * to handshake with the user level daemon. This state tarcks
+ * if we are in the handshake phase.
+ */
+static bool in_hand_shake = true;
+
+/*
+ * This state maintains the version number registered by the daemon.
+ */
+static int dm_reg_value;
+
 static void kvp_send_key(struct work_struct *dummy);
 
-#define TIMEOUT_FIRED 1
 
 static void kvp_respond_to_host(char *key, char *value, int error);
 static void kvp_work_func(struct work_struct *dummy);
-static void kvp_register(void);
+static void kvp_register(int);
 
 static DECLARE_DELAYED_WORK(kvp_work, kvp_work_func);
 static DECLARE_WORK(kvp_sendkey_work, kvp_send_key);
@@ -68,7 +79,7 @@ static u8 *recv_buffer;
  */
 
 static void
-kvp_register(void)
+kvp_register(int reg_value)
 {
 
 	struct cn_msg *msg;
@@ -83,7 +94,7 @@ kvp_register(void)
 		msg->id.idx =  CN_KVP_IDX;
 		msg->id.val = CN_KVP_VAL;
 
-		kvp_msg->kvp_hdr.operation = KVP_OP_REGISTER;
+		kvp_msg->kvp_hdr.operation = reg_value;
 		strcpy(version, HV_DRV_VERSION);
 		msg->len = sizeof(struct hv_kvp_msg);
 		cn_netlink_send(msg, 0, GFP_ATOMIC);
@@ -97,9 +108,43 @@ kvp_work_func(struct work_struct *dummy)
 	 * If the timer fires, the user-mode component has not responded;
 	 * process the pending transaction.
 	 */
-	kvp_respond_to_host("Unknown key", "Guest timed out", TIMEOUT_FIRED);
+	kvp_respond_to_host("Unknown key", "Guest timed out", HV_E_FAIL);
+}
+
+static int kvp_handle_handshake(struct hv_kvp_msg *msg)
+{
+	int ret = 1;
+
+	switch (msg->kvp_hdr.operation) {
+	case KVP_OP_REGISTER:
+		dm_reg_value = KVP_OP_REGISTER;
+		pr_info("KVP: IP injection functionality not available\n");
+		pr_info("KVP: Upgrade the KVP daemon\n");
+		break;
+	case KVP_OP_REGISTER1:
+		dm_reg_value = KVP_OP_REGISTER1;
+		break;
+	default:
+		pr_info("KVP: incompatible daemon\n");
+		pr_info("KVP: KVP version: %d, Daemon version: %d\n",
+			KVP_OP_REGISTER1, msg->kvp_hdr.operation);
+		ret = 0;
+	}
+
+	if (ret) {
+		/*
+		 * We have a compatible daemon; complete the handshake.
+		 */
+		pr_info("KVP: user-mode registering done.\n");
+		kvp_register(dm_reg_value);
+		kvp_transaction.active = false;
+		if (kvp_transaction.kvp_context)
+			hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+	}
+	return ret;
 }
 
+
 /*
  * Callback when data is received from user mode.
  */
@@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	struct hv_kvp_msg *message;
 	struct hv_kvp_msg_enumerate *data;
+	int	error = 0;
 
 	message = (struct hv_kvp_msg *)msg->data;
-	switch (message->kvp_hdr.operation) {
+
+	/*
+	 * If we are negotiating the version information
+	 * with the daemon; handle that first.
+	 */
+
+	if (in_hand_shake) {
+		if (kvp_handle_handshake(message))
+			in_hand_shake = false;
+		return;
+	}
+
+	/*
+	 * Based on the version of the daemon, we propagate errors from the
+	 * daemon differently.
+	 */
+
+	data = &message->body.kvp_enum_data;
+
+	switch (dm_reg_value) {
 	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);
+		/*
+		 * Null string is used to pass back error condition.
+		 */
+		if (!strlen(data->data.key))
+			error = HV_S_CONT;
 		break;
 
-	default:
-		data = &message->body.kvp_enum_data;
+	case KVP_OP_REGISTER1:
 		/*
-		 * Complete the transaction by forwarding the key value
-		 * to the host. But first, cancel the timeout.
+		 * We use the message header information from
+		 * the user level daemon to transmit errors.
 		 */
-		if (cancel_delayed_work_sync(&kvp_work))
-			kvp_respond_to_host(data->data.key,
-					 data->data.value,
-					!strlen(data->data.key));
+		error = *((int *)(&message->kvp_hdr.operation));
+		break;
 	}
+
+	/*
+	 * Complete the transaction by forwarding the key value
+	 * to the host. But first, cancel the timeout.
+	 */
+	if (cancel_delayed_work_sync(&kvp_work))
+		kvp_respond_to_host(data->data.key, data->data.value, error);
 }
 
 static void
@@ -287,6 +357,7 @@ kvp_respond_to_host(char *key, char *value, int error)
 		 */
 		return;
 
+	icmsghdrp->status = error;
 
 	/*
 	 * If the error parameter is set, terminate the host's enumeration
@@ -294,15 +365,12 @@ kvp_respond_to_host(char *key, char *value, int error)
 	 */
 	if (error) {
 		/*
-		 * Something failed or the we have timedout;
+		 * Something failed or  we have timedout;
 		 * terminate the current  host-side iteration.
 		 */
-		icmsghdrp->status = HV_S_CONT;
 		goto response_done;
 	}
 
-	icmsghdrp->status = HV_S_OK;
-
 	kvp_msg = (struct hv_kvp_msg *)
 			&recv_buffer[sizeof(struct vmbuspipe_hdr) +
 			sizeof(struct icmsg_hdr)];
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 3c56ca7..5552443 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -181,6 +181,17 @@ enum hv_kvp_exchg_pool {
 	KVP_POOL_COUNT /* Number of pools, must be last. */
 };
 
+/*
+ * Some Hyper-V status codes.
+ */
+
+#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
+#define HV_ERROR_DEVICE_NOT_CONNECTED	0x8007048F
+
 #define ADDR_FAMILY_NONE	0x00
 #define ADDR_FAMILY_IPV4	0x01
 #define ADDR_FAMILY_IPV6	0x02
@@ -1045,12 +1056,6 @@ void vmbus_driver_unregister(struct hv_driver *hv_driver);
 #define ICMSGHDRFLAG_REQUEST		2
 #define ICMSGHDRFLAG_RESPONSE		4
 
-#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
-#define HV_ERROR_DEVICE_NOT_CONNECTED	0x8007048F
 
 /*
  * While we want to handle util services as regular devices,
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 8fbcf7b..72f5fd3 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -71,13 +71,14 @@ enum key_index {
 static char kvp_send_buffer[4096];
 static char kvp_recv_buffer[4096 * 2];
 static struct sockaddr_nl addr;
+static int in_hand_shake = 1;
 
 static char *os_name = "";
 static char *os_major = "";
 static char *os_minor = "";
 static char *processor_arch;
 static char *os_build;
-static char *lic_version;
+static char *lic_version = "Unknown version";
 static struct utsname uts_buf;
 
 
@@ -394,7 +395,7 @@ 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,
+static int kvp_pool_enumerate(int pool, int index, __u8 *key, int key_size,
 				__u8 *value, int value_size)
 {
 	struct kvp_record *record;
@@ -406,16 +407,12 @@ static void kvp_pool_enumerate(int pool, int index, __u8 *key, int key_size,
 	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;
+		return 1;
 	}
 
 	memcpy(key, record[index].key, key_size);
 	memcpy(value, record[index].value, value_size);
+	return 0;
 }
 
 
@@ -646,6 +643,8 @@ int main(void)
 	char	*p;
 	char	*key_value;
 	char	*key_name;
+	int	op;
+	int	pool;
 
 	daemon(1, 0);
 	openlog("KVP", 0, LOG_USER);
@@ -687,7 +686,7 @@ int main(void)
 	message->id.val = CN_KVP_VAL;
 
 	hv_msg = (struct hv_kvp_msg *)message->data;
-	hv_msg->kvp_hdr.operation = KVP_OP_REGISTER;
+	hv_msg->kvp_hdr.operation = KVP_OP_REGISTER1;
 	message->ack = 0;
 	message->len = sizeof(struct hv_kvp_msg);
 
@@ -721,12 +720,21 @@ int main(void)
 		incoming_cn_msg = (struct cn_msg *)NLMSG_DATA(incoming_msg);
 		hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
 
-		switch (hv_msg->kvp_hdr.operation) {
-		case KVP_OP_REGISTER:
+		/*
+		 * We will use the KVP header information to pass back
+		 * the error from this daemon. So, first copy the state
+		 * and set the error code to success.
+		 */
+		op = hv_msg->kvp_hdr.operation;
+		pool = hv_msg->kvp_hdr.pool;
+		*((int *)(&hv_msg->kvp_hdr.operation)) = HV_S_OK;
+
+		if ((in_hand_shake) && (op == KVP_OP_REGISTER1)) {
 			/*
 			 * Driver is registering with us; stash away the version
 			 * information.
 			 */
+			in_hand_shake = 0;
 			p = (char *)hv_msg->body.kvp_register.version;
 			lic_version = malloc(strlen(p) + 1);
 			if (lic_version) {
@@ -737,44 +745,42 @@ int main(void)
 				syslog(LOG_ERR, "malloc failed");
 			}
 			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.
-		 */
-
+		switch (op) {
 		case KVP_OP_SET:
-			if (kvp_key_add_or_modify(hv_msg->kvp_hdr.pool,
+			if (kvp_key_add_or_modify(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, "");
+				*((int *)(&hv_msg->kvp_hdr.operation)) =
+								HV_S_CONT;
 			break;
 
 		case KVP_OP_GET:
-			if (kvp_get_value(hv_msg->kvp_hdr.pool,
+			if (kvp_get_value(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, "");
+				*((int *)(&hv_msg->kvp_hdr.operation)) =
+								HV_S_CONT;
 			break;
 
 		case KVP_OP_DELETE:
-			if (kvp_key_delete(hv_msg->kvp_hdr.pool,
+			if (kvp_key_delete(pool,
 					hv_msg->body.kvp_delete.key,
 					hv_msg->body.kvp_delete.key_size))
-				strcpy(hv_msg->body.kvp_delete.key, "");
+				*((int *)(&hv_msg->kvp_hdr.operation)) =
+								HV_S_CONT;
 			break;
 
 		default:
 			break;
 		}
 
-		if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
+		if (op != KVP_OP_ENUMERATE)
 			goto kvp_done;
 
 		/*
@@ -782,13 +788,15 @@ int main(void)
 		 * 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,
+		if (pool != KVP_POOL_AUTO) {
+			if (kvp_pool_enumerate(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);
+					HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+				*((int *)(&hv_msg->kvp_hdr.operation)) =
+								HV_S_CONT;
 			goto kvp_done;
 		}
 
@@ -841,11 +849,7 @@ int main(void)
 			strcpy(key_name, "ProcessorArchitecture");
 			break;
 		default:
-			strcpy(key_value, "Unknown Key");
-			/*
-			 * We use a null key name to terminate enumeration.
-			 */
-			strcpy(key_name, "");
+			*((int *)(&hv_msg->kvp_hdr.operation)) = HV_S_CONT;
 			break;
 		}
 		/*
-- 
1.7.4.1

[PATCH 04/17] Drivers: hv: kvp: Support the new IP injection messages

[K. Y. Srinivasan]

Implement support for the new IP injection messages in the driver code.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/hv_kvp.c |  143 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 9b7fc4a..83e16ea 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -63,7 +63,7 @@ static int dm_reg_value;
 static void kvp_send_key(struct work_struct *dummy);
 
 
-static void kvp_respond_to_host(char *key, char *value, int error);
+static void kvp_respond_to_host(struct hv_kvp_msg *msg, int error);
 static void kvp_work_func(struct work_struct *dummy);
 static void kvp_register(int);
 
@@ -108,7 +108,7 @@ kvp_work_func(struct work_struct *dummy)
 	 * If the timer fires, the user-mode component has not responded;
 	 * process the pending transaction.
 	 */
-	kvp_respond_to_host("Unknown key", "Guest timed out", HV_E_FAIL);
+	kvp_respond_to_host(NULL, HV_E_FAIL);
 }
 
 static int kvp_handle_handshake(struct hv_kvp_msg *msg)
@@ -199,9 +199,120 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 	 * to the host. But first, cancel the timeout.
 	 */
 	if (cancel_delayed_work_sync(&kvp_work))
-		kvp_respond_to_host(data->data.key, data->data.value, error);
+		kvp_respond_to_host(message, error);
 }
 
+
+static int process_ob_ipinfo(void *in_msg, void *out_msg, int op)
+{
+	struct hv_kvp_msg *in = in_msg;
+	struct hv_kvp_ip_msg *out = out_msg;
+	int len;
+
+	switch (op) {
+	case KVP_OP_GET_IP_INFO:
+		/*
+		 * Transform all parameters into utf16 encoding.
+		 */
+		len = utf8s_to_utf16s((char *)in->body.kvp_ip_val.ip_addr,
+				strlen((char *)in->body.kvp_ip_val.ip_addr),
+				UTF16_HOST_ENDIAN,
+				(wchar_t *)out->kvp_ip_val.ip_addr,
+				MAX_IP_ADDR_SIZE);
+		if (len < 0)
+			return len;
+
+		len = utf8s_to_utf16s((char *)in->body.kvp_ip_val.sub_net,
+				strlen((char *)in->body.kvp_ip_val.sub_net),
+				UTF16_HOST_ENDIAN,
+				(wchar_t *)out->kvp_ip_val.sub_net,
+				MAX_IP_ADDR_SIZE);
+		if (len < 0)
+			return len;
+
+		len = utf8s_to_utf16s((char *)in->body.kvp_ip_val.gate_way,
+				strlen((char *)in->body.kvp_ip_val.gate_way),
+				UTF16_HOST_ENDIAN,
+				(wchar_t *)out->kvp_ip_val.gate_way,
+				MAX_GATEWAY_SIZE);
+		if (len < 0)
+			return len;
+
+		len = utf8s_to_utf16s((char *)in->body.kvp_ip_val.dns_addr,
+				strlen((char *)in->body.kvp_ip_val.dns_addr),
+				UTF16_HOST_ENDIAN,
+				(wchar_t *)out->kvp_ip_val.dns_addr,
+				MAX_IP_ADDR_SIZE);
+		if (len < 0)
+			return len;
+
+		len = utf8s_to_utf16s((char *)in->body.kvp_ip_val.adapter_id,
+				strlen((char *)in->body.kvp_ip_val.adapter_id),
+				UTF16_HOST_ENDIAN,
+				(wchar_t *)out->kvp_ip_val.adapter_id,
+				MAX_IP_ADDR_SIZE);
+		if (len < 0)
+			return len;
+
+		out->kvp_ip_val.dhcp_enabled =
+			in->body.kvp_ip_val.dhcp_enabled;
+	}
+
+	return 0;
+}
+
+static void process_ib_ipinfo(void *in_msg, void *out_msg, int op)
+{
+	struct hv_kvp_ip_msg *in = in_msg;
+	struct hv_kvp_msg *out = out_msg;
+
+	switch (op) {
+	case KVP_OP_SET_IP_INFO:
+		/*
+		 * Transform all parameters into utf8 encoding.
+		 */
+		utf16s_to_utf8s((wchar_t *)in->kvp_ip_val.ip_addr,
+				MAX_IP_ADDR_SIZE,
+				UTF16_LITTLE_ENDIAN,
+				(__u8 *)out->body.kvp_ip_val.ip_addr,
+				MAX_IP_ADDR_SIZE);
+
+		utf16s_to_utf8s((wchar_t *)in->kvp_ip_val.sub_net,
+				MAX_IP_ADDR_SIZE,
+				UTF16_LITTLE_ENDIAN,
+				(__u8 *)out->body.kvp_ip_val.sub_net,
+				MAX_IP_ADDR_SIZE);
+
+		utf16s_to_utf8s((wchar_t *)in->kvp_ip_val.gate_way,
+				MAX_GATEWAY_SIZE,
+				UTF16_LITTLE_ENDIAN,
+				(__u8 *)out->body.kvp_ip_val.gate_way,
+				MAX_GATEWAY_SIZE);
+
+		utf16s_to_utf8s((wchar_t *)in->kvp_ip_val.dns_addr,
+				MAX_IP_ADDR_SIZE,
+				UTF16_LITTLE_ENDIAN,
+				(__u8 *)out->body.kvp_ip_val.dns_addr,
+				MAX_IP_ADDR_SIZE);
+
+		out->body.kvp_ip_val.dhcp_enabled =
+			in->kvp_ip_val.dhcp_enabled;
+
+	default:
+		utf16s_to_utf8s((wchar_t *)in->kvp_ip_val.adapter_id,
+				MAX_ADAPTER_ID_SIZE,
+				UTF16_LITTLE_ENDIAN,
+				(__u8 *)out->body.kvp_ip_val.adapter_id,
+				MAX_ADAPTER_ID_SIZE);
+
+		out->body.kvp_ip_val.addr_family =
+		in->kvp_ip_val.addr_family;
+	}
+}
+
+
+
+
 static void
 kvp_send_key(struct work_struct *dummy)
 {
@@ -237,6 +348,12 @@ kvp_send_key(struct work_struct *dummy)
 	 */
 
 	switch (message->kvp_hdr.operation) {
+	case KVP_OP_SET_IP_INFO:
+		process_ib_ipinfo(in_msg, message, KVP_OP_SET_IP_INFO);
+		break;
+	case KVP_OP_GET_IP_INFO:
+		process_ib_ipinfo(in_msg, message, KVP_OP_GET_IP_INFO);
+		break;
 	case KVP_OP_SET:
 		switch (in_msg->body.kvp_set.data.value_type) {
 		case REG_SZ:
@@ -313,17 +430,19 @@ kvp_send_key(struct work_struct *dummy)
  */
 
 static void
-kvp_respond_to_host(char *key, char *value, int error)
+kvp_respond_to_host(struct hv_kvp_msg *msg_to_host, int error)
 {
 	struct hv_kvp_msg  *kvp_msg;
 	struct hv_kvp_exchg_msg_value  *kvp_data;
 	char	*key_name;
+	char	*value;
 	struct icmsg_hdr *icmsghdrp;
 	int	keylen = 0;
 	int	valuelen = 0;
 	u32	buf_len;
 	struct vmbus_channel *channel;
 	u64	req_id;
+	int ret;
 
 	/*
 	 * If a transaction is not active; log and return.
@@ -376,6 +495,16 @@ kvp_respond_to_host(char *key, char *value, int error)
 			sizeof(struct icmsg_hdr)];
 
 	switch (kvp_transaction.kvp_msg->kvp_hdr.operation) {
+	case KVP_OP_GET_IP_INFO:
+		ret = process_ob_ipinfo(msg_to_host,
+				 (struct hv_kvp_ip_msg *)kvp_msg,
+				 KVP_OP_GET_IP_INFO);
+		if (ret < 0)
+			icmsghdrp->status = HV_E_FAIL;
+
+		goto response_done;
+	case KVP_OP_SET_IP_INFO:
+		goto response_done;
 	case KVP_OP_GET:
 		kvp_data = &kvp_msg->body.kvp_get.data;
 		goto copy_value;
@@ -389,7 +518,7 @@ kvp_respond_to_host(char *key, char *value, int error)
 	}
 
 	kvp_data = &kvp_msg->body.kvp_enum_data.data;
-	key_name = key;
+	key_name = msg_to_host->body.kvp_enum_data.data.key;
 
 	/*
 	 * The windows host expects the key/value pair to be encoded
@@ -403,6 +532,7 @@ kvp_respond_to_host(char *key, char *value, int error)
 	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
 
 copy_value:
+	value = msg_to_host->body.kvp_enum_data.data.value;
 	valuelen = utf8s_to_utf16s(value, strlen(value), UTF16_HOST_ENDIAN,
 				(wchar_t *) kvp_data->value,
 				(HV_KVP_EXCHANGE_MAX_VALUE_SIZE / 2) - 2);
@@ -455,7 +585,8 @@ void hv_kvp_onchannelcallback(void *context)
 		return;
 	}
 
-	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE, &recvlen, &requestid);
+	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
+			 &requestid);
 
 	if (recvlen > 0) {
 		icmsghdrp = (struct icmsg_hdr *)&recv_buffer[
-- 
1.7.4.1

[PATCH 05/17] Tools: hv: Prepare to expand kvp_get_ip_address() functionality

[K. Y. Srinivasan]

kvp_get_ip_address() implemented the functionality to retrieve IP address info.
Make this function more generic so that we could retrieve additional
per-interface information.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 72f5fd3..2169f2f 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -492,7 +492,8 @@ done:
 }
 
 static int
-kvp_get_ip_address(int family, char *buffer, int length)
+kvp_get_ip_address(int family, char *if_name, int op,
+		 void  *out_buffer, int length)
 {
 	struct ifaddrs *ifap;
 	struct ifaddrs *curp;
@@ -502,10 +503,19 @@ kvp_get_ip_address(int family, char *buffer, int length)
 	const char *str;
 	char tmp[50];
 	int error = 0;
-
+	char *buffer;
+	struct hv_kvp_ipaddr_value *ip_buffer;
+
+	if (op == KVP_OP_ENUMERATE) {
+		buffer = out_buffer;
+	} else {
+		ip_buffer = out_buffer;
+		buffer = (char *)ip_buffer->ip_addr;
+		ip_buffer->addr_family = 0;
+	}
 	/*
 	 * On entry into this function, the buffer is capable of holding the
-	 * maximum key value (2048 bytes).
+	 * maximum key value.
 	 */
 
 	if (getifaddrs(&ifap)) {
@@ -515,58 +525,87 @@ kvp_get_ip_address(int family, char *buffer, int length)
 
 	curp = ifap;
 	while (curp != NULL) {
-		if ((curp->ifa_addr != NULL) &&
-		   (curp->ifa_addr->sa_family == family)) {
-			if (family == AF_INET) {
-				struct sockaddr_in *addr =
-				(struct sockaddr_in *) curp->ifa_addr;
-
-				str = inet_ntop(family, &addr->sin_addr,
-						tmp, 50);
-				if (str == NULL) {
-					strcpy(buffer, "inet_ntop failed\n");
-					error = 1;
-					goto getaddr_done;
-				}
-				if (offset == 0)
-					strcpy(buffer, tmp);
-				else
-					strcat(buffer, tmp);
-				strcat(buffer, ";");
+		if (curp->ifa_addr == NULL) {
+			curp = curp->ifa_next;
+			continue;
+		}
 
-				offset += strlen(str) + 1;
-				if ((length - offset) < (ipv4_len + 1))
-					goto getaddr_done;
+		if ((if_name != NULL) &&
+			(strncmp(curp->ifa_name, if_name, strlen(if_name)))) {
+			/*
+			 * We want info about a specific interface;
+			 * just continue.
+			 */
+			curp = curp->ifa_next;
+			continue;
+		}
 
-			} else {
+		/*
+		 * We only support two address families: AF_INET and AF_INET6.
+		 * If a family value of 0 is specified, we collect both
+		 * supported address families; if not we gather info on
+		 * the specified address family.
+		 */
+		if ((family != 0) && (curp->ifa_addr->sa_family != family)) {
+			curp = curp->ifa_next;
+			continue;
+		}
+		if ((curp->ifa_addr->sa_family != AF_INET) &&
+			(curp->ifa_addr->sa_family != AF_INET6)) {
+			curp = curp->ifa_next;
+			continue;
+		}
+
+		if ((curp->ifa_addr->sa_family == AF_INET) &&
+			((family == AF_INET) || (family == 0))) {
+			struct sockaddr_in *addr =
+			(struct sockaddr_in *) curp->ifa_addr;
+
+			str = inet_ntop(AF_INET, &addr->sin_addr, tmp, 50);
+			if (str == NULL) {
+				strcpy(buffer, "inet_ntop failed\n");
+				error = 1;
+				goto getaddr_done;
+			}
+			if (offset == 0)
+				strcpy(buffer, tmp);
+			else
+				strcat(buffer, tmp);
+			strcat(buffer, ";");
+
+			offset += strlen(str) + 1;
+			if ((length - offset) < (ipv4_len + 1))
+				goto getaddr_done;
+
+		} else if ((family == AF_INET6) || (family == 0)) {
 
 			/*
 			 * We only support AF_INET and AF_INET6
 			 * and the list of addresses is separated by a ";".
 			 */
-				struct sockaddr_in6 *addr =
+			struct sockaddr_in6 *addr =
 				(struct sockaddr_in6 *) curp->ifa_addr;
 
-				str = inet_ntop(family,
+			str = inet_ntop(AF_INET6,
 					&addr->sin6_addr.s6_addr,
 					tmp, 50);
-				if (str == NULL) {
-					strcpy(buffer, "inet_ntop failed\n");
-					error = 1;
-					goto getaddr_done;
-				}
-				if (offset == 0)
-					strcpy(buffer, tmp);
-				else
-					strcat(buffer, tmp);
-				strcat(buffer, ";");
-				offset += strlen(str) + 1;
-				if ((length - offset) < (ipv6_len + 1))
-					goto getaddr_done;
-
+			if (str == NULL) {
+				strcpy(buffer, "inet_ntop failed\n");
+				error = 1;
+				goto getaddr_done;
 			}
+			if (offset == 0)
+				strcpy(buffer, tmp);
+			else
+				strcat(buffer, tmp);
+			strcat(buffer, ";");
+			offset += strlen(str) + 1;
+			if ((length - offset) < (ipv6_len + 1))
+				goto getaddr_done;
 
 		}
+
+
 		curp = curp->ifa_next;
 	}
 
@@ -815,13 +854,13 @@ int main(void)
 			strcpy(key_value, lic_version);
 			break;
 		case NetworkAddressIPv4:
-			kvp_get_ip_address(AF_INET, key_value,
-					HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
+			kvp_get_ip_address(AF_INET, NULL, KVP_OP_ENUMERATE,
+				key_value, HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
 			strcpy(key_name, "NetworkAddressIPv4");
 			break;
 		case NetworkAddressIPv6:
-			kvp_get_ip_address(AF_INET6, key_value,
-					HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
+			kvp_get_ip_address(AF_INET6, NULL, KVP_OP_ENUMERATE,
+				key_value, HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
 			strcpy(key_name, "NetworkAddressIPv6");
 			break;
 		case OSBuildNumber:
-- 
1.7.4.1

[PATCH 06/17] Tools: hv: Further refactor kvp_get_ip_address()

[K. Y. Srinivasan]

In preparation for making kvp_get_ip_address() more generic, factor out
the code for handling IP addresses.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 2169f2f..933c164 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -491,17 +491,50 @@ done:
 	return;
 }
 
+static int kvp_process_ip_address(void *addrp,
+				int family, char *buffer,
+				int length,  int *offset)
+{
+	struct sockaddr_in *addr;
+	struct sockaddr_in6 *addr6;
+	int addr_length;
+	char tmp[50];
+	const char *str;
+
+	if (family == AF_INET) {
+		addr = (struct sockaddr_in *)addrp;
+		str = inet_ntop(family, &addr->sin_addr, tmp, 50);
+		addr_length = INET_ADDRSTRLEN;
+	} else {
+		addr6 = (struct sockaddr_in6 *)addrp;
+		str = inet_ntop(family, &addr6->sin6_addr.s6_addr, tmp, 50);
+		addr_length = INET6_ADDRSTRLEN;
+	}
+
+	if ((length - *offset) < addr_length + 1)
+		return 1;
+	if (str == NULL) {
+		strcpy(buffer, "inet_ntop failed\n");
+		return 1;
+	}
+	if (*offset == 0)
+		strcpy(buffer, tmp);
+	else
+		strcat(buffer, tmp);
+	strcat(buffer, ";");
+
+	*offset += strlen(str) + 1;
+	return 0;
+}
+
 static int
 kvp_get_ip_address(int family, char *if_name, int op,
 		 void  *out_buffer, int length)
 {
 	struct ifaddrs *ifap;
 	struct ifaddrs *curp;
-	int ipv4_len = strlen("255.255.255.255") + 1;
-	int ipv6_len = strlen("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff")+1;
 	int offset = 0;
 	const char *str;
-	char tmp[50];
 	int error = 0;
 	char *buffer;
 	struct hv_kvp_ipaddr_value *ip_buffer;
@@ -556,55 +589,12 @@ kvp_get_ip_address(int family, char *if_name, int op,
 			continue;
 		}
 
-		if ((curp->ifa_addr->sa_family == AF_INET) &&
-			((family == AF_INET) || (family == 0))) {
-			struct sockaddr_in *addr =
-			(struct sockaddr_in *) curp->ifa_addr;
-
-			str = inet_ntop(AF_INET, &addr->sin_addr, tmp, 50);
-			if (str == NULL) {
-				strcpy(buffer, "inet_ntop failed\n");
-				error = 1;
-				goto getaddr_done;
-			}
-			if (offset == 0)
-				strcpy(buffer, tmp);
-			else
-				strcat(buffer, tmp);
-			strcat(buffer, ";");
-
-			offset += strlen(str) + 1;
-			if ((length - offset) < (ipv4_len + 1))
-				goto getaddr_done;
-
-		} else if ((family == AF_INET6) || (family == 0)) {
-
-			/*
-			 * We only support AF_INET and AF_INET6
-			 * and the list of addresses is separated by a ";".
-			 */
-			struct sockaddr_in6 *addr =
-				(struct sockaddr_in6 *) curp->ifa_addr;
-
-			str = inet_ntop(AF_INET6,
-					&addr->sin6_addr.s6_addr,
-					tmp, 50);
-			if (str == NULL) {
-				strcpy(buffer, "inet_ntop failed\n");
-				error = 1;
-				goto getaddr_done;
-			}
-			if (offset == 0)
-				strcpy(buffer, tmp);
-			else
-				strcat(buffer, tmp);
-			strcat(buffer, ";");
-			offset += strlen(str) + 1;
-			if ((length - offset) < (ipv6_len + 1))
-				goto getaddr_done;
-
-		}
-
+		error = kvp_process_ip_address(curp->ifa_addr,
+						curp->ifa_addr->sa_family,
+						buffer,
+						length, &offset);
+		if (error)
+			goto getaddr_done;
 
 		curp = curp->ifa_next;
 	}
-- 
1.7.4.1

[PATCH 07/17] Tools: hv: Gather address family information

[K. Y. Srinivasan]

Now, gather address family information for the specified interface.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 933c164..79eb130 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -589,6 +589,17 @@ kvp_get_ip_address(int family, char *if_name, int op,
 			continue;
 		}
 
+		if (op == KVP_OP_GET_IP_INFO) {
+			/*
+			 * Gather info other than the IP address.
+			 * IP address info will be gathered later.
+			 */
+			if (curp->ifa_addr->sa_family == AF_INET)
+				ip_buffer->addr_family |= ADDR_FAMILY_IPV4;
+			else
+				ip_buffer->addr_family |= ADDR_FAMILY_IPV6;
+		}
+
 		error = kvp_process_ip_address(curp->ifa_addr,
 						curp->ifa_addr->sa_family,
 						buffer,
-- 
1.7.4.1

[PATCH 08/17] Tools: hv: Gather subnet information

[K. Y. Srinivasan]

Now gather sub-net information for the specified interface.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 79eb130..2c24ebf 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -534,6 +534,7 @@ kvp_get_ip_address(int family, char *if_name, int op,
 	struct ifaddrs *ifap;
 	struct ifaddrs *curp;
 	int offset = 0;
+	int sn_offset = 0;
 	const char *str;
 	int error = 0;
 	char *buffer;
@@ -594,12 +595,38 @@ kvp_get_ip_address(int family, char *if_name, int op,
 			 * Gather info other than the IP address.
 			 * IP address info will be gathered later.
 			 */
-			if (curp->ifa_addr->sa_family == AF_INET)
+			if (curp->ifa_addr->sa_family == AF_INET) {
 				ip_buffer->addr_family |= ADDR_FAMILY_IPV4;
-			else
+				/*
+				 * Get subnet info.
+				 */
+				error = kvp_process_ip_address(
+							curp->ifa_netmask,
+							AF_INET,
+							(char *)
+							ip_buffer->sub_net,
+							length,
+							&sn_offset);
+				if (error)
+					goto gather_ipaddr;
+			} else {
 				ip_buffer->addr_family |= ADDR_FAMILY_IPV6;
+				/*
+				 * Get subnet info.
+				 */
+				error = kvp_process_ip_address(
+							curp->ifa_netmask,
+							AF_INET6,
+							(char *)
+							ip_buffer->sub_net,
+							length,
+							&sn_offset);
+				if (error)
+					goto gather_ipaddr;
+			}
 		}
 
+gather_ipaddr:
 		error = kvp_process_ip_address(curp->ifa_addr,
 						curp->ifa_addr->sa_family,
 						buffer,
-- 
1.7.4.1

[PATCH 09/17] Tools: hv: Represent the ipv6 mask using CIDR notation

[K. Y. Srinivasan]

Transform ipv6 subnet information to CIDR notation.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 2c24ebf..007e698 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -491,6 +491,15 @@ done:
 	return;
 }
 
+static unsigned int hweight32(unsigned int *w)
+{
+	unsigned int res = *w - ((*w >> 1) & 0x55555555);
+	res = (res & 0x33333333) + ((res >> 2) & 0x33333333);
+	res = (res + (res >> 4)) & 0x0F0F0F0F;
+	res = res + (res >> 8);
+	return (res + (res >> 16)) & 0x000000FF;
+}
+
 static int kvp_process_ip_address(void *addrp,
 				int family, char *buffer,
 				int length,  int *offset)
@@ -535,10 +544,15 @@ kvp_get_ip_address(int family, char *if_name, int op,
 	struct ifaddrs *curp;
 	int offset = 0;
 	int sn_offset = 0;
-	const char *str;
 	int error = 0;
 	char *buffer;
 	struct hv_kvp_ipaddr_value *ip_buffer;
+	char cidr_mask[5]; /* /xyz */
+	int weight;
+	int i;
+	unsigned int *w;
+	char *sn_str;
+	struct sockaddr_in6 *addr6;
 
 	if (op == KVP_OP_ENUMERATE) {
 		buffer = out_buffer;
@@ -612,17 +626,28 @@ kvp_get_ip_address(int family, char *if_name, int op,
 			} else {
 				ip_buffer->addr_family |= ADDR_FAMILY_IPV6;
 				/*
-				 * Get subnet info.
+				 * Get subnet info in CIDR format.
 				 */
-				error = kvp_process_ip_address(
-							curp->ifa_netmask,
-							AF_INET6,
-							(char *)
-							ip_buffer->sub_net,
-							length,
-							&sn_offset);
-				if (error)
+				weight = 0;
+				sn_str = (char *)ip_buffer->sub_net;
+				addr6 = (struct sockaddr_in6 *)
+					curp->ifa_netmask;
+				w = addr6->sin6_addr.s6_addr32;
+
+				for (i = 0; i < 4; i++)
+					weight += hweight32(&w[i]);
+
+				sprintf(cidr_mask, "/%d", weight);
+				if ((length - sn_offset) <
+					(strlen(cidr_mask) + 1))
 					goto gather_ipaddr;
+
+				if (sn_offset == 0)
+					strcpy(sn_str, cidr_mask);
+				else
+					strcat(sn_str, cidr_mask);
+				strcat((char *)ip_buffer->sub_net, ";");
+				sn_offset += strlen(sn_str) + 1;
 			}
 		}
 
-- 
1.7.4.1

[PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[K. Y. Srinivasan]

Gather information on the default gateways - ipv4/ipv6.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 007e698..b627236 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -491,6 +491,72 @@ done:
 	return;
 }
 
+static void kvp_process_ipconfig_file(char *cmd,
+					char *config_buf, int len,
+					int element_size, int offset)
+{
+	char buf[256];
+	char *p;
+	char *x;
+	FILE *file;
+
+	/*
+	 * First execute the command.
+	 */
+	file = popen(cmd, "r");
+	if (file == NULL)
+		return;
+
+	if (offset == 0)
+		memset(config_buf, 0, len);
+	while ((p = fgets(buf, sizeof(buf), file)) != NULL) {
+		if ((len - strlen(config_buf)) < (element_size + 1))
+			break;
+
+		x = strchr(p, '\n');
+		*x = '\0';
+		strcat(config_buf, p);
+		strcat(config_buf, ";");
+	}
+	pclose(file);
+}
+
+static void kvp_get_ipconfig_info(char *if_name,
+				 struct hv_kvp_ipaddr_value *buffer)
+{
+	char cmd[512];
+
+	/*
+	 * Get the address of default gateway (ipv4).
+	 */
+	memset(cmd, 0, sizeof(cmd));
+	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
+	strcat(cmd, if_name);
+	strcat(cmd, " | awk '/default/ {print $3 }'");
+
+	/*
+	 * Execute the command to gather gateway info.
+	 */
+	kvp_process_ipconfig_file(cmd, (char *)buffer->gate_way,
+				(MAX_GATEWAY_SIZE * 2), INET_ADDRSTRLEN, 0);
+
+	/*
+	 * Get the address of default gateway (ipv6).
+	 */
+	memset(cmd, 0, sizeof(cmd));
+	strcat(cmd, "/sbin/ip -f inet6  route | grep -w ");
+	strcat(cmd, if_name);
+	strcat(cmd, " | awk '/default/ {print $3 }'");
+
+	/*
+	 * Execute the command to gather gateway info (ipv6).
+	 */
+	kvp_process_ipconfig_file(cmd, (char *)buffer->gate_way,
+				(MAX_GATEWAY_SIZE * 2), INET6_ADDRSTRLEN, 1);
+
+}
+
+
 static unsigned int hweight32(unsigned int *w)
 {
 	unsigned int res = *w - ((*w >> 1) & 0x55555555);
@@ -649,6 +715,12 @@ kvp_get_ip_address(int family, char *if_name, int op,
 				strcat((char *)ip_buffer->sub_net, ";");
 				sn_offset += strlen(sn_str) + 1;
 			}
+
+			/*
+			 * Collect other ip related configuration info.
+			 */
+
+			kvp_get_ipconfig_info(if_name, ip_buffer);
 		}
 
 gather_ipaddr:
-- 
1.7.4.1

[PATCH 11/17] Tools: hv: Gather DNS information

[K. Y. Srinivasan]

Now gather DNS information. This information cannot be gathered in
a distro independent fashion. Invoke an external script (that can be
distro dependent) to gather the DNS information.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index b627236..9500cff 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -554,6 +554,33 @@ static void kvp_get_ipconfig_info(char *if_name,
 	kvp_process_ipconfig_file(cmd, (char *)buffer->gate_way,
 				(MAX_GATEWAY_SIZE * 2), INET6_ADDRSTRLEN, 1);
 
+
+	/*
+	 * Gather the DNS  state.
+	 * Since there is no standard way to get this information
+	 * across various distributions of interest; we just invoke
+	 * an external script that needs to be ported across distros
+	 * of interest.
+	 * Input to the script:
+	 * 1) Interface name
+	 *
+	 * Following is the expected format of the information from the script:
+	 *
+	 * ipaddr1 (nameserver1)
+	 * ipaddr2 (nameserver2)
+	 * .
+	 * .
+	 */
+
+	memset(cmd, 0, sizeof(cmd));
+	strcat(cmd, "/sbin/hv_get_dns_info ");
+	strcat(cmd, if_name);
+
+	/*
+	 * Execute the command to gather DNS info.
+	 */
+	kvp_process_ipconfig_file(cmd, (char *)buffer->dns_addr,
+				(MAX_IP_ADDR_SIZE * 2), INET_ADDRSTRLEN, 0);
 }
 
 
-- 
1.7.4.1

[PATCH 12/17] Tools: hv: Gather DHCP information

[K. Y. Srinivasan]

Collect information on dhcp setting for the specified interface.
We invoke an exyernal script to get this information.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 9500cff..2d01ccd 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -525,6 +525,9 @@ static void kvp_get_ipconfig_info(char *if_name,
 				 struct hv_kvp_ipaddr_value *buffer)
 {
 	char cmd[512];
+	char dhcp_info[128];
+	char *p;
+	FILE *file;
 
 	/*
 	 * Get the address of default gateway (ipv4).
@@ -581,6 +584,36 @@ static void kvp_get_ipconfig_info(char *if_name,
 	 */
 	kvp_process_ipconfig_file(cmd, (char *)buffer->dns_addr,
 				(MAX_IP_ADDR_SIZE * 2), INET_ADDRSTRLEN, 0);
+
+	/*
+	 * Gather the DHCP state.
+	 * We will gather this state by invoking an external script.
+	 * The parameter to the script is the interface name.
+	 * Here is the expected output:
+	 *
+	 * Enabled: DHCP enabled.
+	 */
+
+	memset(cmd, 0, sizeof(cmd));
+	strcat(cmd, "/sbin/hv_get_dhcp_info ");
+	strcat(cmd, if_name);
+
+	file = popen(cmd, "r");
+	if (file == NULL)
+		return;
+
+	p = fgets(dhcp_info, sizeof(dhcp_info), file);
+	if (p == NULL) {
+		pclose(file);
+		return;
+	}
+
+	if (!strncmp(p, "Enabled", 7))
+		buffer->dhcp_enabled = 1;
+	else
+		buffer->dhcp_enabled = 0;
+
+	pclose(file);
 }
 
 
-- 
1.7.4.1

[PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[K. Y. Srinivasan]

Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures the
specified interface based on the given configuration. Since configuring
an interface is very distro specific, we invoke an external script to
configure the interface.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 include/linux/hyperv.h   |    2 +
 tools/hv/hv_kvp_daemon.c |  440 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 442 insertions(+), 0 deletions(-)

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 5552443..ac6d553 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -191,6 +191,8 @@ enum hv_kvp_exchg_pool {
 #define HV_ERROR_NOT_SUPPORTED		0x80070032
 #define HV_ERROR_MACHINE_LOCKED		0x800704F7
 #define HV_ERROR_DEVICE_NOT_CONNECTED	0x8007048F
+#define HV_INVALIDARG			0x80070057
+#define HV_GUID_NOTFOUND		0x80041002
 
 #define ADDR_FAMILY_NONE	0x00
 #define ADDR_FAMILY_IPV4	0x01
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 2d01ccd..090d6a2 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -31,6 +31,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <ctype.h>
 #include <errno.h>
 #include <arpa/inet.h>
 #include <linux/connector.h>
@@ -41,6 +42,7 @@
 #include <syslog.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <dirent.h>
 
 /*
  * KVP protocol: The user mode component first registers with the
@@ -68,6 +70,14 @@ enum key_index {
 	ProcessorArchitecture
 };
 
+
+enum {
+	IPADDR = 0,
+	NETMASK,
+	GATEWAY,
+	DNS
+};
+
 static char kvp_send_buffer[4096];
 static char kvp_recv_buffer[4096 * 2];
 static struct sockaddr_nl addr;
@@ -491,6 +501,106 @@ done:
 	return;
 }
 
+
+
+/*
+ * Retrieve an interface name corresponding to the specified guid.
+ * If there is a match, the function returns a pointer
+ * to the interface name and if not, a NULL is returned.
+ * If a match is found, the caller is responsible for
+ * freeing the memory.
+ */
+
+static char *kvp_get_if_name(char *guid)
+{
+	DIR *dir;
+	struct dirent *entry;
+	FILE    *file;
+	char    *p, *q, *x;
+	char    *if_name = NULL;
+	char    buf[256];
+	char *kvp_net_dir = "/sys/class/net/";
+	char dev_id[100];
+
+	dir = opendir(kvp_net_dir);
+	if (dir == NULL)
+		return NULL;
+
+	memset(dev_id, 0, sizeof(dev_id));
+	strcat(dev_id, kvp_net_dir);
+	q = dev_id + strlen(kvp_net_dir);
+
+	while ((entry = readdir(dir)) != NULL) {
+		/*
+		 * Set the state for the next pass.
+		 */
+		*q = '\0';
+		strcat(dev_id, entry->d_name);
+		strcat(dev_id, "/device/device_id");
+
+		file = fopen(dev_id, "r");
+		if (file == NULL)
+			continue;
+
+		p = fgets(buf, sizeof(buf), file);
+		if (p) {
+			x = strchr(p, '\n');
+			if (x)
+				*x = '\0';
+
+			if (!strcmp(p, guid)) {
+				/*
+				 * Found the guid match; return the interface
+				 * name. The caller will free the memory.
+				 */
+				if_name = strdup(entry->d_name);
+				break;
+			}
+		}
+		fclose(file);
+	}
+
+	closedir(dir);
+	return if_name;
+}
+
+/*
+ * Retrieve the MAC address given the interface name.
+ */
+
+static char *kvp_if_name_to_mac(char *if_name)
+{
+	FILE    *file;
+	char    *p, *x;
+	char    buf[256];
+	char addr_file[100];
+	int i;
+	char *mac_addr = NULL;
+
+	memset(addr_file, 0, sizeof(addr_file));
+	strcat(addr_file, "/sys/class/net/");
+	strcat(addr_file, if_name);
+	strcat(addr_file, "/address");
+
+	file = fopen(addr_file, "r");
+	if (file == NULL)
+		return NULL;
+
+	p = fgets(buf, sizeof(buf), file);
+	if (p) {
+		x = strchr(p, '\n');
+		if (x)
+			*x = '\0';
+		for (i = 0; i < strlen(p); i++)
+			p[i] = toupper(p[i]);
+		mac_addr = strdup(p);
+	}
+
+	fclose(file);
+	return mac_addr;
+}
+
+
 static void kvp_process_ipconfig_file(char *cmd,
 					char *config_buf, int len,
 					int element_size, int offset)
@@ -800,6 +910,314 @@ getaddr_done:
 }
 
 
+static int expand_ipv6(char *addr, int type)
+{
+	int ret;
+	struct in6_addr v6_addr;
+
+	ret = inet_pton(AF_INET6, addr, &v6_addr);
+
+	if (ret != 1) {
+		if (type == NETMASK)
+			return 1;
+		return 0;
+	}
+
+	sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
+		"%02x%02x:%02x%02x:%02x%02x",
+		(int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],
+		(int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],
+		(int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],
+		(int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],
+		(int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],
+		(int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],
+		(int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],
+		(int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);
+
+	return 1;
+
+}
+
+static int is_ipv4(char *addr)
+{
+	int ret;
+	struct in_addr ipv4_addr;
+
+	ret = inet_pton(AF_INET, addr, &ipv4_addr);
+
+	if (ret == 1)
+		return 1;
+	return 0;
+}
+
+static int parse_ip_val_buffer(char *in_buf, int *offset,
+				char *out_buf, int out_len)
+{
+	char *x;
+	char *start;
+
+	/*
+	 * in_buf has sequence of characters that are seperated by
+	 * the character ';'. The last sequence does not have the
+	 * terminating ";" character.
+	 */
+	start = in_buf + *offset;
+
+	x = strchr(start, ';');
+	if (x)
+		*x = 0;
+	else
+		x = start + strlen(start);
+
+	if (strlen(start) != 0) {
+		int i = 0;
+		/*
+		 * Get rid of leading spaces.
+		 */
+		while (start[i] == ' ')
+			i++;
+
+		if ((x - start) <= out_len) {
+			strcpy(out_buf, (start + i));
+			*offset += (x - start) + 1;
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
+{
+	char str[256];
+	int error;
+
+	memset(str, 0, sizeof(str));
+	strcat(str, s1);
+	if (s2 != NULL)
+		strcat(str, s2);
+	strcat(str, "=");
+	strcat(str, s3);
+	strcat(str, "\n");
+
+	error = fputs(str, f);
+	if (error == EOF)
+		return HV_E_FAIL;
+
+	return 0;
+}
+
+
+static int process_ip_string(FILE *f, char *ip_string, int type)
+{
+	int error = 0;
+	char addr[INET6_ADDRSTRLEN];
+	int i = 0;
+	int j = 0;
+	char str[256];
+	char sub_str[10];
+	int offset = 0;
+
+	memset(addr, 0, sizeof(addr));
+
+	while (parse_ip_val_buffer(ip_string, &offset, addr,
+					(MAX_IP_ADDR_SIZE * 2))) {
+		memset(sub_str, 0, sizeof(sub_str));
+		memset(str, 0, sizeof(str));
+
+		if (is_ipv4(addr)) {
+			switch (type) {
+			case IPADDR:
+				strcat(str, "IPADDR");
+				break;
+			case NETMASK:
+				strcat(str, "NETMASK");
+				break;
+			case GATEWAY:
+				strcat(str, "GATEWAY");
+				break;
+			case DNS:
+				strcat(str, "DNS");
+				break;
+			}
+			if (i != 0) {
+				if (type != DNS)
+					sprintf(sub_str, "_%d", i++);
+				else
+					sprintf(sub_str, "%d", ++i);
+			} else if (type == DNS) {
+				sprintf(sub_str, "%d", ++i);
+			}
+
+
+		} else if (expand_ipv6(addr, type)) {
+			switch (type) {
+			case IPADDR:
+				strcat(str, "IPV6ADDR");
+				break;
+			case NETMASK:
+				strcat(str, "IPV6NETMASK");
+				break;
+			case GATEWAY:
+				strcat(str, "IPV6_DEFAULTGW");
+				break;
+			case DNS:
+				strcat(str, "DNS");
+				break;
+			}
+			if ((j != 0) || (type == DNS)) {
+				if (type != DNS)
+					sprintf(sub_str, "_%d", j++);
+				else
+					sprintf(sub_str, "%d", ++i);
+			} else if (type == DNS) {
+				sprintf(sub_str, "%d", ++i);
+			}
+		} else {
+			return  HV_INVALIDARG;
+		}
+
+		error = kvp_write_file(f, str, sub_str, addr);
+		if (error)
+			return error;
+		memset(addr, 0, sizeof(addr));
+	}
+
+	return 0;
+}
+
+static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
+{
+	int error = 0;
+	char if_file[50];
+	FILE *file;
+	char cmd[512];
+	char *mac_addr;
+
+	/*
+	 * Set the configuration for the specified interface with
+	 * the information provided. Since there is no standard
+	 * way to configure an interface, we will have an external
+	 * script that does the job of configuring the interface and
+	 * flushing the configuration.
+	 *
+	 * The parameters passed to this external script are:
+	 * 1. A configuration file that has the specified configuration.
+	 *
+	 * We will embed the name of the interface in the configuration
+	 * file: ifcfg-ethx (where ethx is the interface name).
+	 *
+	 * Here is the format of the ip configuration file:
+	 *
+	 * HWADDR=macaddr
+	 * BOOTPROTO=dhcp (dhcp enabled for the interface)
+	 * NM_CONTROLLED=no (this interface will not be controlled by NM)
+	 * PEERDNS=yes
+	 * IPADDR_x=ipaddr
+	 * NETMASK_x=netmask
+	 * GATEWAY_x=gateway
+	 * DNSx=dns
+	 *
+	 * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
+	 * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
+	 * IPV6NETMASK.
+	 */
+
+	memset(if_file, 0, sizeof(if_file));
+	strcat(if_file, "/var/opt/hyperv/ifcfg-");
+	strcat(if_file, if_name);
+
+	file = fopen(if_file, "w");
+
+	if (file == NULL) {
+		syslog(LOG_ERR, "Failed to open config file");
+		return HV_E_FAIL;
+	}
+
+	/*
+	 * First write out the MAC address.
+	 */
+
+	mac_addr = kvp_if_name_to_mac(if_name);
+	if (mac_addr == NULL) {
+		error = HV_E_FAIL;
+		goto setval_error;
+	}
+
+	error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
+	if (error)
+		goto setval_error;
+
+	error = kvp_write_file(file, "ONBOOT", NULL, "yes");
+	if (error)
+		goto setval_error;
+
+	error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
+	if (error)
+		goto setval_error;
+
+	error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
+	if (error)
+		goto setval_error;
+
+	error = kvp_write_file(file, "PEERDNS", NULL, "yes");
+	if (error)
+		goto setval_error;
+
+	if (new_val->dhcp_enabled) {
+		error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
+		if (error)
+			goto setval_error;
+
+		/*
+		 * We are done!.
+		 */
+		goto setval_done;
+	}
+
+	/*
+	 * Write the configuration for ipaddress, netmask, gateway and
+	 * name servers.
+	 */
+
+	error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
+	if (error)
+		goto setval_error;
+
+	error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
+	if (error)
+		goto setval_error;
+
+	error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
+	if (error)
+		goto setval_error;
+
+	error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
+	if (error)
+		goto setval_error;
+
+setval_done:
+	free(mac_addr);
+	fclose(file);
+
+	/*
+	 * Now that we have populated the configuration file,
+	 * invoke the external script to do its magic.
+	 */
+
+	memset(cmd, 0, sizeof(cmd));
+	strcat(cmd, "/sbin/hv_set_ifconfig ");
+	strcat(cmd, if_file);
+	system(cmd);
+	return 0;
+
+setval_error:
+	syslog(LOG_ERR, "Failed to write config file");
+	free(mac_addr);
+	fclose(file);
+	return error;
+}
+
+
 static int
 kvp_get_domain_name(char *buffer, int length)
 {
@@ -869,6 +1287,8 @@ int main(void)
 	char	*key_name;
 	int	op;
 	int	pool;
+	char	*if_name;
+	struct hv_kvp_ipaddr_value *kvp_ip_val;
 
 	daemon(1, 0);
 	openlog("KVP", 0, LOG_USER);
@@ -972,6 +1392,26 @@ int main(void)
 		}
 
 		switch (op) {
+		case KVP_OP_SET_IP_INFO:
+			kvp_ip_val = &hv_msg->body.kvp_ip_val;
+			if_name = kvp_get_if_name(
+					(char *)kvp_ip_val->adapter_id);
+			if (if_name == NULL) {
+				/*
+				 * We could not map the guid to an
+				 * interface name; return error.
+				 */
+				*((int *)(&hv_msg->kvp_hdr.operation)) =
+					HV_GUID_NOTFOUND;
+				break;
+			}
+			error = kvp_set_ip_info(if_name, kvp_ip_val);
+			if (error)
+				*((int *)(&hv_msg->kvp_hdr.operation)) = error;
+
+			free(if_name);
+			break;
+
 		case KVP_OP_SET:
 			if (kvp_key_add_or_modify(pool,
 					hv_msg->body.kvp_set.data.key,
-- 
1.7.4.1

[PATCH 14/17] Tools: hv: Rename the function kvp_get_ip_address()

[K. Y. Srinivasan]

Rename the function kvp_get_ip_address() to better reflect the functionality
being implemented.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 090d6a2..87cfc64 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -773,7 +773,7 @@ static int kvp_process_ip_address(void *addrp,
 }
 
 static int
-kvp_get_ip_address(int family, char *if_name, int op,
+kvp_get_ip_info(int family, char *if_name, int op,
 		 void  *out_buffer, int length)
 {
 	struct ifaddrs *ifap;
@@ -1479,12 +1479,12 @@ int main(void)
 			strcpy(key_value, lic_version);
 			break;
 		case NetworkAddressIPv4:
-			kvp_get_ip_address(AF_INET, NULL, KVP_OP_ENUMERATE,
+			kvp_get_ip_info(AF_INET, NULL, KVP_OP_ENUMERATE,
 				key_value, HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
 			strcpy(key_name, "NetworkAddressIPv4");
 			break;
 		case NetworkAddressIPv6:
-			kvp_get_ip_address(AF_INET6, NULL, KVP_OP_ENUMERATE,
+			kvp_get_ip_info(AF_INET6, NULL, KVP_OP_ENUMERATE,
 				key_value, HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
 			strcpy(key_name, "NetworkAddressIPv6");
 			break;
-- 
1.7.4.1

[PATCH 15/17] Tools: hv: Implement the KVP verb - KVP_OP_GET_IP_INFO

[K. Y. Srinivasan]

Now implement the KVP verb - KVP_OP_GET_IP_INFO. This operation retrieves IP
information for the specified interface.

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 87cfc64..3bce574 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -601,6 +601,69 @@ static char *kvp_if_name_to_mac(char *if_name)
 }
 
 
+/*
+ * Retrieve the interface name given tha MAC address.
+ */
+
+static char *kvp_mac_to_if_name(char *mac)
+{
+	DIR *dir;
+	struct dirent *entry;
+	FILE    *file;
+	char    *p, *q, *x;
+	char    *if_name = NULL;
+	char    buf[256];
+	char *kvp_net_dir = "/sys/class/net/";
+	char dev_id[100];
+	int i;
+
+	dir = opendir(kvp_net_dir);
+	if (dir == NULL)
+		return NULL;
+
+	memset(dev_id, 0, sizeof(dev_id));
+	strcat(dev_id, kvp_net_dir);
+	q = dev_id + strlen(kvp_net_dir);
+
+	while ((entry = readdir(dir)) != NULL) {
+		/*
+		 * Set the state for the next pass.
+		 */
+		*q = '\0';
+
+		strcat(dev_id, entry->d_name);
+		strcat(dev_id, "/address");
+
+		file = fopen(dev_id, "r");
+		if (file == NULL)
+			continue;
+
+		p = fgets(buf, sizeof(buf), file);
+		if (p) {
+			x = strchr(p, '\n');
+			if (x)
+				*x = '\0';
+
+			for (i = 0; i < strlen(p); i++)
+				p[i] = toupper(p[i]);
+
+			if (!strcmp(p, mac)) {
+				/*
+				 * Found the MAC match; return the interface
+				 * name. The caller will free the memory.
+				 */
+				if_name = strdup(entry->d_name);
+				break;
+			}
+		}
+		fclose(file);
+	}
+
+	closedir(dir);
+	return if_name;
+}
+
+
 static void kvp_process_ipconfig_file(char *cmd,
 					char *config_buf, int len,
 					int element_size, int offset)
@@ -757,10 +820,10 @@ static int kvp_process_ip_address(void *addrp,
 	}
 
 	if ((length - *offset) < addr_length + 1)
-		return 1;
+		return HV_E_FAIL;
 	if (str == NULL) {
 		strcpy(buffer, "inet_ntop failed\n");
-		return 1;
+		return HV_E_FAIL;
 	}
 	if (*offset == 0)
 		strcpy(buffer, tmp);
@@ -804,7 +867,7 @@ kvp_get_ip_info(int family, char *if_name, int op,
 
 	if (getifaddrs(&ifap)) {
 		strcpy(buffer, "getifaddrs failed\n");
-		return 1;
+		return HV_E_FAIL;
 	}
 
 	curp = ifap;
@@ -1392,6 +1455,31 @@ int main(void)
 		}
 
 		switch (op) {
+		case KVP_OP_GET_IP_INFO:
+			kvp_ip_val = &hv_msg->body.kvp_ip_val;
+			if_name =
+			kvp_mac_to_if_name((char *)kvp_ip_val->adapter_id);
+
+			if (if_name == NULL) {
+				/*
+				 * We could not map the mac address to an
+				 * interface name; return error.
+				 */
+				*((int *)(&hv_msg->kvp_hdr.operation)) =
+					HV_E_FAIL;
+				break;
+			}
+			error = kvp_get_ip_info(
+						0, if_name, KVP_OP_GET_IP_INFO,
+						kvp_ip_val,
+						(MAX_IP_ADDR_SIZE * 2));
+
+			if (error)
+				*((int *)(&hv_msg->kvp_hdr.operation)) = error;
+
+			free(if_name);
+			break;
+
 		case KVP_OP_SET_IP_INFO:
 			kvp_ip_val = &hv_msg->body.kvp_ip_val;
 			if_name = kvp_get_if_name(
-- 
1.7.4.1

[PATCH 16/17] Tools: hv: Get rid of some unused variables

[K. Y. Srinivasan]

Get rid of unused variables. I would like to thank Ben Hutchings
for pointing out the issue (ben@decadent.org.uk).

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 3bce574..e72d6db 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -206,7 +206,7 @@ static void kvp_update_mem_state(int pool)
 }
 static int kvp_file_init(void)
 {
-	int ret, fd;
+	int  fd;
 	FILE *filep;
 	size_t records_read;
 	__u8 *fname;
@@ -322,7 +322,6 @@ 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;
 	struct kvp_record *record;
 	int num_blocks;
-- 
1.7.4.1

[PATCH 17/17] Tools: hv: Correctly type string variables

[K. Y. Srinivasan]

Correctly type character strings. I would like to thank Ben Hutchings
for pointing out the issue (ben@decadent.org.uk).

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

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index e72d6db..3746033 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -96,8 +96,8 @@ static struct utsname uts_buf;
 #define ENTRIES_PER_BLOCK 50
 
 struct kvp_record {
-	__u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
-	__u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+	char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+	char value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
 };
 
 struct kvp_file_state {
@@ -105,7 +105,7 @@ struct kvp_file_state {
 	int num_blocks;
 	struct kvp_record *records;
 	int num_records;
-	__u8 fname[MAX_FILE_NAME];
+	char fname[MAX_FILE_NAME];
 };
 
 static struct kvp_file_state kvp_file_info[KVP_POOL_COUNT];
@@ -209,7 +209,7 @@ static int kvp_file_init(void)
 	int  fd;
 	FILE *filep;
 	size_t records_read;
-	__u8 *fname;
+	char *fname;
 	struct kvp_record *record;
 	struct kvp_record *readp;
 	int num_blocks;
-- 
1.7.4.1

Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[Stephen Hemminger]

On Tue, 24 Jul 2012 09:01:34 -0700
"K. Y. Srinivasan" <kys@microsoft.com> wrote:

> +	memset(cmd, 0, sizeof(cmd));
> +	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
> +	strcat(cmd, if_name);
> +	strcat(cmd, " | awk '/default/ {print $3 }'");


Much simpler method:

ip route show match 0/0

RE: [PATCH 09/17] Tools: hv: Represent the ipv6 mask using CIDR notation

[KY Srinivasan]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, July 24, 2012 12:01 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org; ben@decadent.org.uk
> Subject: Re: [PATCH 09/17] Tools: hv: Represent the ipv6 mask using CIDR
> notation
> 
> On Tue, Jul 24, 2012 at 09:01:33AM -0700, K. Y. Srinivasan wrote:
> > Transform ipv6 subnet information to CIDR notation.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  tools/hv/hv_kvp_daemon.c |   45
> +++++++++++++++++++++++++++++++++++----------
> >  1 files changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > index 2c24ebf..007e698 100644
> > --- a/tools/hv/hv_kvp_daemon.c
> > +++ b/tools/hv/hv_kvp_daemon.c
> > @@ -491,6 +491,15 @@ done:
> >  	return;
> >  }
> >
> > +static unsigned int hweight32(unsigned int *w)
> > +{
> > +	unsigned int res = *w - ((*w >> 1) & 0x55555555);
> > +	res = (res & 0x33333333) + ((res >> 2) & 0x33333333);
> > +	res = (res + (res >> 4)) & 0x0F0F0F0F;
> > +	res = res + (res >> 8);
> > +	return (res + (res >> 16)) & 0x000000FF;
> > +}
> 
> What's wrong with the hweight32 version we have already in
> <include/asm-generic/bitops/const_hweight.h> which you can include by
> simply by including <asm-generic/bitops.h>?

Boris,

This code is a user-level daemon that will be compiled outside of the kernel.
I did not want to include Kernel header files for this one function and deal with
all the dependencies that will have to be dealt with.

Regards,

K. Y

Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[Olaf Hering]

On Tue, Jul 24, Stephen Hemminger wrote:

> On Tue, 24 Jul 2012 09:01:34 -0700
> "K. Y. Srinivasan" <kys@microsoft.com> wrote:
> 
> > +	memset(cmd, 0, sizeof(cmd));
> > +	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
> > +	strcat(cmd, if_name);
> > +	strcat(cmd, " | awk '/default/ {print $3 }'");
> 
> 
> Much simpler method:
> 
> ip route show match 0/0

This also has the benefit that ip is not called with absolute path, now
that distros move binaries around.

Olaf

Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[Stephen Hemminger]

On Tue, 24 Jul 2012 18:53:59 +0200
Olaf Hering <olaf@aepfle.de> wrote:

> On Tue, Jul 24, Stephen Hemminger wrote:
> 
> > On Tue, 24 Jul 2012 09:01:34 -0700
> > "K. Y. Srinivasan" <kys@microsoft.com> wrote:
> > 
> > > +	memset(cmd, 0, sizeof(cmd));
> > > +	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
> > > +	strcat(cmd, if_name);
> > > +	strcat(cmd, " | awk '/default/ {print $3 }'");
> > 
> > 
> > Much simpler method:
> > 
> > ip route show match 0/0
> 
> This also has the benefit that ip is not called with absolute path, now
> that distros move binaries around.
> 
> Olaf

It is also not hard to do the same thing with a little function
using libmnl

Re: [PATCH 09/17] Tools: hv: Represent the ipv6 mask using CIDR notation

[Borislav Petkov]

On Tue, Jul 24, 2012 at 04:53:50PM +0000, KY Srinivasan wrote:
> This code is a user-level daemon that will be compiled outside of the
> kernel. I did not want to include Kernel header files for this one
> function and deal with all the dependencies that will have to be dealt
> with.

Ah, I missed the tools/ prefix in the patch, sorry.

Well, FWIW, we have hweight32 in <tools/perf/util/hweight.c> too and
there was a patchset preparing a generic kernel tools library where
tools can share functions but the author doesn't have time to dust the
patches off and get it upstream... :-).

I guess having a local function is the easiest for now.

Thanks.

-- 
Regards/Gruss,
Boris.

RE: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Tuesday, July 24, 2012 12:54 PM
> To: Stephen Hemminger
> Cc: KY Srinivasan; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; apw@canonical.com;
> netdev@vger.kernel.org; ben@decadent.org.uk
> Subject: Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information
> 
> On Tue, Jul 24, Stephen Hemminger wrote:
> 
> > On Tue, 24 Jul 2012 09:01:34 -0700
> > "K. Y. Srinivasan" <kys@microsoft.com> wrote:
> >
> > > +	memset(cmd, 0, sizeof(cmd));
> > > +	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
> > > +	strcat(cmd, if_name);
> > > +	strcat(cmd, " | awk '/default/ {print $3 }'");
> >
> >
> > Much simpler method:
> >
> > ip route show match 0/0
> 
> This also has the benefit that ip is not called with absolute path, now
> that distros move binaries around.

I could have chosen to not specify the full path for the ip command and for that
matter all the external scripts I invoke from the KVP daemon. Do you mind if I 
submitted a patch to get rid of the absolute paths in this code.

Stephen's suggestion is clearly simpler (I don't need to invoke awk to filter what 
we want). Steve, I could make this change as well as an additional patch.

Regards,

K. Y 

Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[Dan Williams]

On Tue, 2012-07-24 at 09:56 -0700, Stephen Hemminger wrote:
> On Tue, 24 Jul 2012 18:53:59 +0200
> Olaf Hering <olaf@aepfle.de> wrote:
> 
> > On Tue, Jul 24, Stephen Hemminger wrote:
> > 
> > > On Tue, 24 Jul 2012 09:01:34 -0700
> > > "K. Y. Srinivasan" <kys@microsoft.com> wrote:
> > > 
> > > > +	memset(cmd, 0, sizeof(cmd));
> > > > +	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
> > > > +	strcat(cmd, if_name);
> > > > +	strcat(cmd, " | awk '/default/ {print $3 }'");
> > > 
> > > 
> > > Much simpler method:
> > > 
> > > ip route show match 0/0
> > 
> > This also has the benefit that ip is not called with absolute path, now
> > that distros move binaries around.
> > 
> > Olaf
> 
> It is also not hard to do the same thing with a little function
> using libmnl

Yeah seriously, netlink anyone?  You'll even get nicer error reporting
that way.

Dan

RE: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information

[KY Srinivasan]

> -----Original Message-----
> From: Dan Williams [mailto:dcbw@redhat.com]
> Sent: Tuesday, July 24, 2012 2:37 PM
> To: Stephen Hemminger
> Cc: Olaf Hering; KY Srinivasan; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; apw@canonical.com; netdev@vger.kernel.org;
> ben@decadent.org.uk
> Subject: Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information
> 
> On Tue, 2012-07-24 at 09:56 -0700, Stephen Hemminger wrote:
> > On Tue, 24 Jul 2012 18:53:59 +0200
> > Olaf Hering <olaf@aepfle.de> wrote:
> >
> > > On Tue, Jul 24, Stephen Hemminger wrote:
> > >
> > > > On Tue, 24 Jul 2012 09:01:34 -0700
> > > > "K. Y. Srinivasan" <kys@microsoft.com> wrote:
> > > >
> > > > > +	memset(cmd, 0, sizeof(cmd));
> > > > > +	strcat(cmd, "/sbin/ip -f inet  route | grep -w ");
> > > > > +	strcat(cmd, if_name);
> > > > > +	strcat(cmd, " | awk '/default/ {print $3 }'");
> > > >
> > > >
> > > > Much simpler method:
> > > >
> > > > ip route show match 0/0
> > >
> > > This also has the benefit that ip is not called with absolute path, now
> > > that distros move binaries around.
> > >
> > > Olaf
> >
> > It is also not hard to do the same thing with a little function
> > using libmnl
> 
> Yeah seriously, netlink anyone?  You'll even get nicer error reporting
> that way.

While I will be the first admit that using C API is always better (in C code),
in this particular instance I am not so sure. All I am doing is retrieving information
on default gateways. If there is an error, that is ok and this won't be reported
back to the host. Using the ip command significantly simplifies the code here.

Regards,

K. Y  

Re: [PATCH 11/17] Tools: hv: Gather DNS information

[Ben Hutchings]

On Tue, Jul 24, 2012 at 09:01:35AM -0700, K. Y. Srinivasan wrote:
> Now gather DNS information. This information cannot be gathered in
> a distro independent fashion. Invoke an external script (that can be
> distro dependent) to gather the DNS information.
[...]
> +	memset(cmd, 0, sizeof(cmd));
> +	strcat(cmd, "/sbin/hv_get_dns_info ");
> +	strcat(cmd, if_name);
[...]

This is a weird way to build a string; why are you not using
snprintf()?  Not to mention that interface names can contain several
characters that are special to the shell - in fact the only disallowed
characters are / and whitespace.
 
Also, the external script will not be useful to anything other than
hv_kvp_daemon, so it probably belongs somewhere under /usr/share.
 
Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP

[Ben Hutchings]

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

On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> In preparation to implementing IP injection, cleanup the way we propagate
> and handle errors both in the driver as well as in the user level daemon.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/hv/hv_kvp.c      |  112 +++++++++++++++++++++++++++++++++++++---------
>  include/linux/hyperv.h   |   17 +++++---
>  tools/hv/hv_kvp_daemon.c |   70 +++++++++++++++-------------
>  3 files changed, 138 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 0012eed..9b7fc4a 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
[...]
> @@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
>  {
>  	struct hv_kvp_msg *message;
>  	struct hv_kvp_msg_enumerate *data;
> +	int	error = 0;
>  
>  	message = (struct hv_kvp_msg *)msg->data;
> -	switch (message->kvp_hdr.operation) {
> +
> +	/*
> +	 * If we are negotiating the version information
> +	 * with the daemon; handle that first.
> +	 */
> +
> +	if (in_hand_shake) {
> +		if (kvp_handle_handshake(message))
> +			in_hand_shake = false;
> +		return;
> +	}
> +
> +	/*
> +	 * Based on the version of the daemon, we propagate errors from the
> +	 * daemon differently.
> +	 */
> +
> +	data = &message->body.kvp_enum_data;
> +
> +	switch (dm_reg_value) {
>  	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);
> +		/*
> +		 * Null string is used to pass back error condition.
> +		 */
> +		if (!strlen(data->data.key))

Do we know that the key is null-terminated here?  Shouldn't we just
check whether data->data.key[0] == 0?

> +			error = HV_S_CONT;
>  		break;
>  
> -	default:
> -		data = &message->body.kvp_enum_data;
> +	case KVP_OP_REGISTER1:
>  		/*
> -		 * Complete the transaction by forwarding the key value
> -		 * to the host. But first, cancel the timeout.
> +		 * We use the message header information from
> +		 * the user level daemon to transmit errors.
>  		 */
> -		if (cancel_delayed_work_sync(&kvp_work))
> -			kvp_respond_to_host(data->data.key,
> -					 data->data.value,
> -					!strlen(data->data.key));
> +		error = *((int *)(&message->kvp_hdr.operation));
[...]

What's with the casting (repeated in many other places)?  Wouldn't it be
better to redefine struct hv_kvp_msg to start with something like:

	union {
		struct hv_kvp_hdr	request;
		int			error;
	} kvp_hdr;

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 08/17] Tools: hv: Gather subnet information

[Ben Hutchings]

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

On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> Now gather sub-net information for the specified interface.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  tools/hv/hv_kvp_daemon.c |   31 +++++++++++++++++++++++++++++--
>  1 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 79eb130..2c24ebf 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -534,6 +534,7 @@ kvp_get_ip_address(int family, char *if_name, int op,
>  	struct ifaddrs *ifap;
>  	struct ifaddrs *curp;
>  	int offset = 0;
> +	int sn_offset = 0;
>  	const char *str;
>  	int error = 0;
>  	char *buffer;
> @@ -594,12 +595,38 @@ kvp_get_ip_address(int family, char *if_name, int op,
>  			 * Gather info other than the IP address.
>  			 * IP address info will be gathered later.
>  			 */
> -			if (curp->ifa_addr->sa_family == AF_INET)
> +			if (curp->ifa_addr->sa_family == AF_INET) {
>  				ip_buffer->addr_family |= ADDR_FAMILY_IPV4;
> -			else
> +				/*
> +				 * Get subnet info.
> +				 */
> +				error = kvp_process_ip_address(
> +							curp->ifa_netmask,
> +							AF_INET,
> +							(char *)
> +							ip_buffer->sub_net,
> +							length,
> +							&sn_offset);
[...]

This is barely readable; why don't you indent the arguments by just one
extra tab?

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[Ben Hutchings]

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

On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures the
> specified interface based on the given configuration. Since configuring
> an interface is very distro specific, we invoke an external script to
> configure the interface.
[...]
> +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> +{
> +	char str[256];
> +	int error;
> +
> +	memset(str, 0, sizeof(str));
> +	strcat(str, s1);
> +	if (s2 != NULL)
> +		strcat(str, s2);
> +	strcat(str, "=");
> +	strcat(str, s3);
> +	strcat(str, "\n");
> +
> +	error = fputs(str, f);

This style of string pasting is crazy; have you never heard of
fprintf()?

[...]
> +	/*
> +	 * Set the configuration for the specified interface with
> +	 * the information provided. Since there is no standard
> +	 * way to configure an interface, we will have an external
> +	 * script that does the job of configuring the interface and
> +	 * flushing the configuration.
> +	 *
> +	 * The parameters passed to this external script are:
> +	 * 1. A configuration file that has the specified configuration.
> +	 *
> +	 * We will embed the name of the interface in the configuration
> +	 * file: ifcfg-ethx (where ethx is the interface name).
> +	 *
> +	 * Here is the format of the ip configuration file:
> +	 *
> +	 * HWADDR=macaddr

Is the interface supposed to be matched by name or by MAC address?

> +	 * BOOTPROTO=dhcp (dhcp enabled for the interface)

The BOOTPROTO line may or may not appear.

> +	 * NM_CONTROLLED=no (this interface will not be controlled by NM)
> +	 * PEERDNS=yes

I wonder what the point is of including constant lines in the file.
What is the external script supposed to do if it these apparent
constants change in future?

> +	 * IPADDR_x=ipaddr
> +	 * NETMASK_x=netmask
> +	 * GATEWAY_x=gateway
> +	 * DNSx=dns

A strangely familiar format...

> +	 * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> +	 * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> +	 * IPV6NETMASK.
> +	 */
> +
> +	memset(if_file, 0, sizeof(if_file));
> +	strcat(if_file, "/var/opt/hyperv/ifcfg-");

Like I said before about the key-value files, this should be under
/var/lib if the daemon is included in a distribution.  You should
perhaps use a macro for the "/var/opt" part so it can be overridden
depending on whether it's built as a distribution or add-on package.

> +	strcat(if_file, if_name);
> +
> +	file = fopen(if_file, "w");
> +
> +	if (file == NULL) {
> +		syslog(LOG_ERR, "Failed to open config file");
> +		return HV_E_FAIL;
> +	}
> +
> +	/*
> +	 * First write out the MAC address.
> +	 */
> +
> +	mac_addr = kvp_if_name_to_mac(if_name);
> +	if (mac_addr == NULL) {
> +		error = HV_E_FAIL;
> +		goto setval_error;
> +	}
> +
> +	error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> +	if (error)
> +		goto setval_error;
> +
> +	error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> +	if (error)
> +		goto setval_error;
> +
> +	error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> +	if (error)
> +		goto setval_error;
[...]

This line isn't mentioned in the above comment.

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

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

Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP

[Olaf Hering]

On Tue, Jul 24, K. Y. Srinivasan wrote:


> +++ b/drivers/hv/hv_kvp.c
> @@ -48,13 +48,24 @@ static struct {
>  	void *kvp_context; /* for the channel callback */
>  } kvp_transaction;
>  
> +/*
> + * Before we can accept KVP messages from the host, we need
> + * to handshake with the user level daemon. This state tarcks

tracks

> + * if we are in the handshake phase.
> + */

> -		 * Something failed or the we have timedout;
> +		 * Something failed or  we have timedout;

extra space

RE: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP

[KY Srinivasan]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Tuesday, July 24, 2012 9:11 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
> 
> On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > In preparation to implementing IP injection, cleanup the way we propagate
> > and handle errors both in the driver as well as in the user level daemon.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  drivers/hv/hv_kvp.c      |  112 +++++++++++++++++++++++++++++++++++++-
> --------
> >  include/linux/hyperv.h   |   17 +++++---
> >  tools/hv/hv_kvp_daemon.c |   70 +++++++++++++++-------------
> >  3 files changed, 138 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > index 0012eed..9b7fc4a 100644
> > --- a/drivers/hv/hv_kvp.c
> > +++ b/drivers/hv/hv_kvp.c
> [...]
> > @@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct
> netlink_skb_parms *nsp)
> >  {
> >  	struct hv_kvp_msg *message;
> >  	struct hv_kvp_msg_enumerate *data;
> > +	int	error = 0;
> >
> >  	message = (struct hv_kvp_msg *)msg->data;
> > -	switch (message->kvp_hdr.operation) {
> > +
> > +	/*
> > +	 * If we are negotiating the version information
> > +	 * with the daemon; handle that first.
> > +	 */
> > +
> > +	if (in_hand_shake) {
> > +		if (kvp_handle_handshake(message))
> > +			in_hand_shake = false;
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Based on the version of the daemon, we propagate errors from the
> > +	 * daemon differently.
> > +	 */
> > +
> > +	data = &message->body.kvp_enum_data;
> > +
> > +	switch (dm_reg_value) {
> >  	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);
> > +		/*
> > +		 * Null string is used to pass back error condition.
> > +		 */
> > +		if (!strlen(data->data.key))
> 
> Do we know that the key is null-terminated here?  Shouldn't we just
> check whether data->data.key[0] == 0?

Yes, currently we do return null string to indicate error.

> 
> > +			error = HV_S_CONT;
> >  		break;
> >
> > -	default:
> > -		data = &message->body.kvp_enum_data;
> > +	case KVP_OP_REGISTER1:
> >  		/*
> > -		 * Complete the transaction by forwarding the key value
> > -		 * to the host. But first, cancel the timeout.
> > +		 * We use the message header information from
> > +		 * the user level daemon to transmit errors.
> >  		 */
> > -		if (cancel_delayed_work_sync(&kvp_work))
> > -			kvp_respond_to_host(data->data.key,
> > -					 data->data.value,
> > -					!strlen(data->data.key));
> > +		error = *((int *)(&message->kvp_hdr.operation));
> [...]
> 
> What's with the casting (repeated in many other places)?  Wouldn't it be
> better to redefine struct hv_kvp_msg to start with something like:
> 
> 	union {
> 		struct hv_kvp_hdr	request;
> 		int			error;
> 	} kvp_hdr;

Agreed; will do.
> 
> Ben.
> 
> --
> Ben Hutchings
> If more than one person is responsible for a bug, no one is at fault.

RE: [PATCH 08/17] Tools: hv: Gather subnet information

[KY Srinivasan]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Tuesday, July 24, 2012 9:14 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 08/17] Tools: hv: Gather subnet information
> 
> On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > Now gather sub-net information for the specified interface.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  tools/hv/hv_kvp_daemon.c |   31 +++++++++++++++++++++++++++++--
> >  1 files changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > index 79eb130..2c24ebf 100644
> > --- a/tools/hv/hv_kvp_daemon.c
> > +++ b/tools/hv/hv_kvp_daemon.c
> > @@ -534,6 +534,7 @@ kvp_get_ip_address(int family, char *if_name, int op,
> >  	struct ifaddrs *ifap;
> >  	struct ifaddrs *curp;
> >  	int offset = 0;
> > +	int sn_offset = 0;
> >  	const char *str;
> >  	int error = 0;
> >  	char *buffer;
> > @@ -594,12 +595,38 @@ kvp_get_ip_address(int family, char *if_name, int op,
> >  			 * Gather info other than the IP address.
> >  			 * IP address info will be gathered later.
> >  			 */
> > -			if (curp->ifa_addr->sa_family == AF_INET)
> > +			if (curp->ifa_addr->sa_family == AF_INET) {
> >  				ip_buffer->addr_family |= ADDR_FAMILY_IPV4;
> > -			else
> > +				/*
> > +				 * Get subnet info.
> > +				 */
> > +				error = kvp_process_ip_address(
> > +							curp->ifa_netmask,
> > +							AF_INET,
> > +							(char *)
> > +							ip_buffer->sub_net,
> > +							length,
> > +							&sn_offset);
> [...]
> 
> This is barely readable; why don't you indent the arguments by just one
> extra tab?

Will do.

Regards,

K. Y

Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP

[Ben Hutchings]

On Wed, Jul 25, 2012 at 02:10:05PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > Sent: Tuesday, July 24, 2012 9:11 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> > apw@canonical.com; netdev@vger.kernel.org
> > Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
> > 
> > On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > > In preparation to implementing IP injection, cleanup the way we propagate
> > > and handle errors both in the driver as well as in the user level daemon.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > >  drivers/hv/hv_kvp.c      |  112 +++++++++++++++++++++++++++++++++++++-
> > --------
> > >  include/linux/hyperv.h   |   17 +++++---
> > >  tools/hv/hv_kvp_daemon.c |   70 +++++++++++++++-------------
> > >  3 files changed, 138 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > > index 0012eed..9b7fc4a 100644
> > > --- a/drivers/hv/hv_kvp.c
> > > +++ b/drivers/hv/hv_kvp.c
> > [...]
> > > @@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct
> > netlink_skb_parms *nsp)
> > >  {
> > >  	struct hv_kvp_msg *message;
> > >  	struct hv_kvp_msg_enumerate *data;
> > > +	int	error = 0;
> > >
> > >  	message = (struct hv_kvp_msg *)msg->data;
> > > -	switch (message->kvp_hdr.operation) {
> > > +
> > > +	/*
> > > +	 * If we are negotiating the version information
> > > +	 * with the daemon; handle that first.
> > > +	 */
> > > +
> > > +	if (in_hand_shake) {
> > > +		if (kvp_handle_handshake(message))
> > > +			in_hand_shake = false;
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Based on the version of the daemon, we propagate errors from the
> > > +	 * daemon differently.
> > > +	 */
> > > +
> > > +	data = &message->body.kvp_enum_data;
> > > +
> > > +	switch (dm_reg_value) {
> > >  	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);
> > > +		/*
> > > +		 * Null string is used to pass back error condition.
> > > +		 */
> > > +		if (!strlen(data->data.key))
> > 
> > Do we know that the key is null-terminated here?  Shouldn't we just
> > check whether data->data.key[0] == 0?
> 
> Yes, currently we do return null string to indicate error.
[...]

So the kernel should assume userland input is always valid?

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[KY Srinivasan]

Ben,

At the outset I want to thank you for taking the time to review this code. Given that Greg
has indicated that he will not be able to look at this patch set till 3.6 and the nature of review
comments I have gotten from you and others, I will re-spin this patch set to address all the
comments I have gotten to date. Specific responses to your comments are in-line.

Regards,

K. Y 

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Tuesday, July 24, 2012 9:25 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
> 
> On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures
> the
> > specified interface based on the given configuration. Since configuring
> > an interface is very distro specific, we invoke an external script to
> > configure the interface.
> [...]
> > +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> > +{
> > +	char str[256];
> > +	int error;
> > +
> > +	memset(str, 0, sizeof(str));
> > +	strcat(str, s1);
> > +	if (s2 != NULL)
> > +		strcat(str, s2);
> > +	strcat(str, "=");
> > +	strcat(str, s3);
> > +	strcat(str, "\n");
> > +
> > +	error = fputs(str, f);
> 
> This style of string pasting is crazy; have you never heard of
> fprintf()?
> 
> [...]
> > +	/*
> > +	 * Set the configuration for the specified interface with
> > +	 * the information provided. Since there is no standard
> > +	 * way to configure an interface, we will have an external
> > +	 * script that does the job of configuring the interface and
> > +	 * flushing the configuration.
> > +	 *
> > +	 * The parameters passed to this external script are:
> > +	 * 1. A configuration file that has the specified configuration.
> > +	 *
> > +	 * We will embed the name of the interface in the configuration
> > +	 * file: ifcfg-ethx (where ethx is the interface name).
> > +	 *
> > +	 * Here is the format of the ip configuration file:
> > +	 *
> > +	 * HWADDR=macaddr
> 
> Is the interface supposed to be matched by name or by MAC address?

I do not dictate that. My plan was to package all the information I have about
the interface and the desired configuration in a file and invoke the external
distro specific script to do its magic. This external  script is free to ignore 
what it does not need.

> 
> > +	 * BOOTPROTO=dhcp (dhcp enabled for the interface)
> 
> The BOOTPROTO line may or may not appear.
> 
> > +	 * NM_CONTROLLED=no (this interface will not be controlled by NM)
> > +	 * PEERDNS=yes
> 
> I wonder what the point is of including constant lines in the file.
> What is the external script supposed to do if it these apparent
> constants change in future?

As you can see, I did my testing on a RHEL system and I was too lazy to create
a RHEL specific config file in the external script and so I ended up creating pretty much
the config file needed by RHEL in this daemon. All the external script had to do was to
simply copy this file into the right location and bring up the interface. So, if you prefer
that we not populate the config file with these constant lines, I can get rid of them.
As I noted earlier external scripts may not choose to use all the information in the file
that this daemon generates. What I have done here, simplifies the external script
at least for one distro.

> 
> > +	 * IPADDR_x=ipaddr
> > +	 * NETMASK_x=netmask
> > +	 * GATEWAY_x=gateway
> > +	 * DNSx=dns
> 
> A strangely familiar format...
> 
> > +	 * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> > +	 * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> > +	 * IPV6NETMASK.
> > +	 */
> > +
> > +	memset(if_file, 0, sizeof(if_file));
> > +	strcat(if_file, "/var/opt/hyperv/ifcfg-");
> 
> Like I said before about the key-value files, this should be under
> /var/lib if the daemon is included in a distribution.  You should
> perhaps use a macro for the "/var/opt" part so it can be overridden
> depending on whether it's built as a distribution or add-on package.

I will make this a macro.

> 
> > +	strcat(if_file, if_name);
> > +
> > +	file = fopen(if_file, "w");
> > +
> > +	if (file == NULL) {
> > +		syslog(LOG_ERR, "Failed to open config file");
> > +		return HV_E_FAIL;
> > +	}
> > +
> > +	/*
> > +	 * First write out the MAC address.
> > +	 */
> > +
> > +	mac_addr = kvp_if_name_to_mac(if_name);
> > +	if (mac_addr == NULL) {
> > +		error = HV_E_FAIL;
> > +		goto setval_error;
> > +	}
> > +
> > +	error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> > +	if (error)
> > +		goto setval_error;
> [...]
> 
> This line isn't mentioned in the above comment.
> 
> Ben.
> 
> --
> Ben Hutchings
> If more than one person is responsible for a bug, no one is at fault.

RE: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP

[KY Srinivasan]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Wednesday, July 25, 2012 10:47 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
> 
> On Wed, Jul 25, 2012 at 02:10:05PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > > Sent: Tuesday, July 24, 2012 9:11 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> > > apw@canonical.com; netdev@vger.kernel.org
> > > Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
> > >
> > > On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > > > In preparation to implementing IP injection, cleanup the way we propagate
> > > > and handle errors both in the driver as well as in the user level daemon.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > ---
> > > >  drivers/hv/hv_kvp.c      |  112
> +++++++++++++++++++++++++++++++++++++-
> > > --------
> > > >  include/linux/hyperv.h   |   17 +++++---
> > > >  tools/hv/hv_kvp_daemon.c |   70 +++++++++++++++-------------
> > > >  3 files changed, 138 insertions(+), 61 deletions(-)
> > > >
> > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > > > index 0012eed..9b7fc4a 100644
> > > > --- a/drivers/hv/hv_kvp.c
> > > > +++ b/drivers/hv/hv_kvp.c
> > > [...]
> > > > @@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct
> > > netlink_skb_parms *nsp)
> > > >  {
> > > >  	struct hv_kvp_msg *message;
> > > >  	struct hv_kvp_msg_enumerate *data;
> > > > +	int	error = 0;
> > > >
> > > >  	message = (struct hv_kvp_msg *)msg->data;
> > > > -	switch (message->kvp_hdr.operation) {
> > > > +
> > > > +	/*
> > > > +	 * If we are negotiating the version information
> > > > +	 * with the daemon; handle that first.
> > > > +	 */
> > > > +
> > > > +	if (in_hand_shake) {
> > > > +		if (kvp_handle_handshake(message))
> > > > +			in_hand_shake = false;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Based on the version of the daemon, we propagate errors from the
> > > > +	 * daemon differently.
> > > > +	 */
> > > > +
> > > > +	data = &message->body.kvp_enum_data;
> > > > +
> > > > +	switch (dm_reg_value) {
> > > >  	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);
> > > > +		/*
> > > > +		 * Null string is used to pass back error condition.
> > > > +		 */
> > > > +		if (!strlen(data->data.key))
> > >
> > > Do we know that the key is null-terminated here?  Shouldn't we just
> > > check whether data->data.key[0] == 0?
> >
> > Yes, currently we do return null string to indicate error.
> [...]
> 
> So the kernel should assume userland input is always valid?

Good point! This is the existing code and this patch-set cleans up all
the error handling and would not have this problem.

K. Y

RE: [PATCH 00/17] drivers: hv: kvp

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, July 24, 2012 11:54 AM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; olaf@aepfle.de; apw@canonical.com;
> netdev@vger.kernel.org; ben@decadent.org.uk
> Subject: Re: [PATCH 00/17] drivers: hv: kvp
> 
> On Tue, Jul 24, 2012 at 09:01:12AM -0700, K. Y. Srinivasan wrote:
> > This patchset expands the KVP (Key Value Pair) functionality to
> > implement the mechanism to GET/SET IP addresses in the guest. This
> > functionality is used in Windows Server 2012 to implement VM
> > replication functionality. The way IP configuration information
> > is managed is distro specific. Based on the feedback I have gotten
> > from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
> > distro specific code from this patch-set. Most of the GET operation
> > can be implemented in a way that is completely distro independent and
> > I have implemented that as such and is included in this patch-set.
> > Some of the attributes that can only be fetched in a distro
> > dependent way as well the mechanism for configuring an interface
> > (the SET operation) that is clearly distro specific is to be
> > implemented via external scripts that will be invoked via the KVP
> > code. We define here the interface to these scripts.
> >
> > Adding support for IP injection resulted in some changes to the
> > protocol between the user level daemon and the kernel driver.
> > These changes have been implemented in way that would retain
> > compatibility with older daemons. I would like to thank Olaf and
> > Greg for pointing out the compatibility issue.
> 
> Due to this being the middle of the merge window, I will not be able to
> look at this until after 3.6-rc1 is out.

Thanks Greg. In the meantime, I have addressed all the comments that both Olaf
and Ben have posted on this patch-set. Since addressing these comments changed
some data structures, I think it will be best if you dropped this patch-set. I will post the 
updated patch-set shortly.

Regards,

K. Y

Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[Olaf Hering]

On Tue, Jul 24, K. Y. Srinivasan wrote:

> +static char *kvp_get_if_name(char *guid)
> +{
> +	DIR *dir;
> +	struct dirent *entry;
> +	FILE    *file;
> +	char    *p, *q, *x;
> +	char    *if_name = NULL;
> +	char    buf[256];
> +	char *kvp_net_dir = "/sys/class/net/";
> +	char dev_id[100];

Perhaps this can be written as char dev_id[100] = "short string";?

> +
> +	dir = opendir(kvp_net_dir);
> +	if (dir == NULL)
> +		return NULL;
> +
> +	memset(dev_id, 0, sizeof(dev_id));
> +	strcat(dev_id, kvp_net_dir);
> +	q = dev_id + strlen(kvp_net_dir);
> +
> +	while ((entry = readdir(dir)) != NULL) {
> +		/*
> +		 * Set the state for the next pass.
> +		 */
> +		*q = '\0';
> +		strcat(dev_id, entry->d_name);
> +		strcat(dev_id, "/device/device_id");

Maybe this (and other) strcat should be changed to snprintf?

> +
> +		file = fopen(dev_id, "r");
> +		if (file == NULL)
> +			continue;
> +
> +		p = fgets(buf, sizeof(buf), file);
> +		if (p) {
> +			x = strchr(p, '\n');
> +			if (x)
> +				*x = '\0';
> +
> +			if (!strcmp(p, guid)) {
> +				/*
> +				 * Found the guid match; return the interface
> +				 * name. The caller will free the memory.
> +				 */
> +				if_name = strdup(entry->d_name);
> +				break;

This will leave 'file' open.
I have seen it in some other place as well.

> +			}
> +		}
> +		fclose(file);
> +	}
> +
> +	closedir(dir);
> +	return if_name;
> +}
> +
> +/*
> + * Retrieve the MAC address given the interface name.
> + */
> +
> +static char *kvp_if_name_to_mac(char *if_name)
> +{
> +	FILE    *file;
> +	char    *p, *x;
> +	char    buf[256];
> +	char addr_file[100];
> +	int i;
> +	char *mac_addr = NULL;
> +
> +	memset(addr_file, 0, sizeof(addr_file));
> +	strcat(addr_file, "/sys/class/net/");
> +	strcat(addr_file, if_name);
> +	strcat(addr_file, "/address");

snprintf?

> +
> +	file = fopen(addr_file, "r");
> +	if (file == NULL)
> +		return NULL;
> +
> +	p = fgets(buf, sizeof(buf), file);
> +	if (p) {
> +		x = strchr(p, '\n');
> +		if (x)
> +			*x = '\0';
> +		for (i = 0; i < strlen(p); i++)
> +			p[i] = toupper(p[i]);
> +		mac_addr = strdup(p);
> +	}
> +
> +	fclose(file);
> +	return mac_addr;
> +}
> +
> +
>  static void kvp_process_ipconfig_file(char *cmd,
>  					char *config_buf, int len,
>  					int element_size, int offset)
> @@ -800,6 +910,314 @@ getaddr_done:
>  }
>  
>  
> +static int expand_ipv6(char *addr, int type)
> +{
> +	int ret;
> +	struct in6_addr v6_addr;
> +
> +	ret = inet_pton(AF_INET6, addr, &v6_addr);
> +
> +	if (ret != 1) {
> +		if (type == NETMASK)
> +			return 1;
> +		return 0;
> +	}
> +
> +	sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
> +		"%02x%02x:%02x%02x:%02x%02x",
> +		(int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],
> +		(int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],
> +		(int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],
> +		(int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],
> +		(int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],
> +		(int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],
> +		(int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],
> +		(int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);
> +
> +	return 1;
> +
> +}
> +
> +static int is_ipv4(char *addr)
> +{
> +	int ret;
> +	struct in_addr ipv4_addr;
> +
> +	ret = inet_pton(AF_INET, addr, &ipv4_addr);
> +
> +	if (ret == 1)
> +		return 1;
> +	return 0;
> +}
> +
> +static int parse_ip_val_buffer(char *in_buf, int *offset,
> +				char *out_buf, int out_len)
> +{
> +	char *x;
> +	char *start;
> +
> +	/*
> +	 * in_buf has sequence of characters that are seperated by
> +	 * the character ';'. The last sequence does not have the
> +	 * terminating ";" character.
> +	 */
> +	start = in_buf + *offset;
> +
> +	x = strchr(start, ';');
> +	if (x)
> +		*x = 0;
> +	else
> +		x = start + strlen(start);
> +
> +	if (strlen(start) != 0) {
> +		int i = 0;
> +		/*
> +		 * Get rid of leading spaces.
> +		 */
> +		while (start[i] == ' ')
> +			i++;
> +
> +		if ((x - start) <= out_len) {
> +			strcpy(out_buf, (start + i));
> +			*offset += (x - start) + 1;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> +{
> +	char str[256];
> +	int error;
> +
> +	memset(str, 0, sizeof(str));
> +	strcat(str, s1);
> +	if (s2 != NULL)
> +		strcat(str, s2);
> +	strcat(str, "=");
> +	strcat(str, s3);
> +	strcat(str, "\n");
> +
> +	error = fputs(str, f);
> +	if (error == EOF)
> +		return HV_E_FAIL;
> +
> +	return 0;
> +}
> +
> +
> +static int process_ip_string(FILE *f, char *ip_string, int type)
> +{
> +	int error = 0;
> +	char addr[INET6_ADDRSTRLEN];
> +	int i = 0;
> +	int j = 0;
> +	char str[256];
> +	char sub_str[10];
> +	int offset = 0;
> +
> +	memset(addr, 0, sizeof(addr));
> +
> +	while (parse_ip_val_buffer(ip_string, &offset, addr,
> +					(MAX_IP_ADDR_SIZE * 2))) {
> +		memset(sub_str, 0, sizeof(sub_str));
> +		memset(str, 0, sizeof(str));
> +
> +		if (is_ipv4(addr)) {
> +			switch (type) {
> +			case IPADDR:
> +				strcat(str, "IPADDR");
> +				break;
> +			case NETMASK:
> +				strcat(str, "NETMASK");
> +				break;
> +			case GATEWAY:
> +				strcat(str, "GATEWAY");
> +				break;
> +			case DNS:
> +				strcat(str, "DNS");
> +				break;
> +			}
> +			if (i != 0) {
> +				if (type != DNS)
> +					sprintf(sub_str, "_%d", i++);
> +				else
> +					sprintf(sub_str, "%d", ++i);
> +			} else if (type == DNS) {
> +				sprintf(sub_str, "%d", ++i);
> +			}
> +
> +
> +		} else if (expand_ipv6(addr, type)) {
> +			switch (type) {
> +			case IPADDR:
> +				strcat(str, "IPV6ADDR");
> +				break;
> +			case NETMASK:
> +				strcat(str, "IPV6NETMASK");
> +				break;
> +			case GATEWAY:
> +				strcat(str, "IPV6_DEFAULTGW");
> +				break;
> +			case DNS:
> +				strcat(str, "DNS");
> +				break;
> +			}
> +			if ((j != 0) || (type == DNS)) {
> +				if (type != DNS)
> +					sprintf(sub_str, "_%d", j++);
> +				else
> +					sprintf(sub_str, "%d", ++i);
> +			} else if (type == DNS) {
> +				sprintf(sub_str, "%d", ++i);
> +			}
> +		} else {
> +			return  HV_INVALIDARG;
> +		}
> +
> +		error = kvp_write_file(f, str, sub_str, addr);
> +		if (error)
> +			return error;
> +		memset(addr, 0, sizeof(addr));
> +	}
> +
> +	return 0;
> +}
> +
> +static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> +{
> +	int error = 0;
> +	char if_file[50];
> +	FILE *file;
> +	char cmd[512];
> +	char *mac_addr;
> +
> +	/*
> +	 * Set the configuration for the specified interface with
> +	 * the information provided. Since there is no standard
> +	 * way to configure an interface, we will have an external
> +	 * script that does the job of configuring the interface and
> +	 * flushing the configuration.
> +	 *
> +	 * The parameters passed to this external script are:
> +	 * 1. A configuration file that has the specified configuration.
> +	 *
> +	 * We will embed the name of the interface in the configuration
> +	 * file: ifcfg-ethx (where ethx is the interface name).
> +	 *
> +	 * Here is the format of the ip configuration file:
> +	 *
> +	 * HWADDR=macaddr
> +	 * BOOTPROTO=dhcp (dhcp enabled for the interface)
> +	 * NM_CONTROLLED=no (this interface will not be controlled by NM)
> +	 * PEERDNS=yes
> +	 * IPADDR_x=ipaddr
> +	 * NETMASK_x=netmask
> +	 * GATEWAY_x=gateway
> +	 * DNSx=dns
> +	 *
> +	 * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> +	 * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> +	 * IPV6NETMASK.
> +	 */
> +
> +	memset(if_file, 0, sizeof(if_file));
> +	strcat(if_file, "/var/opt/hyperv/ifcfg-");
> +	strcat(if_file, if_name);
> +
> +	file = fopen(if_file, "w");
> +
> +	if (file == NULL) {
> +		syslog(LOG_ERR, "Failed to open config file");
> +		return HV_E_FAIL;
> +	}
> +
> +	/*
> +	 * First write out the MAC address.
> +	 */
> +
> +	mac_addr = kvp_if_name_to_mac(if_name);
> +	if (mac_addr == NULL) {
> +		error = HV_E_FAIL;
> +		goto setval_error;
> +	}
> +
> +	error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> +	if (error)
> +		goto setval_error;
> +
> +	error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> +	if (error)
> +		goto setval_error;
> +
> +	error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> +	if (error)
> +		goto setval_error;
> +
> +	error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
> +	if (error)
> +		goto setval_error;
> +
> +	error = kvp_write_file(file, "PEERDNS", NULL, "yes");
> +	if (error)
> +		goto setval_error;
> +
> +	if (new_val->dhcp_enabled) {
> +		error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
> +		if (error)
> +			goto setval_error;
> +
> +		/*
> +		 * We are done!.
> +		 */
> +		goto setval_done;
> +	}
> +
> +	/*
> +	 * Write the configuration for ipaddress, netmask, gateway and
> +	 * name servers.
> +	 */
> +
> +	error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> +	if (error)
> +		goto setval_error;
> +
> +	error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> +	if (error)
> +		goto setval_error;
> +
> +	error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> +	if (error)
> +		goto setval_error;
> +
> +	error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> +	if (error)
> +		goto setval_error;
> +
> +setval_done:
> +	free(mac_addr);
> +	fclose(file);
> +
> +	/*
> +	 * Now that we have populated the configuration file,
> +	 * invoke the external script to do its magic.
> +	 */
> +
> +	memset(cmd, 0, sizeof(cmd));
> +	strcat(cmd, "/sbin/hv_set_ifconfig ");
> +	strcat(cmd, if_file);

The new patch should use "%s %s", not "%s%s" as format string.

Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[Olaf Hering]

On Tue, Jul 24, K. Y. Srinivasan wrote:

> +	/*
> +	 * Set the configuration for the specified interface with
> +	 * the information provided. Since there is no standard
> +	 * way to configure an interface, we will have an external
> +	 * script that does the job of configuring the interface and
> +	 * flushing the configuration.
> +	 *
> +	 * The parameters passed to this external script are:
> +	 * 1. A configuration file that has the specified configuration.

Maybe this should be written as 'A info file that has the requested
network configuration' or something like that.

> +	 *
> +	 * We will embed the name of the interface in the configuration
> +	 * file: ifcfg-ethx (where ethx is the interface name).

I think the intention here is to use the generated file as is. Depending
on the distro in the guest the file may need some processing. So I think
the actual interface name should also be part of the file.

> +	 *
> +	 * Here is the format of the ip configuration file:
> +	 *
> +	 * HWADDR=macaddr
> +	 * BOOTPROTO=dhcp (dhcp enabled for the interface)

While BOOTPROTO= is used in current network config files, its meaning
there is unrelated to what its meant here. Here it means DHCP=yes/no, so
I think the file should contain just that. And as the code is written
now BOOTPROTO= is optional, which is not mentioned in the comment.

> +	 * NM_CONTROLLED=no (this interface will not be controlled by NM)

I think this is not up to kvp_deamon to decide what controls the
interface. Maybe one day NM is sufficiently advanced technology that it
can cope with such requests?
The helper script should decide if the NM flag should be written to the
final config file.

> +	 * PEERDNS=yes
> +	 * IPADDR_x=ipaddr
> +	 * NETMASK_x=netmask
> +	 * GATEWAY_x=gateway

This should mention that its either IPADDR=ipaddr1 or 'IPADDR=ipaddr1;
IPADDR_1=ipaddr2'

> +	 * DNSx=dns

This should mention that its either DNS=ipaddr1 or 'DNS=ipaddr1;
DNS1=ipaddr2'

Olaf

RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Monday, July 30, 2012 2:03 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> ben@decadent.org.uk
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
> 
> On Tue, Jul 24, K. Y. Srinivasan wrote:
> 
> > +	/*
> > +	 * Set the configuration for the specified interface with
> > +	 * the information provided. Since there is no standard
> > +	 * way to configure an interface, we will have an external
> > +	 * script that does the job of configuring the interface and
> > +	 * flushing the configuration.
> > +	 *
> > +	 * The parameters passed to this external script are:
> > +	 * 1. A configuration file that has the specified configuration.
> 
> Maybe this should be written as 'A info file that has the requested
> network configuration' or something like that.

That is the idea. This configuration file simply reflects all the information we have
perhaps with some additional constant information. The script is free to ignore what
it does not need. 

> 
> > +	 *
> > +	 * We will embed the name of the interface in the configuration
> > +	 * file: ifcfg-ethx (where ethx is the interface name).
> 
> I think the intention here is to use the generated file as is. Depending
> on the distro in the guest the file may need some processing. So I think
> the actual interface name should also be part of the file.

That is not the intention although on some distros, the format of this file
may be closer to the distro specific configuration file than others. I will 
however include the name of the interface in the file as well.
 

> 
> > +	 *
> > +	 * Here is the format of the ip configuration file:
> > +	 *
> > +	 * HWADDR=macaddr
> > +	 * BOOTPROTO=dhcp (dhcp enabled for the interface)
> 
> While BOOTPROTO= is used in current network config files, its meaning
> there is unrelated to what its meant here. Here it means DHCP=yes/no, so
> I think the file should contain just that. And as the code is written
> now BOOTPROTO= is optional, which is not mentioned in the comment.

I will fix this.

> 
> > +	 * NM_CONTROLLED=no (this interface will not be controlled by NM)
> 
> I think this is not up to kvp_deamon to decide what controls the
> interface. Maybe one day NM is sufficiently advanced technology that it
> can cope with such requests?
> The helper script should decide if the NM flag should be written to the
> final config file.

As I noted earlier, the external script can choose to interpret the contents of this
file in a way that it makes sense for the distribution. Maybe, I will get rid of all the 
constant information.

> 
> > +	 * PEERDNS=yes
> > +	 * IPADDR_x=ipaddr
> > +	 * NETMASK_x=netmask
> > +	 * GATEWAY_x=gateway
> 
> This should mention that its either IPADDR=ipaddr1 or 'IPADDR=ipaddr1;
> IPADDR_1=ipaddr2'
> 
> > +	 * DNSx=dns
> 
> This should mention that its either DNS=ipaddr1 or 'DNS=ipaddr1;
> DNS1=ipaddr2'

I take it that you want the comments to be more explicit on the format.

> 
> Olaf
> 
> 

Thank you Olaf,

K. Y

RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Monday, July 30, 2012 1:33 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> ben@decadent.org.uk
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
> 
> On Tue, Jul 24, K. Y. Srinivasan wrote:
> 
> > +static char *kvp_get_if_name(char *guid)
> > +{
> > +	DIR *dir;
> > +	struct dirent *entry;
> > +	FILE    *file;
> > +	char    *p, *q, *x;
> > +	char    *if_name = NULL;
> > +	char    buf[256];
> > +	char *kvp_net_dir = "/sys/class/net/";
> > +	char dev_id[100];
> 
> Perhaps this can be written as char dev_id[100] = "short string";?

Why?

> 
> > +
> > +	dir = opendir(kvp_net_dir);
> > +	if (dir == NULL)
> > +		return NULL;
> > +
> > +	memset(dev_id, 0, sizeof(dev_id));
> > +	strcat(dev_id, kvp_net_dir);
> > +	q = dev_id + strlen(kvp_net_dir);
> > +
> > +	while ((entry = readdir(dir)) != NULL) {
> > +		/*
> > +		 * Set the state for the next pass.
> > +		 */
> > +		*q = '\0';
> > +		strcat(dev_id, entry->d_name);
> > +		strcat(dev_id, "/device/device_id");
> 
> Maybe this (and other) strcat should be changed to snprintf?

Already done.

> 


> > +
> > +		file = fopen(dev_id, "r");
> > +		if (file == NULL)
> > +			continue;
> > +
> > +		p = fgets(buf, sizeof(buf), file);
> > +		if (p) {
> > +			x = strchr(p, '\n');
> > +			if (x)
> > +				*x = '\0';
> > +
> > +			if (!strcmp(p, guid)) {
> > +				/*
> > +				 * Found the guid match; return the interface
> > +				 * name. The caller will free the memory.
> > +				 */
> > +				if_name = strdup(entry->d_name);
> > +				break;
> 
> This will leave 'file' open.
> I have seen it in some other place as well.

I will audit all instances and fix it.

> 
> > +			}
> > +		}
> > +		fclose(file);
> > +	}
> > +
> > +	closedir(dir);
> > +	return if_name;
> > +}
> > +
> > +/*
> > + * Retrieve the MAC address given the interface name.
> > + */
> > +
> > +static char *kvp_if_name_to_mac(char *if_name)
> > +{
> > +	FILE    *file;
> > +	char    *p, *x;
> > +	char    buf[256];
> > +	char addr_file[100];
> > +	int i;
> > +	char *mac_addr = NULL;
> > +
> > +	memset(addr_file, 0, sizeof(addr_file));
> > +	strcat(addr_file, "/sys/class/net/");
> > +	strcat(addr_file, if_name);
> > +	strcat(addr_file, "/address");
> 
> snprintf?

Already fixed.

> 
> > +
> > +	file = fopen(addr_file, "r");
> > +	if (file == NULL)
> > +		return NULL;
> > +
> > +	p = fgets(buf, sizeof(buf), file);
> > +	if (p) {
> > +		x = strchr(p, '\n');
> > +		if (x)
> > +			*x = '\0';
> > +		for (i = 0; i < strlen(p); i++)
> > +			p[i] = toupper(p[i]);
> > +		mac_addr = strdup(p);
> > +	}
> > +
> > +	fclose(file);
> > +	return mac_addr;
> > +}
> > +
> > +
> >  static void kvp_process_ipconfig_file(char *cmd,
> >  					char *config_buf, int len,
> >  					int element_size, int offset)
> > @@ -800,6 +910,314 @@ getaddr_done:
> >  }
> >
> >
> > +static int expand_ipv6(char *addr, int type)
> > +{
> > +	int ret;
> > +	struct in6_addr v6_addr;
> > +
> > +	ret = inet_pton(AF_INET6, addr, &v6_addr);
> > +
> > +	if (ret != 1) {
> > +		if (type == NETMASK)
> > +			return 1;
> > +		return 0;
> > +	}
> > +
> > +	sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
> > +		"%02x%02x:%02x%02x:%02x%02x",
> > +		(int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],
> > +		(int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],
> > +		(int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],
> > +		(int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],
> > +		(int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],
> > +		(int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],
> > +		(int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],
> > +		(int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);
> > +
> > +	return 1;
> > +
> > +}
> > +
> > +static int is_ipv4(char *addr)
> > +{
> > +	int ret;
> > +	struct in_addr ipv4_addr;
> > +
> > +	ret = inet_pton(AF_INET, addr, &ipv4_addr);
> > +
> > +	if (ret == 1)
> > +		return 1;
> > +	return 0;
> > +}
> > +
> > +static int parse_ip_val_buffer(char *in_buf, int *offset,
> > +				char *out_buf, int out_len)
> > +{
> > +	char *x;
> > +	char *start;
> > +
> > +	/*
> > +	 * in_buf has sequence of characters that are seperated by
> > +	 * the character ';'. The last sequence does not have the
> > +	 * terminating ";" character.
> > +	 */
> > +	start = in_buf + *offset;
> > +
> > +	x = strchr(start, ';');
> > +	if (x)
> > +		*x = 0;
> > +	else
> > +		x = start + strlen(start);
> > +
> > +	if (strlen(start) != 0) {
> > +		int i = 0;
> > +		/*
> > +		 * Get rid of leading spaces.
> > +		 */
> > +		while (start[i] == ' ')
> > +			i++;
> > +
> > +		if ((x - start) <= out_len) {
> > +			strcpy(out_buf, (start + i));
> > +			*offset += (x - start) + 1;
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> > +{
> > +	char str[256];
> > +	int error;
> > +
> > +	memset(str, 0, sizeof(str));
> > +	strcat(str, s1);
> > +	if (s2 != NULL)
> > +		strcat(str, s2);
> > +	strcat(str, "=");
> > +	strcat(str, s3);
> > +	strcat(str, "\n");
> > +
> > +	error = fputs(str, f);
> > +	if (error == EOF)
> > +		return HV_E_FAIL;
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static int process_ip_string(FILE *f, char *ip_string, int type)
> > +{
> > +	int error = 0;
> > +	char addr[INET6_ADDRSTRLEN];
> > +	int i = 0;
> > +	int j = 0;
> > +	char str[256];
> > +	char sub_str[10];
> > +	int offset = 0;
> > +
> > +	memset(addr, 0, sizeof(addr));
> > +
> > +	while (parse_ip_val_buffer(ip_string, &offset, addr,
> > +					(MAX_IP_ADDR_SIZE * 2))) {
> > +		memset(sub_str, 0, sizeof(sub_str));
> > +		memset(str, 0, sizeof(str));
> > +
> > +		if (is_ipv4(addr)) {
> > +			switch (type) {
> > +			case IPADDR:
> > +				strcat(str, "IPADDR");
> > +				break;
> > +			case NETMASK:
> > +				strcat(str, "NETMASK");
> > +				break;
> > +			case GATEWAY:
> > +				strcat(str, "GATEWAY");
> > +				break;
> > +			case DNS:
> > +				strcat(str, "DNS");
> > +				break;
> > +			}
> > +			if (i != 0) {
> > +				if (type != DNS)
> > +					sprintf(sub_str, "_%d", i++);
> > +				else
> > +					sprintf(sub_str, "%d", ++i);
> > +			} else if (type == DNS) {
> > +				sprintf(sub_str, "%d", ++i);
> > +			}
> > +
> > +
> > +		} else if (expand_ipv6(addr, type)) {
> > +			switch (type) {
> > +			case IPADDR:
> > +				strcat(str, "IPV6ADDR");
> > +				break;
> > +			case NETMASK:
> > +				strcat(str, "IPV6NETMASK");
> > +				break;
> > +			case GATEWAY:
> > +				strcat(str, "IPV6_DEFAULTGW");
> > +				break;
> > +			case DNS:
> > +				strcat(str, "DNS");
> > +				break;
> > +			}
> > +			if ((j != 0) || (type == DNS)) {
> > +				if (type != DNS)
> > +					sprintf(sub_str, "_%d", j++);
> > +				else
> > +					sprintf(sub_str, "%d", ++i);
> > +			} else if (type == DNS) {
> > +				sprintf(sub_str, "%d", ++i);
> > +			}
> > +		} else {
> > +			return  HV_INVALIDARG;
> > +		}
> > +
> > +		error = kvp_write_file(f, str, sub_str, addr);
> > +		if (error)
> > +			return error;
> > +		memset(addr, 0, sizeof(addr));
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value
> *new_val)
> > +{
> > +	int error = 0;
> > +	char if_file[50];
> > +	FILE *file;
> > +	char cmd[512];
> > +	char *mac_addr;
> > +
> > +	/*
> > +	 * Set the configuration for the specified interface with
> > +	 * the information provided. Since there is no standard
> > +	 * way to configure an interface, we will have an external
> > +	 * script that does the job of configuring the interface and
> > +	 * flushing the configuration.
> > +	 *
> > +	 * The parameters passed to this external script are:
> > +	 * 1. A configuration file that has the specified configuration.
> > +	 *
> > +	 * We will embed the name of the interface in the configuration
> > +	 * file: ifcfg-ethx (where ethx is the interface name).
> > +	 *
> > +	 * Here is the format of the ip configuration file:
> > +	 *
> > +	 * HWADDR=macaddr
> > +	 * BOOTPROTO=dhcp (dhcp enabled for the interface)
> > +	 * NM_CONTROLLED=no (this interface will not be controlled by NM)
> > +	 * PEERDNS=yes
> > +	 * IPADDR_x=ipaddr
> > +	 * NETMASK_x=netmask
> > +	 * GATEWAY_x=gateway
> > +	 * DNSx=dns
> > +	 *
> > +	 * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> > +	 * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> > +	 * IPV6NETMASK.
> > +	 */
> > +
> > +	memset(if_file, 0, sizeof(if_file));
> > +	strcat(if_file, "/var/opt/hyperv/ifcfg-");
> > +	strcat(if_file, if_name);
> > +
> > +	file = fopen(if_file, "w");
> > +
> > +	if (file == NULL) {
> > +		syslog(LOG_ERR, "Failed to open config file");
> > +		return HV_E_FAIL;
> > +	}
> > +
> > +	/*
> > +	 * First write out the MAC address.
> > +	 */
> > +
> > +	mac_addr = kvp_if_name_to_mac(if_name);
> > +	if (mac_addr == NULL) {
> > +		error = HV_E_FAIL;
> > +		goto setval_error;
> > +	}
> > +
> > +	error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = kvp_write_file(file, "PEERDNS", NULL, "yes");
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	if (new_val->dhcp_enabled) {
> > +		error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
> > +		if (error)
> > +			goto setval_error;
> > +
> > +		/*
> > +		 * We are done!.
> > +		 */
> > +		goto setval_done;
> > +	}
> > +
> > +	/*
> > +	 * Write the configuration for ipaddress, netmask, gateway and
> > +	 * name servers.
> > +	 */
> > +
> > +	error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> > +	if (error)
> > +		goto setval_error;
> > +
> > +	error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> > +	if (error)
> > +		goto setval_error;
> > +
> > +setval_done:
> > +	free(mac_addr);
> > +	fclose(file);
> > +
> > +	/*
> > +	 * Now that we have populated the configuration file,
> > +	 * invoke the external script to do its magic.
> > +	 */
> > +
> > +	memset(cmd, 0, sizeof(cmd));
> > +	strcat(cmd, "/sbin/hv_set_ifconfig ");
> > +	strcat(cmd, if_file);
> 
> The new patch should use "%s %s", not "%s%s" as format string.
> 
> 
> 

Thanks Olaf,

K. Y

Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[Ben Hutchings]

On Mon, Jul 30, 2012 at 06:32:15PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Olaf Hering [mailto:olaf@aepfle.de]
> > Sent: Monday, July 30, 2012 2:03 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> > ben@decadent.org.uk
> > Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> > KVP_OP_SET_IP_INFO
> > 
> > On Tue, Jul 24, K. Y. Srinivasan wrote:
> > 
> > > +	/*
> > > +	 * Set the configuration for the specified interface with
> > > +	 * the information provided. Since there is no standard
> > > +	 * way to configure an interface, we will have an external
> > > +	 * script that does the job of configuring the interface and
> > > +	 * flushing the configuration.
> > > +	 *
> > > +	 * The parameters passed to this external script are:
> > > +	 * 1. A configuration file that has the specified configuration.
> > 
> > Maybe this should be written as 'A info file that has the requested
> > network configuration' or something like that.
> 
> That is the idea. This configuration file simply reflects all the
> information we have perhaps with some additional constant
> information. The script is free to ignore what it does not need. 
[...]

This does not strike me as a sensible interface.  If scripts are
'free to ignore' information then the KVP interface becomes unreliable
as a means for managing networking on Linux guests.  I would suggest
that at the least the script should be able to report that it did not
recognise some parts of the configuration.  This would be logged
and/or reported back to the hypervisor.

(This is separate from the issue of constant configuration lines;
for some distributions the script might recognise but ignore them
because they have no use on that distribution.  I don't see the
point in constant lines, but they don't seem to result in any
unreliability.)

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO

[KY Srinivasan]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Monday, July 30, 2012 3:19 PM
> To: KY Srinivasan
> Cc: Olaf Hering; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
> 
> On Mon, Jul 30, 2012 at 06:32:15PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Olaf Hering [mailto:olaf@aepfle.de]
> > > Sent: Monday, July 30, 2012 2:03 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> > > ben@decadent.org.uk
> > > Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> > > KVP_OP_SET_IP_INFO
> > >
> > > On Tue, Jul 24, K. Y. Srinivasan wrote:
> > >
> > > > +	/*
> > > > +	 * Set the configuration for the specified interface with
> > > > +	 * the information provided. Since there is no standard
> > > > +	 * way to configure an interface, we will have an external
> > > > +	 * script that does the job of configuring the interface and
> > > > +	 * flushing the configuration.
> > > > +	 *
> > > > +	 * The parameters passed to this external script are:
> > > > +	 * 1. A configuration file that has the specified configuration.
> > >
> > > Maybe this should be written as 'A info file that has the requested
> > > network configuration' or something like that.
> >
> > That is the idea. This configuration file simply reflects all the
> > information we have perhaps with some additional constant
> > information. The script is free to ignore what it does not need.
> [...]
> 
> This does not strike me as a sensible interface.  If scripts are
> 'free to ignore' information then the KVP interface becomes unreliable
> as a means for managing networking on Linux guests.  I would suggest
> that at the least the script should be able to report that it did not
> recognise some parts of the configuration.  This would be logged
> and/or reported back to the hypervisor.
> 
> (This is separate from the issue of constant configuration lines;
> for some distributions the script might recognise but ignore them
> because they have no use on that distribution.  I don't see the
> point in constant lines, but they don't seem to result in any
> unreliability.)

Ben,

I see your point. I have cleaned up the contents of the KVP produced
configuration file to not include constant information that can be
auto generated by the distro specific script if it needs to. Also, I have
tried to make the documentation of the contents of the file a little
better. I will send out these new patches soon. Still, there is a possibility that
some of the content of this file may be redundant on a specific distro and I think
that should be fine. For instance, per Olaf's suggestion, I have included a line that
specifies the interface name in the file (in addition to the mac address). Given the
current format of the name of the config file (where the interface name is embedded
in the config file name, this additional name entry may be redundant on some distros.

Once again, thank you for taking the time to review this code.

Regards,

K. Y