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.