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
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel