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@xxxxxxxxxxxxxxx]
> Sent: Tuesday, July 24, 2012 9:25 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> apw@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> 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.
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_