Uploaded image for project: 'OpenVZ'
  1. OpenVZ
  2. OVZ-4802

Support for --ipadd a.b.c.d/xx suffix

    Details

    • Type: Feature Request
    • Status: Closed
    • Priority: Major
    • Resolution: Duplicate
    • Fix Version/s: OpenVZ-legacy
    • Component/s: Containers::Userspace
    • Security Level: Public
    • Environment:
      Operating System: All
      Platform: All

      Description

      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.

        Issue Links

          Activity

          Hide
          contacto@victor-roman.com Víctor Román Archidona added a comment -

          Attachment vzctl-3.0.24.2-cidr-suffix.patch has been added with description: Adds IP suffix support for vzctl set VEID --ipadd 1.2.3.4/XX

          Show
          contacto@victor-roman.com Víctor Román Archidona added a comment - Attachment vzctl-3.0.24.2-cidr-suffix.patch has been added with description: Adds IP suffix support for vzctl set VEID --ipadd 1.2.3.4/XX
          Hide
          kir Kir Kolyshkin added a comment - - edited

          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).

          Show
          kir Kir Kolyshkin added a comment - - edited 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).
          Hide
          kir Kir Kolyshkin added a comment - - edited

          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.

          Show
          kir Kir Kolyshkin added a comment - - edited 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.
          Hide
          kir Kir Kolyshkin added a comment - - edited

          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 OVZ-4225.

          Show
          kir Kir Kolyshkin added a comment - - edited 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 OVZ-4225 .

            People

            • Assignee:
              kir Kir Kolyshkin
              Reporter:
              contacto@victor-roman.com Víctor Román Archidona
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: