Thank you for the review! You found a lot of real issues.
On Fri, Aug 09, 2013 at 04:34:08PM -0700, Alex Wang wrote:
> @@ -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.
Thanks. YAMAMOTO Takashi made a similar comment earlier (on a different
patch), so I had already fixed it.
> > @@ -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.
Thanks for pointing this out. I decided that it was not an informative
comment in either place, so I just deleted it.
>
>
>
> > @@ -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.
Thank you. I deleted it.
>
>
>
> > return 0;
> >
> > @@ -393,7 +386,7 @@ error:
> > }
> >
>
>
>
> > error:
>
> free(netdev);
>
>
>
> In the end there shouldn't be the "free(netdev)",
Thank you. I deleted it.
> > @@ -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.
Thanks. YAMAMOTO Takashi made a similar comment earlier (on a different
patch), so I had already fixed it.
> > 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.
Thanks. YAMAMOTO Takashi made a similar comment earlier (on a different
patch), so I had already fixed it.
> > 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".
Good spotting! I decided to delete the *netdevp = NULL; at the top of
the function and write
if (!error) {
netdev->ref_cnt++;
*netdevp = netdev;
} else {
*netdev = NULL;
}
return error;
at the end.
> > /* 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.
I think that the list has to be empty here because each element in
saved_flags_list owns a ref_cnt, and the code in question runs only when
ref_cnt reaches 0.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
