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

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.