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

[Ben Hutchings]

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: signature.asc
Description: This is a digitally signed message part