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

From: KY Srinivasan
Date: Mon Jul 30 2012 - 15:12:31 EST

Next message: Christoph Lameter: "Re: [PATCH] slub: remove one code path and reduce lock contentionin __slab_free()"
Previous message: Larry Woodman: "Re: [PATCH -alternative] mm: hugetlbfs: Close race during teardownof hugetlbfs shared page tables V2 (resend)"
In reply to: Olaf Hering: "Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -KVP_OP_SET_IP_INFO"
Next in thread: Olaf Hering: "Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -KVP_OP_SET_IP_INFO"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@xxxxxxxxx]
> Sent: Monday, July 30, 2012 1:33 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:
>
> > +static char *kvp_get_if_name(char *guid)
> > +{
> > + DIR *dir;
> > + struct dirent *entry;
> > + FILE *file;
> > + char *p, *q, *x;
> > + char *if_name = NULL;
> > + char buf[256];
> > + char *kvp_net_dir = "/sys/class/net/";
> > + char dev_id[100];
>
> Perhaps this can be written as char dev_id[100] = "short string";?

Why?

>
> > +
> > + dir = opendir(kvp_net_dir);
> > + if (dir == NULL)
> > + return NULL;
> > +
> > + memset(dev_id, 0, sizeof(dev_id));
> > + strcat(dev_id, kvp_net_dir);
> > + q = dev_id + strlen(kvp_net_dir);
> > +
> > + while ((entry = readdir(dir)) != NULL) {
> > + /*
> > + * Set the state for the next pass.
> > + */
> > + *q = '\0';
> > + strcat(dev_id, entry->d_name);
> > + strcat(dev_id, "/device/device_id");
>
> Maybe this (and other) strcat should be changed to snprintf?

Already done.

>

> > +
> > + file = fopen(dev_id, "r");
> > + if (file == NULL)
> > + continue;
> > +
> > + p = fgets(buf, sizeof(buf), file);
> > + if (p) {
> > + x = strchr(p, '\n');
> > + if (x)
> > + *x = '\0';
> > +
> > + if (!strcmp(p, guid)) {
> > + /*
> > + * Found the guid match; return the interface
> > + * name. The caller will free the memory.
> > + */
> > + if_name = strdup(entry->d_name);
> > + break;
>
> This will leave 'file' open.
> I have seen it in some other place as well.

I will audit all instances and fix it.

>
> > + }
> > + }
> > + fclose(file);
> > + }
> > +
> > + closedir(dir);
> > + return if_name;
> > +}
> > +
> > +/*
> > + * Retrieve the MAC address given the interface name.
> > + */
> > +
> > +static char *kvp_if_name_to_mac(char *if_name)
> > +{
> > + FILE *file;
> > + char *p, *x;
> > + char buf[256];
> > + char addr_file[100];
> > + int i;
> > + char *mac_addr = NULL;
> > +
> > + memset(addr_file, 0, sizeof(addr_file));
> > + strcat(addr_file, "/sys/class/net/");
> > + strcat(addr_file, if_name);
> > + strcat(addr_file, "/address");
>
> snprintf?

Already fixed.

>
> > +
> > + file = fopen(addr_file, "r");
> > + if (file == NULL)
> > + return NULL;
> > +
> > + p = fgets(buf, sizeof(buf), file);
> > + if (p) {
> > + x = strchr(p, '\n');
> > + if (x)
> > + *x = '\0';
> > + for (i = 0; i < strlen(p); i++)
> > + p[i] = toupper(p[i]);
> > + mac_addr = strdup(p);
> > + }
> > +
> > + fclose(file);
> > + return mac_addr;
> > +}
> > +
> > +
> > static void kvp_process_ipconfig_file(char *cmd,
> > char *config_buf, int len,
> > int element_size, int offset)
> > @@ -800,6 +910,314 @@ getaddr_done:
> > }
> >
> >
> > +static int expand_ipv6(char *addr, int type)
> > +{
> > + int ret;
> > + struct in6_addr v6_addr;
> > +
> > + ret = inet_pton(AF_INET6, addr, &v6_addr);
> > +
> > + if (ret != 1) {
> > + if (type == NETMASK)
> > + return 1;
> > + return 0;
> > + }
> > +
> > + sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
> > + "%02x%02x:%02x%02x:%02x%02x",
> > + (int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],
> > + (int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],
> > + (int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],
> > + (int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],
> > + (int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],
> > + (int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],
> > + (int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],
> > + (int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);
> > +
> > + return 1;
> > +
> > +}
> > +
> > +static int is_ipv4(char *addr)
> > +{
> > + int ret;
> > + struct in_addr ipv4_addr;
> > +
> > + ret = inet_pton(AF_INET, addr, &ipv4_addr);
> > +
> > + if (ret == 1)
> > + return 1;
> > + return 0;
> > +}
> > +
> > +static int parse_ip_val_buffer(char *in_buf, int *offset,
> > + char *out_buf, int out_len)
> > +{
> > + char *x;
> > + char *start;
> > +
> > + /*
> > + * in_buf has sequence of characters that are seperated by
> > + * the character ';'. The last sequence does not have the
> > + * terminating ";" character.
> > + */
> > + start = in_buf + *offset;
> > +
> > + x = strchr(start, ';');
> > + if (x)
> > + *x = 0;
> > + else
> > + x = start + strlen(start);
> > +
> > + if (strlen(start) != 0) {
> > + int i = 0;
> > + /*
> > + * Get rid of leading spaces.
> > + */
> > + while (start[i] == ' ')
> > + i++;
> > +
> > + if ((x - start) <= out_len) {
> > + strcpy(out_buf, (start + i));
> > + *offset += (x - start) + 1;
> > + return 1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +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);
> > + if (error == EOF)
> > + return HV_E_FAIL;
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static int process_ip_string(FILE *f, char *ip_string, int type)
> > +{
> > + int error = 0;
> > + char addr[INET6_ADDRSTRLEN];
> > + int i = 0;
> > + int j = 0;
> > + char str[256];
> > + char sub_str[10];
> > + int offset = 0;
> > +
> > + memset(addr, 0, sizeof(addr));
> > +
> > + while (parse_ip_val_buffer(ip_string, &offset, addr,
> > + (MAX_IP_ADDR_SIZE * 2))) {
> > + memset(sub_str, 0, sizeof(sub_str));
> > + memset(str, 0, sizeof(str));
> > +
> > + if (is_ipv4(addr)) {
> > + switch (type) {
> > + case IPADDR:
> > + strcat(str, "IPADDR");
> > + break;
> > + case NETMASK:
> > + strcat(str, "NETMASK");
> > + break;
> > + case GATEWAY:
> > + strcat(str, "GATEWAY");
> > + break;
> > + case DNS:
> > + strcat(str, "DNS");
> > + break;
> > + }
> > + if (i != 0) {
> > + if (type != DNS)
> > + sprintf(sub_str, "_%d", i++);
> > + else
> > + sprintf(sub_str, "%d", ++i);
> > + } else if (type == DNS) {
> > + sprintf(sub_str, "%d", ++i);
> > + }
> > +
> > +
> > + } else if (expand_ipv6(addr, type)) {
> > + switch (type) {
> > + case IPADDR:
> > + strcat(str, "IPV6ADDR");
> > + break;
> > + case NETMASK:
> > + strcat(str, "IPV6NETMASK");
> > + break;
> > + case GATEWAY:
> > + strcat(str, "IPV6_DEFAULTGW");
> > + break;
> > + case DNS:
> > + strcat(str, "DNS");
> > + break;
> > + }
> > + if ((j != 0) || (type == DNS)) {
> > + if (type != DNS)
> > + sprintf(sub_str, "_%d", j++);
> > + else
> > + sprintf(sub_str, "%d", ++i);
> > + } else if (type == DNS) {
> > + sprintf(sub_str, "%d", ++i);
> > + }
> > + } else {
> > + return HV_INVALIDARG;
> > + }
> > +
> > + error = kvp_write_file(f, str, sub_str, addr);
> > + if (error)
> > + return error;
> > + memset(addr, 0, sizeof(addr));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value
> *new_val)
> > +{
> > + int error = 0;
> > + char if_file[50];
> > + FILE *file;
> > + char cmd[512];
> > + char *mac_addr;
> > +
> > + /*
> > + * 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
> > + * BOOTPROTO=dhcp (dhcp enabled for the interface)
> > + * NM_CONTROLLED=no (this interface will not be controlled by NM)
> > + * PEERDNS=yes
> > + * IPADDR_x=ipaddr
> > + * NETMASK_x=netmask
> > + * GATEWAY_x=gateway
> > + * DNSx=dns
> > + *
> > + * 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-");
> > + 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;
> > +
> > + error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "PEERDNS", NULL, "yes");
> > + if (error)
> > + goto setval_error;
> > +
> > + if (new_val->dhcp_enabled) {
> > + error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
> > + if (error)
> > + goto setval_error;
> > +
> > + /*
> > + * We are done!.
> > + */
> > + goto setval_done;
> > + }
> > +
> > + /*
> > + * Write the configuration for ipaddress, netmask, gateway and
> > + * name servers.
> > + */
> > +
> > + error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> > + if (error)
> > + goto setval_error;
> > +
> > +setval_done:
> > + free(mac_addr);
> > + fclose(file);
> > +
> > + /*
> > + * Now that we have populated the configuration file,
> > + * invoke the external script to do its magic.
> > + */
> > +
> > + memset(cmd, 0, sizeof(cmd));
> > + strcat(cmd, "/sbin/hv_set_ifconfig ");
> > + strcat(cmd, if_file);
>
> The new patch should use "%s %s", not "%s%s" as format string.
>
>
>

Thanks Olaf,

K. Y
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨èÚ&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_

Next message: Christoph Lameter: "Re: [PATCH] slub: remove one code path and reduce lock contentionin __slab_free()"
Previous message: Larry Woodman: "Re: [PATCH -alternative] mm: hugetlbfs: Close race during teardownof hugetlbfs shared page tables V2 (resend)"
In reply to: Olaf Hering: "Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -KVP_OP_SET_IP_INFO"
Next in thread: Olaf Hering: "Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -KVP_OP_SET_IP_INFO"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]