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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/