Hey, most of code looks good to me,
Few comments inline mostly at netdev-bsd.c:
@@ -162,7 +160,7 @@ netdev_bsd_cast(const struct netdev *netdev)
> static struct netdev_rx_bsd *
> netdev_rx_bsd_cast(const struct netdev_rx *rx)
> {
> - netdev_rx_assert_class(rx, &netdev_rx_bsd_class);
> + ovs_assert(is_netdev_bsd_class(netdev_get_class(rx->netdev)));
> return CONTAINER_OF(rx, struct netdev_rx_bsd, up);
> }
>
The "is_netdev_bsd_class()" function also requires modification, since
there is no "init()" function.
>
> @@ -269,12 +267,18 @@ cache_notifier_unref(void)
> return 0;
> }
>
> +static struct netdev *
> +netdev_bsd_alloc(void)
> +{
> + struct netdev_bsd *netdev = xzalloc(sizeof *netdev);
> + return &netdev->up;
> +}
> +
> /* Allocate a netdev_bsd structure */
> static int
>
Should we move this comment up to "netdev_bsd_alloc"? Seems more
appropriate there.
> @@ -378,9 +373,7 @@ netdev_bsd_create_tap(const struct netdev_class
> *class, const char *name,
>
> /* initialize the device structure and
> * link the structure to its netdev */
> - netdev_init(&netdev->up, name, class);
> netdev->kernel_name = kernel_name;
> - *netdevp = &netdev->up;
>
>
The comment above is not applicable.
> return 0;
>
> @@ -393,7 +386,7 @@ error:
> }
>
> error:
free(netdev);
In the end there shouldn't be the "free(netdev)",
> @@ -1385,8 +1390,6 @@ const struct netdev_class netdev_bsd_class = {
> NULL, /* set_config */
> NULL, /* get_tunnel_config */
>
> - netdev_bsd_rx_open,
> -
> netdev_bsd_send,
> netdev_bsd_send_wait,
>
Miss the alloc/construct/destruct/dealloc functions.
> const struct netdev_class netdev_tap_class = {
> "tap",
>
> - netdev_bsd_init,
> + NULL, /* init */
> netdev_bsd_run,
> netdev_bsd_wait,
> netdev_bsd_create_tap,
>
Miss the alloc/construct/destruct/dealloc functions.
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 30c44a2..0fdebe8 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -259,22 +259,36 @@ netdev_open(const char *name, const char
> + }
> + if (!error) {
> + netdev->ref_cnt++;
> }
> - netdev->ref_cnt++;
>
> *netdevp = netdev;
> - return 0;
> + return error;
> }
>
>
Could we move "*netdevp = netdev;" into the "if" statement? So, in error,
"netdevp = NULL".
> /* Returns a reference to 'netdev_' for the caller to own. Returns null if
> @@ -348,7 +362,11 @@ netdev_unref(struct netdev *dev)
> {
> ovs_assert(dev->ref_cnt);
> if (!--dev->ref_cnt) {
> - netdev_uninit(dev, true);
> + dev->netdev_class->destruct(dev);
> +
> + shash_delete(&netdev_shash, dev->node);
> + free(dev->name);
> + dev->netdev_class->dealloc(dev);
> }
> }
In "netdev_open()" there is the "list_init(&netdev->saved_flags_list);".
I didn't find the where we check and free the list elements.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
