[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V2] nwfilter: lock interface by its index



On Wed, Apr 21, 2010 at 02:38:33PM -0400, Stefan Berger wrote:
> Changes from V1 to V2:
[...]
>  - there are 2 calls in the virNWFilterLockIface function that can fail
> due to OOM error; in case that happens, loop until the memory is
> available to avoid a failing lock. The call initializing the mutex
> should never fail with pthreads, otherwise we'd have a compile-time
> problem (wrong parameters for recursive lock) and may fail due to OOM
> error on win-32, so I am also looping there. The interface name may
> cause a failure, but callers should be certain at this point that the
> string is suitable. I am adding ATTRIBUTE_RETURN_CHECK to force the
> caller to check for failure and adapting the code where the function is
> called.

  I'm a bit puzzled by that.
The stategy in libvirt is that if you get out of memory, you fail the
operation, this mean releassing the locks you may helpd at the various
levels and propagate the errors back up to a place where the error can
be reported safely, and process continue. I dislike your suggestion,
waiting doesn't help, windows or not.


> +int
> +virNWFilterLockIface(const char *ifname) {
> +    virNWFilterIfaceLockPtr ifaceLock;
> +
> +    virMutexLock(&ifaceMapLock);
> +
> +    ifaceLock = virHashLookup(ifaceLockMap, ifname);
> +    if (!ifaceLock) {
> +        while (VIR_ALLOC(ifaceLock) < 0) {
> +            /* wait for memory */
> +            usleep(50);
> +        }
> +
> +        if (virMutexInitRecursive(&ifaceLock->lock)) {

  that's supposed to be a while I guess...

> +            /* win32 - wait for memory */
> +            usleep(50);
> +        }
> +
> +        if (virStrcpyStatic(ifaceLock->ifname, ifname) == NULL) {
> +            VIR_FREE(ifaceLock);
> +            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("%s: interface name %s does not fit into "
> +                                     "buffer "),
> +                                   __FUNCTION__, ifaceLock->ifname);
> +            goto err_exit;
> +        }
> +
> +        while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) {
> +            /* wait for memory */
> +            usleep(50);
> +        }
> +
> +        ifaceLock->refctr = 0;
> +    }
> +
> +    ifaceLock->refctr++;
> +
> +    virMutexUnlock(&ifaceMapLock);
> +
> +    virMutexLock(&ifaceLock->lock);
> +
> +    return 0;
> +
> + err_exit:
> +    virMutexUnlock(&ifaceMapLock);
> +
> +    return 1;
> +}

  yeah really those usleep(50); make little sense to me
I prefer to just fail the operation in this case and report the
problem.

[...]
>  
> +    if (virNWFilterLockIface(req->ifname))
> +       goto err_no_lock;
> +

 As long as the error is caught when used,  and properly propagated
that strategy is correct IMHO.

  otherwise, patch looks okay to me,

ACK once those forever loops are replaced with error returns.

Daniel

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]