[libvirt] [PATCH] Update MAC address in virInterface objects when needed.

[Daniel Veillard]

On Mon, Jul 20, 2009 at 07:44:43PM +0100, Daniel P. Berrange wrote:
> On Mon, Jul 20, 2009 at 01:42:04PM -0400, Laine Stump wrote:
> > MAC address of a particular interface may change over time, and the
> > reduced virInterface object (which contains just name and mac) needs
> > to reflect these changes.
> > ---
> >  src/datatypes.c |   24 ++++++++++++++++--------
> >  1 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/datatypes.c b/src/datatypes.c
> > index a8bffd2..a0d027c 100644
> > --- a/src/datatypes.c
> > +++ b/src/datatypes.c
> > @@ -516,9 +516,10 @@ virUnrefNetwork(virNetworkPtr network) {
> >   * @mac: pointer to the mac
> >   *
> >   * Lookup if the interface is already registered for that connection,
> > - * if yes return a new pointer to it, if no allocate a new structure,
> > - * and register it in the table. In any case a corresponding call to
> > - * virUnrefInterface() is needed to not leak data.
> > + * if yes return a new pointer to it (possibly updating the MAC
> > + * address), if no allocate a new structure, and register it in the
> > + * table. In any case a corresponding call to virUnrefInterface() is
> > + * needed to not leak data.
> >   *
> >   * Returns a pointer to the interface, or NULL in case of failure
> >   */
> > @@ -532,11 +533,19 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) {
> >      }
> >      virMutexLock(&conn->lock);
> >  
> > -    /* TODO search by MAC first as they are better differentiators */
> > -
> >      ret = (virInterfacePtr) virHashLookup(conn->interfaces, name);
> > -    /* TODO check the MAC */
> > -    if (ret == NULL) {
> > +
> > +    if (ret != NULL) {
> > +        /* update MAC address if necessary */
> > +        if ((ret->mac == NULL) || STRNEQ(ret->mac, mac)) {
> > +            VIR_FREE(ret->mac);
> > +            ret->mac = strdup(mac);
> > +            if (ret->mac == NULL) {
> > +                virReportOOMError(conn);
> > +                goto error;
> > +            }
> > +        }
> > +    } else {
> 
> There's a small edge case there. the 'ret' object you have
> there is a cached one, whose handled is already in use by
> other callers on libvirt public API. So although you are
> reported an OOM to this caller, other users of this cached
> object have a dangerous instance witha NULL 'mac' field.
> Easy solution, don't VIR_FREE the existing mac until you
> have successfully strdup'd the new one

  Good point, actually if the size permits writing the new value over
just avoids a new allocation.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/