Bug 1671 - Support for --ipadd a.b.c.d/xx suffix
Support for --ipadd a.b.c.d/xx suffix
Status: RESOLVED DUPLICATE of bug 1088
Product: OpenVZ
Classification: Unclassified
Component: vzctl
mainstream
All All
: P2 enhancement
Assigned To: Kir Kolyshkin
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-11 14:05 EDT by Víctor Román Archidona
Modified: 2011-06-28 16:44 EDT (History)
0 users

See Also:


Attachments
Adds IP suffix support for vzctl set VEID --ipadd 1.2.3.4/XX (1.55 KB, patch)
2010-10-11 14:05 EDT, Víctor Román Archidona
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Víctor Román Archidona 2010-10-11 14:05:22 EDT
Created attachment 1285 [details]
Adds IP suffix support for vzctl set VEID --ipadd 1.2.3.4/XX

Currently is not possible to specify a suffix when adding an address to a VE in the form "vzctl set VEID --ipadd a.b.c.d/XX".

Nowadays some distributions switched to iproute2 which recommends to specify the suffix and warning the user that don't doing so may fail in the future.

The attached patch modifies get_netaddr() to support that form of address.
Comment 1 Kir Kolyshkin 2011-06-27 16:12:43 EDT
JFYI, I like this patch, but review and testing revealed two problems:

(1) Bad handling of string lists, so adding/deleting IPs works in funny ways.

> for_each_strtok(token, val, "\t ") {
> 
> ...
>  		if (!find_ip(&net->ip, dst))
> -			add_str_param(&net->ip, dst);
> +			add_str_param(&net->ip, val);

It should be 'token' not 'val'. 


(2)
You never free the memory allocated in get_netaddr(). This can be easily fixed using alloca() (or adding free).

Also, a styling remark, instead of:

+	char *ip_address = NULL;
+	int slash_pos = 0;
+	
+	if (0 != (slash_pos = strcspn(ip_str, "/"))) {
+		ip_address = malloc(slash_pos + 1);
+		strncpy(ip_address, ip_str, slash_pos);
+		ip_address[slash_pos + 1] = '\0';
+	} else {
+		size_t ip_address_len = strlen(ip_str);
+		ip_address = malloc(ip_address_len + 1);
+		strncpy(ip_address, ip_str, ip_address_len);
+		ip_address[ip_address_len + 1] = '\0';
+	}

You could just have
+       const char *ip_str, *slash;
+
+       slash = strchr(str, '/');
+       if (slash)
+               ip_str = strndupa(str, slash - str);
+       else
+               ip_str = str;

which is way easier to comprehend and actually more effective than your code (no malloc/copy in case there is no slash).
Comment 2 Kir Kolyshkin 2011-06-27 16:22:49 EDT
Next problem is there are different ways of specifying an IP, especially for an IPv6 IP (but also for IPv4, too).

For example, 10:29:00:00:00::1, 10:29::1 and 10:29:0::1 is the same address, and it should be treated as being the same. This was handled by the following code:

                if ((family = get_netaddr(token, ip)) < 0)
                        return ERR_INVAL;
                if (inet_ntop(family, ip, dst, sizeof(dst)) == NULL)
                        return ERR_INVAL;
                if (!find_ip(&net->ip, dst))
                        add_str_param(&net->ip, dst)

It translates string IP representation to numeric (get_netaddr) than back to string (inet_ntop), basically "canonicalizing" it, and then it use that string to save.

You changed the add_str_param() so a string is stored in its original form, but it makes 10:29:00:00:00::1, 10:29::1 and 10:29:0::1 different unique strings.

Will rework this code to "canonicalize" IP with netmask.
Comment 3 Kir Kolyshkin 2011-06-28 16:44:11 EDT
Initial work is committed to git:

http://git.openvz.org/?p=vzctl;a=commit;h=f018af5faa10fbe2fa56363c5b393485372c246f

Next step is to modify all the dists/scripts ... so closing as a dupe of bug #1088.

*** This bug has been marked as a duplicate of bug 1088 ***