From f7fce51a1bd4a4430da5fab53bac7ab79304f0da Mon Sep 17 00:00:00 2001 From: Anthony Hinsinger Date: Fri, 17 Apr 2015 12:16:33 +0200 Subject: [PATCH 1/4] Forced GetId result to -1 for stopped domains. When a domain is not started, virDomainGetID returns -1u, but virGetLastError() returns NULL, so we get unexpected result in the callback. --- src/domain.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/domain.cc b/src/domain.cc index 842e3bd..a580397 100644 --- a/src/domain.cc +++ b/src/domain.cc @@ -458,8 +458,9 @@ NLV_WORKER_EXECUTE(Domain, GetId) { NLV_WORKER_ASSERT_DOMAIN(); - int result = virDomainGetID(Handle().ToDomain()); - if (result == -1) { + unsigned int result = virDomainGetID(Handle().ToDomain()); + if (result == -1u) { + data_ = -1; SetVirError(virGetLastError()); return; } From 9ff50a5adc61065209ca85c6e72648e524fff9cd Mon Sep 17 00:00:00 2001 From: Anthony Hinsinger Date: Wed, 22 Apr 2015 13:25:40 +0200 Subject: [PATCH 2/4] Modified libvirt event implementation to be thread safe --- src/event_impl.cc | 190 ++++++++++++++++++++++++++++++++++------------ src/event_impl.h | 10 ++- 2 files changed, 149 insertions(+), 51 deletions(-) diff --git a/src/event_impl.cc b/src/event_impl.cc index fcacf70..48e4e72 100644 --- a/src/event_impl.cc +++ b/src/event_impl.cc @@ -5,30 +5,37 @@ namespace NodeLibvirt { -struct nodeEventHandle +class nodeEventHandle { +public: int watch; virEventHandleCallback cb; void *opaque; int event; - int running; + int newEvent; + int toDelete; int deleted; int fd; uv_poll_t watcher; }; -struct nodeEventTimeout +class nodeEventTimeout { +public: int timer; int frequency; + int newFrequency; virEventTimeoutCallback cb; void *opaque; + int toDelete; int deleted; uv_timer_t timerWatcher; uv_check_t checkWatcher; }; +uv_check_t updateHandleChecker; +uv_mutex_t lock; int EventImpl::nextWatch = 1; int EventImpl::nextTimeout = 1; std::vector EventImpl::handles; @@ -44,6 +51,11 @@ void EventImpl::Initialize(Handle exports) NAN_METHOD(EventImpl::SetupEvent) { NanScope(); + + uv_mutex_init(&lock); + uv_check_init(uv_default_loop(), &updateHandleChecker); + uv_check_start(&updateHandleChecker, EventImpl::UpdateHandlesOnce); + virEventRegisterImpl( AddHandle, UpdateHandle, RemoveHandle, AddTimeout, UpdateTimeout, RemoveTimeout @@ -52,35 +64,116 @@ NAN_METHOD(EventImpl::SetupEvent) NanReturnUndefined(); } +#if UV_VERSION_MAJOR < 1 +void EventImpl::UpdateHandlesOnce(uv_check_t* handle, int status) +#else +void EventImpl::UpdateHandlesOnce(uv_check_t* handle) +#endif +{ + uv_mutex_lock(&lock); + + for (std::vector::iterator it = handles.begin() ; it != handles.end(); ++it) { + nodeEventHandle *handle = *it; + + if (handle->newEvent == handle->event && !handle->toDelete) + continue; + + if (handle->toDelete) { + //fprintf(stderr, " CLOSE POLL, watch=%d event=%d\n", handle->watch, handle->newEvent); + handle->toDelete = 0; + uv_poll_stop(&handle->watcher); + uv_close((uv_handle_t*)&handle->watcher, EventImpl::ClosePollCallback); + } else if (EventToUV(handle->newEvent) == 0) { + //fprintf(stderr, " STOP POLL, watch=%d event=%d\n", handle->watch, handle->newEvent); + uv_poll_stop(&handle->watcher); + } else { + //fprintf(stderr, " START POLL, watch=%d event=%d\n", handle->watch, handle->newEvent); + uv_poll_start(&handle->watcher, EventToUV(handle->newEvent), EventImpl::HandleCallback); + } + + handle->event = handle->newEvent; + } + + for (std::vector::iterator it = timeouts.begin() ; it != timeouts.end(); ++it) { + nodeEventTimeout *timeout = *it; + + if (timeout->newFrequency == timeout->frequency && !timeout->toDelete) + continue; + + fprintf(stderr, "CHANGE FREQ, freq=%d\n", timeout->newFrequency); + uv_timer_stop(&timeout->timerWatcher); + uv_check_stop(&timeout->checkWatcher); + + if (timeout->toDelete) { + timeout->deleted = 1; + } else if (timeout->newFrequency == 0) { + uv_check_start(&timeout->checkWatcher, CheckCallback); + } else if (timeout->newFrequency >= 0) { + uv_timer_start(&timeout->timerWatcher, TimerCallback, timeout->newFrequency, timeout->newFrequency); + } + + timeout->frequency = timeout->newFrequency; + } + + uv_mutex_unlock(&lock); +} + void EventImpl::HandleCallback(uv_poll_t* handle, int status, int events) { + //fprintf(stderr, "dispatch handle=%d status=%d, events=%d\n", h->watch, status, events); + nodeEventHandle *h = (nodeEventHandle*) handle->data; virEventHandleCallback cb = h->cb; (cb)(h->watch, h->fd, EventImpl::EventFromUV(events), h->opaque); } +#if UV_VERSION_MAJOR < 1 void EventImpl::CheckCallback(uv_check_t* handle, int status) +#else +void EventImpl::CheckCallback(uv_check_t* handle) +#endif { TimeoutCallback((uv_handle_s*)handle); } +#if UV_VERSION_MAJOR < 1 void EventImpl::TimerCallback(uv_timer_t* handle, int status) +#else +void EventImpl::TimerCallback(uv_timer_t* handle) +#endif { TimeoutCallback((uv_handle_s*)handle); } void EventImpl::TimeoutCallback(uv_handle_s* handle) { + //fprintf(stderr, "dispatch timer=%d\n", t->timer); + nodeEventTimeout *t = (nodeEventTimeout*) handle->data; virEventTimeoutCallback cb = t->cb; (cb)(t->timer, t->opaque); + +} + +void EventImpl::ClosePollCallback(uv_handle_t* handle) { + //fprintf(stderr, "close handle cb\n"); + + uv_mutex_lock(&lock); + + nodeEventHandle *h = (nodeEventHandle*) handle->data; + h->deleted = 1; + + uv_mutex_unlock(&lock); } int EventImpl::AddHandle(int fd, int event, virEventHandleCallback cb, void *opaque, virFreeCallback ff) { + fprintf(stderr, "Adding handle, fd=%d, event=%d\n", fd, event); + + uv_mutex_lock(&lock); nodeEventHandle *handle; handle = FindDeletedHandle(); @@ -93,7 +186,8 @@ int EventImpl::AddHandle(int fd, int event, virEventHandleCallback cb, void *opa handle->cb = cb; handle->fd = fd; handle->event = event; - handle->running = 0; + handle->newEvent = event; + handle->toDelete = 0; handle->deleted = 0; handle->opaque = opaque; @@ -102,46 +196,41 @@ int EventImpl::AddHandle(int fd, int event, virEventHandleCallback cb, void *opa if (event) { uv_poll_start(&handle->watcher, EventToUV(event), HandleCallback); - handle->running = 1; } + uv_mutex_unlock(&lock); + return handle->watch; } void EventImpl::UpdateHandle(int watch, int event) { + //fprintf(stderr, "Update handle, watch=%d, event=%d\n", watch, event); + + uv_mutex_lock(&lock); + nodeEventHandle* handle; handle = FindHandle(watch); + if (handle != NULL) + handle->newEvent = event; - if (handle == NULL) - return; - - if (!event && handle->running) { - //fprintf(stderr, " STOP POLL, watch=%d event=%d\n", watch, event); - uv_poll_stop(&handle->watcher); - handle->running = 0; - } else if (event && handle->event == event && !handle->running) { - //fprintf(stderr, " RESTART POLL, watch=%d event=%d\n", watch, event); - uv_poll_start(&handle->watcher, EventToUV(event), HandleCallback); - handle->running = 1; - } else if (event && handle->event != event) { - //fprintf(stderr, " MODIFYING POLL, watch=%d event=%d\n", watch, event); - uv_poll_start(&handle->watcher, EventToUV(event), HandleCallback); - handle->event = event; - handle->running = 1; - } + uv_mutex_unlock(&lock); } int EventImpl::RemoveHandle(int watch) { + uv_mutex_lock(&lock); + nodeEventHandle* handle; handle = FindHandle(watch); - if (handle == NULL) - return -1; + if (handle != NULL) { + handle->newEvent = 0; + handle->toDelete = 1; + } + + uv_mutex_unlock(&lock); - uv_poll_stop(&handle->watcher); - handle->deleted = 1; return 0; } @@ -170,24 +259,32 @@ nodeEventHandle* EventImpl::FindDeletedHandle() int EventImpl::AddTimeout(int frequency, virEventTimeoutCallback cb, void *opaque, virFreeCallback ff) { - //fprintf(stderr, "Adding timeout, freq=%d\n", frequency); + fprintf(stderr, "Adding timeout, freq=%d\n", frequency); + + uv_mutex_lock(&lock); + nodeEventTimeout *timeout; timeout = FindDeletedTimeout(); if (timeout == NULL) { timeout = new nodeEventTimeout(); timeout->timer = nextTimeout++; + + uv_check_init(uv_default_loop(), &timeout->checkWatcher); + uv_timer_init(uv_default_loop(), &timeout->timerWatcher); + timeout->checkWatcher.data = timeout; + timeout->timerWatcher.data = timeout; + timeouts.push_back(timeout); } timeout->cb = cb; timeout->frequency = frequency; + timeout->newFrequency = frequency; + timeout->toDelete = 0; timeout->deleted = 0; timeout->opaque = opaque; - uv_check_init(uv_default_loop(), &timeout->checkWatcher); - uv_timer_init(uv_default_loop(), &timeout->timerWatcher); - timeout->checkWatcher.data = timeout; timeout->timerWatcher.data = timeout; @@ -197,43 +294,36 @@ int EventImpl::AddTimeout(int frequency, virEventTimeoutCallback cb, void *opaqu uv_timer_start(&timeout->timerWatcher, TimerCallback, frequency, frequency); } + uv_mutex_unlock(&lock); + return timeout->timer; } void EventImpl::UpdateTimeout(int timer, int frequency) { - //fprintf(stderr, "update timeout, timer=%d timeout=%d\n", timer, frequency); - nodeEventTimeout* timeout = FindTimeout(timer); + fprintf(stderr, "update timeout, timer=%d timeout=%d\n", timer, frequency); - if (timeout == NULL) - return; + uv_mutex_lock(&lock); - if (frequency == timeout->frequency) - return; - - uv_timer_stop(&timeout->timerWatcher); - uv_check_stop(&timeout->checkWatcher); + nodeEventTimeout* timeout = FindTimeout(timer); - timeout->frequency = frequency; + if (timeout != NULL) + timeout->newFrequency = frequency; - if (frequency == 0) { - uv_check_start(&timeout->checkWatcher, CheckCallback); - } else if (frequency >= 0) { - uv_timer_start(&timeout->timerWatcher, TimerCallback, frequency, frequency); - } + uv_mutex_unlock(&lock); } int EventImpl::RemoveTimeout(int timer) { //fprintf(stderr, "remove timeout, timer=%d\n", timer); + uv_mutex_lock(&lock); + nodeEventTimeout* timeout = FindTimeout(timer); - if (timeout == NULL) - return -1; + if (timeout != NULL) + timeout->toDelete = 1; - uv_timer_stop(&timeout->timerWatcher); - uv_check_stop(&timeout->checkWatcher); - timeout->deleted = 1; + uv_mutex_unlock(&lock); return 0; } diff --git a/src/event_impl.h b/src/event_impl.h index ffc1a49..2475999 100644 --- a/src/event_impl.h +++ b/src/event_impl.h @@ -10,6 +10,7 @@ namespace NodeLibvirt { class nodeEventHandle; class nodeEventTimeout; + class EventImpl { public: @@ -26,9 +27,17 @@ class EventImpl static int RemoveTimeout(int timer); static void HandleCallback(uv_poll_t* handle, int status, int events); +#if UV_VERSION_MAJOR < 1 + static void UpdateHandlesOnce(uv_check_t* handle, int status); static void CheckCallback(uv_check_t* handle, int status); static void TimerCallback(uv_timer_t* handle, int status); +#else + static void UpdateHandlesOnce(uv_check_t* handle); + static void CheckCallback(uv_check_t* handle); + static void TimerCallback(uv_timer_t* handle); +#endif static void TimeoutCallback(uv_handle_s* handle); + static void ClosePollCallback(uv_handle_t* handle); static int EventToUV(int); static int EventFromUV(int); @@ -47,4 +56,3 @@ class EventImpl } //namespace NodeLibvirt #endif // SRC_EVENTIMPL_H - From dc23f0de2ac28f067f29bf60dfeb2c58a01a8780 Mon Sep 17 00:00:00 2001 From: Anthony Hinsinger Date: Wed, 22 Apr 2015 13:32:44 +0200 Subject: [PATCH 3/4] Commented some printf debug --- src/event_impl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/event_impl.cc b/src/event_impl.cc index 48e4e72..d28f80f 100644 --- a/src/event_impl.cc +++ b/src/event_impl.cc @@ -100,7 +100,7 @@ void EventImpl::UpdateHandlesOnce(uv_check_t* handle) if (timeout->newFrequency == timeout->frequency && !timeout->toDelete) continue; - fprintf(stderr, "CHANGE FREQ, freq=%d\n", timeout->newFrequency); + //fprintf(stderr, "CHANGE FREQ, freq=%d\n", timeout->newFrequency); uv_timer_stop(&timeout->timerWatcher); uv_check_stop(&timeout->checkWatcher); @@ -171,7 +171,7 @@ void EventImpl::ClosePollCallback(uv_handle_t* handle) { int EventImpl::AddHandle(int fd, int event, virEventHandleCallback cb, void *opaque, virFreeCallback ff) { - fprintf(stderr, "Adding handle, fd=%d, event=%d\n", fd, event); + //fprintf(stderr, "Adding handle, fd=%d, event=%d\n", fd, event); uv_mutex_lock(&lock); nodeEventHandle *handle; @@ -259,7 +259,7 @@ nodeEventHandle* EventImpl::FindDeletedHandle() int EventImpl::AddTimeout(int frequency, virEventTimeoutCallback cb, void *opaque, virFreeCallback ff) { - fprintf(stderr, "Adding timeout, freq=%d\n", frequency); + //fprintf(stderr, "Adding timeout, freq=%d\n", frequency); uv_mutex_lock(&lock); @@ -301,7 +301,7 @@ int EventImpl::AddTimeout(int frequency, virEventTimeoutCallback cb, void *opaqu void EventImpl::UpdateTimeout(int timer, int frequency) { - fprintf(stderr, "update timeout, timer=%d timeout=%d\n", timer, frequency); + //fprintf(stderr, "update timeout, timer=%d timeout=%d\n", timer, frequency); uv_mutex_lock(&lock); From 9051d46db597b1e925347bd57f13132b2cb08c22 Mon Sep 17 00:00:00 2001 From: Anthony Hinsinger Date: Wed, 22 Apr 2015 14:52:10 +0200 Subject: [PATCH 4/4] Improved tests --- src/event_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/event_impl.cc b/src/event_impl.cc index d28f80f..b7a6ff5 100644 --- a/src/event_impl.cc +++ b/src/event_impl.cc @@ -75,7 +75,7 @@ void EventImpl::UpdateHandlesOnce(uv_check_t* handle) for (std::vector::iterator it = handles.begin() ; it != handles.end(); ++it) { nodeEventHandle *handle = *it; - if (handle->newEvent == handle->event && !handle->toDelete) + if (handle->deleted || (handle->newEvent == handle->event && !handle->toDelete)) continue; if (handle->toDelete) { @@ -97,7 +97,7 @@ void EventImpl::UpdateHandlesOnce(uv_check_t* handle) for (std::vector::iterator it = timeouts.begin() ; it != timeouts.end(); ++it) { nodeEventTimeout *timeout = *it; - if (timeout->newFrequency == timeout->frequency && !timeout->toDelete) + if (timeout->deleted || (timeout->newFrequency == timeout->frequency && !timeout->toDelete)) continue; //fprintf(stderr, "CHANGE FREQ, freq=%d\n", timeout->newFrequency);