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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list