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

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@xxxxxxxxx]
> Sent: Monday, July 30, 2012 2:03 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; apw@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> ben@xxxxxxxxxxxxxxx
> 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

èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—