[libvirt] [PATCH] Update MAC address in virInterface objects when needed.
Laine Stump
laine at laine.org
Tue Jul 21 01:24:11 UTC 2009
On 07/20/2009 03:22 PM, Daniel Veillard wrote:
> 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.
>
Now that you make me think about it - beyond that, if another thread has
a pointer to this object, they may have already gotten a copy of the mac
pointer into a temp variable, and if they then use it after I've freed
it (and maybe someone else re-uses the same memory) they will get bad data.
So I guess the *real* solution is to always compare the macs, and if
they don't match create a new object. This will mean that this
hypothetical "other thread" may be working with out-of-date information
(it will still have the old mac address, at least for iface-list), but
at least it will never stomp on someone else's data.
More information about the libvir-list
mailing list