netplan-developers team mailing list archive

[Martin Pitt <martin.pitt@xxxxxxxxxx>]

Review: Needs Fixing

The general structure looks good to me, I have a bunch of inline comments.

This is missing the NM implementation and extending the integration test case to cover this.

Diff comments:

> diff --git a/src/parse.c b/src/parse.c
> index 2db2b11..5fecf1d 100644
> --- a/src/parse.c
> +++ b/src/parse.c
> @@ -625,6 +627,128 @@ handle_nameservers_addresses(yaml_document_t* doc, yaml_node_t* node, const void
>      return TRUE;
>  }
>  
> +
> +static int 
> +get_ip_family(const char* address)
> +{
> +    struct in_addr a4;
> +    struct in6_addr a6;
> +    char *ip_str, *prefix_len;

ip_str is missing a g_autofree. But actually I don't see any reason to strdup this at all, you should be able to call inet_pton() directly on "address" (the argument is const).

> +    int family;
> +    int ret = -1;
> +
> +    ip_str = g_strdup(address);
> +    prefix_len = strrchr(ip_str, '/');
> +    if (prefix_len)
> +        *prefix_len = '\0';
> +
> +    family = AF_INET;
> +    ret = inet_pton(AF_INET, ip_str, &a4);
> +    g_assert(ret >= 0);
> +    if (ret > 0) {
> +        ret = family;
> +        goto family_ret;

This could just return the family directly

> +    }
> +
> +    family = AF_INET6;
> +    ret = inet_pton(AF_INET6, ip_str, &a6);
> +    g_assert(ret >= 0);
> +    if (ret > 0)
> +        ret = family;
> +

... and then do a g_assert_not_reached() here or some error message about an invalid value.

> +family_ret:
> +    return ret;
> +}
> +
> +static gboolean
> +check_and_set_family(int family)
> +{
> +    if (cur_route->family != -1 && cur_route->family != family)
> +        return FALSE;
> +
> +    cur_route->family = family;
> +
> +    return TRUE;
> +}
> +
> +static gboolean
> +handle_routes_to(yaml_document_t* doc, yaml_node_t* node, const void* _, GError** error)
> +{
> +    int family = get_ip_family(scalar(node));
> +    g_assert(family >= 0);
> +    if (!check_and_set_family(family))
> +        return yaml_error(node, error, "IP family mismatch in route to %s", scalar(node));
> +    cur_route->to = g_strdup(scalar(node));
> +
> +    return TRUE;
> +}
> +
> +static gboolean
> +handle_routes_via(yaml_document_t* doc, yaml_node_t* node, const void* _, GError** error)
> +{
> +    int family = get_ip_family(scalar(node));
> +    g_assert(family >= 0);
> +    if (!check_and_set_family(family))
> +        return yaml_error(node, error, "IP family mismatch in route via %s", scalar(node));
> +    cur_route->via = g_strdup(scalar(node));
> +
> +    return TRUE;
> +}

This does almost exactly the same. I'd suggest to take the offset of the target field in the userdata, like with netdef_offset(). Then you can also fold check_and_set_family() into this function.

> +
> +static gboolean
> +handle_routes_metric(yaml_document_t* doc, yaml_node_t* node, const void* _, GError** error)
> +{
> +    guint64 v;
> +    gchar* endptr;
> +
> +    v = g_ascii_strtoull(scalar(node), &endptr, 10);
> +    if (*endptr != '\0' || v > G_MAXUINT)
> +        return yaml_error(node, error, "invalid unsigned int value %s", scalar(node));
> +
> +    cur_route->metric = (guint) v;
> +    return TRUE;
> +}
> +
> +/****************************************************
> + * Grammar and handlers for network config "routes" entry
> + ****************************************************/
> +
> +const mapping_entry_handler routes_handlers[] = {
> +    {"to", YAML_SCALAR_NODE, handle_routes_to},
> +    {"via", YAML_SCALAR_NODE, handle_routes_via},
> +    {"metric", YAML_SCALAR_NODE, handle_routes_metric},
> +    {NULL}
> +};
> +
> +static gboolean
> +handle_routes(yaml_document_t* doc, yaml_node_t* node, const void* _, GError** error)
> +{
> +    for (yaml_node_item_t *i = node->data.sequence.items.start; i < node->data.sequence.items.top; i++) {
> +        yaml_node_t *entry = yaml_document_get_node(doc, *i);
> +
> +        cur_route = g_new0(ip_route, 1);
> +        cur_route->family = G_MAXUINT; /* 0 is a valid family ID */
> +        cur_route->metric = G_MAXUINT; /* 0 is a valid metric */
> +
> +        if (process_mapping(doc, entry, routes_handlers, error)) {
> +            if (!cur_netdef->routes) {
> +                cur_netdef->has_routes = TRUE;
> +                cur_netdef->routes = g_array_new(FALSE, FALSE, sizeof(ip_route*));
> +            }
> +
> +            g_array_append_val(cur_netdef->routes, cur_route);
> +        }

For robustness/better debuggability I think it would make sense to set cur_route back to NULL here.

> +
> +        if (error && *error)
> +            return FALSE;
> +    }
> +    return TRUE;
> +}
> +
> +/****************************************************
> + * Grammar and handlers for network devices
> + ****************************************************/
> +
>  const mapping_entry_handler nameservers_handlers[] = {
>      {"search", YAML_SEQUENCE_NODE, handle_nameservers_search},
>      {"addresses", YAML_SEQUENCE_NODE, handle_nameservers_addresses},
> @@ -642,6 +766,7 @@ const mapping_entry_handler ethernet_def_handlers[] = {
>      {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
>      {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
>      {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
> +    {"routes", YAML_SEQUENCE_NODE, handle_routes},

must be MAPPING_NODE

>      {NULL}
>  };
>  
> diff --git a/src/parse.h b/src/parse.h
> index 7c5bf2b..10250a1 100644
> --- a/src/parse.h
> +++ b/src/parse.h
> @@ -61,6 +61,8 @@ typedef struct net_definition {
>      GArray* ip4_nameservers;
>      GArray* ip6_nameservers;
>      GArray* search_domains;
> +    GArray* routes;
> +    gboolean has_routes;

Please drop this, it's redundant with routes != NULL.

>  
>      /* master ID for slave devices */
>      char* bridge;
> diff --git a/tests/generate.py b/tests/generate.py
> index b3a127b..83dedbd 100755
> --- a/tests/generate.py
> +++ b/tests/generate.py
> @@ -510,6 +510,152 @@ Address=2001:FFfe::1/64
>  RouteMetric=100
>  '''})
>  
> +    def test_device_bad_routes(self):
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      routes:
> +        - to: badlocation
> +          via: 192.168.14.20
> +          metric: 100
> +      addresses:
> +        - 192.168.14.2/24
> +        - 2001:FFfe::1/64''', expect_fail=True)
> +
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      routes:
> +        - to: 10.10.0.0/16
> +          via: badgateway
> +          metric: 100
> +      addresses:
> +        - 192.168.14.2/24
> +        - 2001:FFfe::1/64''', expect_fail=True)
> +
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      routes:
> +        - to: 10.10.0.0/16
> +          via: 10.1.1.1
> +          metric: -1
> +      addresses:
> +        - 192.168.14.2/24
> +        - 2001:FFfe::1/64''', expect_fail=True)
> +
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      routes:
> +        - to: 2001:dead:beef::0/16
> +          via: 10.1.1.1
> +          metric: 1
> +      addresses:
> +        - 192.168.14.2/24
> +        - 2001:FFfe::1/64''', expect_fail=True)
> +
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      routes:
> +        - via: 2001:dead:beef::2
> +          to: 10.10.10.0/24
> +          metric: 1
> +      addresses:
> +        - 192.168.14.2/24
> +        - 2001:FFfe::1/64''', expect_fail=True)

Can you please move this to TestConfigErrors for consistency?

Also, please add some tests what happens if a dict entry is missing via or to, and for specifying an IPv4 to and IPv6 to.

> +
> +    def test_route_v4_single(self):
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      addresses: ["192.168.14.2/24"]
> +      routes:
> +        - to: 10.10.10.0/24
> +          via: 192.168.14.20
> +          metric: 100
> +          ''')
> +
> +        self.assert_networkd({'engreen.network': '''[Match]
> +Name=engreen
> +
> +[Network]
> +Address=192.168.14.2/24
> +
> +[Route]
> +Destination=10.10.10.0/24
> +Gateway=192.168.14.20
> +Metric=100
> +'''})
> +
> +    def test_route_v4_multiple(self):
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    engreen:
> +      addresses: ["192.168.14.2/24"]
> +      routes:
> +        - to: 8.8.0.0/16
> +          via: 192.168.1.1
> +          metric: 10
> +        - to: 10.10.10.8
> +          via: 192.168.1.2
> +          metric: 5000
> +        - to: 11.11.11.0/24
> +          via: 192.168.1.3
> +          metric: 9999
> +          ''')
> +
> +        self.assert_networkd({'engreen.network': '''[Match]
> +Name=engreen
> +
> +[Network]
> +Address=192.168.14.2/24
> +
> +[Route]
> +Destination=8.8.0.0/16
> +Gateway=192.168.1.1
> +Metric=10
> +
> +[Route]
> +Destination=10.10.10.8
> +Gateway=192.168.1.2
> +Metric=5000
> +
> +[Route]
> +Destination=11.11.11.0/24
> +Gateway=192.168.1.3
> +Metric=9999
> +'''})
> +
> +    def test_route_v6_single(self):
> +        self.generate('''network:
> +  version: 2
> +  ethernets:
> +    enblue:
> +      addresses: ["192.168.1.3/24"]
> +      routes:
> +        - to: 2001:dead:beef::2/64
> +          via: 2001:beef:beef::1''')
> +
> +        self.assert_networkd({'enblue.network': '''[Match]
> +Name=enblue
> +
> +[Network]
> +Address=192.168.1.3/24
> +
> +[Route]
> +Destination=2001:dead:beef::2/64
> +Gateway=2001:beef:beef::1
> +'''})
> +
>      def test_wifi(self):
>          self.generate('''network:
>    version: 2


-- 
https://code.launchpad.net/~netplan-developers/netplan/+git/netplan/+merge/312683
Your team Developers of netplan is subscribed to branch netplan:master.