[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Mark McLoughlin]

Hi Jan,

On Wed, 2009-04-15 at 19:34 +0200, Jan Kiszka wrote:
> Mark McLoughlin wrote:

> > @@ -1095,12 +1095,12 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
> >  
> >      d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
> >                                   e1000_receive, e1000_can_receive, d);
> > +    d->vc->cleanup = e1000_cleanup;
> 
> Just to leave my comment here as well :) : I still consider this an
> important, mostly required callback that should be lifted into
> qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
> it will have to explicitly null'ify it.

I still think this is terribly ugly, but I'll add the argument to
new_vlan_client() until I can fix the ugliness more comprehensively :-)

>       qemu_format_nic_info_str(n->vc, n->mac);
> > diff --git a/net.c b/net.c
> > index 34ec4c8..1378ea8 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -362,6 +362,8 @@ void qemu_del_vlan_client(VLANClientState *vc)
> >      while (*pvc != NULL)
> >          if (*pvc == vc) {
> >              *pvc = vc->next;
> > +            if (vc->cleanup)
> > +                vc->cleanup(vc);
> >              free(vc->name);
> >              free(vc->model);
> >              free(vc);
> 
> That should become qemu_free(vc) at this chance. The rest should be
> strdup-allocated.

I'll queue that up as a separate patch.

...
> > @@ -748,6 +752,18 @@ static void tap_send(void *opaque)
> >      }
> >  }
> >  
> > +static void tap_cleanup(VLANClientState *vc)
> > +{
> > +    TAPState *s = vc->opaque;
> > +
> > +    if (s->down_script[0])
> > +        launch_script(s->down_script, s->down_script_arg, s->fd);
> > +
> > +    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
> > +    close(s->fd);
> > +    free(s);
> 
> free -> qemu_free

Thanks; I had spotted this and forgot to fix it before posting.

Cheers,
Mark.