Squashed into "util: Add netlink event handling to virnetlink.c" (Note
that I am also re-sending the complete patchset with these changes
already squashed in).
* The remove callback is now an argument of
virNetlinnkEventAddClient instead of virNetlinkeRemoveClient,
and is stored in the virNetlinkEventHandle. This way the event
service shutdown can properly remove any remaining clients to
avoid memory leaks.
* Several small whitespace changes (moving the opening { of
functions down to a separate line, for example).
* As said above, the service stop function now removes any
remaining clients on the list.
* The libvirt convention to check for a failure return from a
function is (ret < 0) rather than (ret == -1). That was changed
in two places.
* If macaddr isn't given when adding a client, it is initialized
to all 0's (on a clean init, it would be 0 anyway, but if the
entry is being re-used, it would have stale data from the
previous use).
* In virNetlinkEventRemoveClient, the to if() clauses inside the
for loop had identical bodies (except for one word in the debug
message). These have been combined into a single if() clause.
* Again in virNetlinkEventRemoveClient, the opaque object was
being freed by the event service code, which is supposed to
treat opaque as a complete unknown. This is no longer done - if
opaque is dynamically allocated data that needs to be freed,
this should be done in the client-supplied removal callback (as
noted earlier, the client has been appropriately changed).
Changes squashed into "Add de-association handling to macvlan code"
* In the virNetCallbackData struct, the pointers to data that are
in the domain's config object are a bit dangerous - it's
possible that the domain's config could be updated while the
domain remains active, freeing the original config object and
allocating a new one. This would leave the
virNetlinkCallbackData pointing at garbage data. To fix this,
the struct has had all consts removed, the object is
initialized by allocating new memory for each of these items
and copying (rather than just pointing at the original), and
the remove callback frees all of the items. The result is that
the callback data object is completely decoupled from the config
object.
* Since the callback data object needs to be freed both in the
remove callback as well as in the error path at the time the
client is added, a separate virNetlinkCallbackDataFree()
function has been added. The remove callback now simply calls
that function.
* Previously the callback data object itself was freed by the
netlink event service code as the client was being removed. This
makes it impossible to use the supposedly "opaque" data as
anything other than a pure pointer to dynamically allocated
data (a client might want to use it simply as a cookie, or
pointer to a piece of static data). Now this object is freed by
the client's remove callback function instead.
* The error path of virNetDevMacVLanCreateWithVPortProfile had
been missing a call to free the callback data object if the
strdup(cr_ifname) failed. Now with even more error cases (since
all the other fields are also dynamically allocated), it became
even more apparent that disassociate_exit: needed to do
this. Since all of these cases need to call
virReportOOMError(), I factored that out into a new goto
label "memory_error".
* Due to a change in the netlink event service code, the remove callback
function is now given when calling virNetlinkEventAddClient
rather than when calling virNetlinkEventRemoveClient.
---
src/util/virnetdevmacvlan.c | 79 +++++++++++------
src/util/virnetlink.c | 197 ++++++++++++++++++++++++-------------------
src/util/virnetlink.h | 7 +-
3 files changed, 165 insertions(+), 118 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 1c4d082..d36b326 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -450,9 +450,9 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = {
struct virNetlinkCallbackData {
char *cr_ifname;
virNetDevVPortProfilePtr virtPortProfile;
- const unsigned char *macaddress;
- const char *linkdev;
- const unsigned char *vmuuid;
+ unsigned char *macaddress;
+ char *linkdev;
+ unsigned char *vmuuid;
enum virNetDevVPortProfileOp vmOp;
unsigned int linkState;
};
@@ -726,6 +726,29 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg,
}
/**
+ * virNetlinkCallbackDataFree
+ *
+ * @calld: pointer to a virNetlinkCallbackData object to free
+ *
+ * This function frees all the data associated with a virNetlinkCallbackData object
+ * as well as the object itself. If called with NULL, it does nothing.
+ *
+ * Returns nothing.
+ */
+static void
+virNetlinkCallbackDataFree(virNetlinkCallbackDataPtr calld)
+{
+ if (calld) {
+ VIR_FREE(calld->cr_ifname);
+ VIR_FREE(calld->virtPortProfile);
+ VIR_FREE(calld->macaddress);
+ VIR_FREE(calld->linkdev);
+ VIR_FREE(calld->vmuuid);
+ }
+ VIR_FREE(calld);
+}
+
+/**
* virNetDevMacVLanVPortProfileDestroyCallback:
*
* @watch: watch whose handle to remove
@@ -734,18 +757,14 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg,
*
* This function is called when a netlink message handler is terminated.
* The function frees locally allocated data referenced in the opaque
- * data.
+ * data, and the opaque object itself.
*/
static void
virNetDevMacVLanVPortProfileDestroyCallback(int watch ATTRIBUTE_UNUSED,
const unsigned char *macaddr ATTRIBUTE_UNUSED,
void *opaque)
{
- virNetlinkCallbackDataPtr calld = opaque;
-
- if (calld) {
- VIR_FREE(calld->cr_ifname);
- }
+ virNetlinkCallbackDataFree((virNetlinkCallbackDataPtr)opaque);
}
@@ -789,6 +808,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
int retries, do_retry = 0;
uint32_t macvtapMode;
const char *cr_ifname;
+ virNetlinkCallbackDataPtr calld = NULL;
int ret;
macvtapMode = modeMap[mode];
@@ -894,30 +914,35 @@ create_name:
}
if (virNetlinkEventServiceIsRunning()) {
- virNetlinkCallbackDataPtr calld;
+ if (VIR_ALLOC(calld) < 0)
+ goto memory_error;
+ if ((calld->cr_ifname = strdup(cr_ifname)) == NULL)
+ goto memory_error;
+ if (VIR_ALLOC(calld->virtPortProfile) < 0)
+ goto memory_error;
+ memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
+ if (VIR_ALLOC_N(calld->macaddress, VIR_MAC_BUFLEN) < 0)
+ goto memory_error;
+ memcpy(calld->macaddress, macaddress, VIR_MAC_BUFLEN);
+ if ((calld->linkdev = strdup(linkdev)) == NULL)
+ goto memory_error;
+ if (VIR_ALLOC_N(calld->vmuuid, VIR_UUID_BUFLEN) < 0)
+ goto memory_error;
+ memcpy(calld->vmuuid, vmuuid, VIR_UUID_BUFLEN);
- if (VIR_ALLOC(calld) < 0) {
- virReportOOMError();
- goto disassociate_exit;
- }
-
- calld->cr_ifname=strdup(cr_ifname);
- if (calld->cr_ifname == NULL) {
- virReportOOMError();
- goto disassociate_exit;
- }
-
- calld->virtPortProfile = virtPortProfile;
- calld->macaddress = macaddress;
- calld->linkdev = linkdev;
- calld->vmuuid = vmuuid;
calld->vmOp = vmOp;
- virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, calld, macaddress);
+ virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
+ virNetDevMacVLanVPortProfileDestroyCallback,
+ calld, macaddress);
}
return rc;
+ memory_error:
+ virReportOOMError();
+ virNetlinkCallbackDataFree(calld);
+
disassociate_exit:
ignore_value(virNetDevVPortProfileDisassociate(cr_ifname,
virtPortProfile,
@@ -965,7 +990,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
ret = -1;
}
- virNetlinkEventRemoveClient(0, macaddr, virNetDevMacVLanVPortProfileDestroyCallback);
+ virNetlinkEventRemoveClient(0, macaddr);
return ret;
}
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index de6a135..fd6f751 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -53,7 +53,8 @@
/* State for a single netlink event handle */
struct virNetlinkEventHandle {
int watch;
- virNetlinkEventHandleCallback cb;
+ virNetlinkEventHandleCallback handleCB;
+ virNetlinkEventRemoveCallback removeCB;
void *opaque;
unsigned char macaddr[VIR_MAC_BUFLEN];
int deleted;
@@ -182,20 +183,49 @@ error:
}
static void
-virNetlinkEventServerLock(virNetlinkEventSrvPrivatePtr driver) {
+virNetlinkEventServerLock(virNetlinkEventSrvPrivatePtr driver)
+{
virMutexLock(&driver->lock);
}
static void
-virNetlinkEventServerUnlock(virNetlinkEventSrvPrivatePtr driver) {
+virNetlinkEventServerUnlock(virNetlinkEventSrvPrivatePtr driver)
+{
virMutexUnlock(&driver->lock);
}
+/**
+ * virNetlinkEventRemoveClientPrimitive:
+ *
+ * @i: index of the client to remove from the table
+ *
+ * This static function does the low level removal of a client from
+ * the table once its index is known, including calling the remove
+ * callback (which usually will free resources required by the
+ * handler). The event server lock *must* be locked before calling
+ * this function.
+ *
+ * assumes success, returns nothing.
+ */
+static void
+virNetlinkEventRemoveClientPrimitive(size_t i)
+{
+ virNetlinkEventRemoveCallback removeCB = server->handles[i].removeCB;
+
+ if (removeCB) {
+ (removeCB)(server->handles[i].watch,
+ server->handles[i].macaddr,
+ server->handles[i].opaque);
+ }
+ server->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED;
+}
+
static void
virNetlinkEventCallback(int watch,
int fd ATTRIBUTE_UNUSED,
int events ATTRIBUTE_UNUSED,
- void *opaque) {
+ void *opaque)
+{
virNetlinkEventSrvPrivatePtr srv = opaque;
unsigned char *msg;
struct sockaddr_nl peer;
@@ -219,23 +249,18 @@ virNetlinkEventCallback(int watch,
(int)srv->handlesCount, watch);
for (i = 0; i < srv->handlesCount; i++) {
- if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) {
+ if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID)
continue;
- }
- VIR_DEBUG("dispatching client %d.",i);
+ VIR_DEBUG("dispatching client %d.", i);
- virNetlinkEventHandleCallback cb = srv->handles[i].cb;
- void *cpopaque = srv->handles[i].opaque;
- (cb)( msg, length, &peer, &handled, cpopaque);
+ (srv->handles[i].handleCB)(msg, length, &peer, &handled,
+ srv->handles[i].opaque);
}
- if (!handled) {
+ if (!handled)
VIR_DEBUG("event not handled.");
- }
-
VIR_FREE(msg);
-
virNetlinkEventServerUnlock(srv);
}
@@ -248,29 +273,32 @@ virNetlinkEventCallback(int watch,
* returns -1 if the monitor cannot be unregistered, 0 upon success
*/
int
-virNetlinkEventServiceStop(void) {
+virNetlinkEventServiceStop(void)
+{
virNetlinkEventSrvPrivatePtr srv = server;
+ int i;
VIR_INFO("stopping netlink event service");
- if (!server) {
+ if (!server)
return 0;
- }
virNetlinkEventServerLock(srv);
-
nl_close(srv->netlinknh);
nl_handle_destroy(srv->netlinknh);
-
virEventRemoveHandle(srv->eventwatch);
- server=0;
+ /* free any remaining clients on the list */
+ for (i = 0; i < srv->handlesCount; i++) {
+ if (srv->handles[i].deleted == VIR_NETLINK_HANDLE_VALID)
+ virNetlinkEventRemoveClientPrimitive(i);
+ }
+
+ server = 0;
virNetlinkEventServerUnlock(srv);
virMutexDestroy(&srv->lock);
-
VIR_FREE(srv);
-
return 0;
}
@@ -282,7 +310,8 @@ virNetlinkEventServiceStop(void) {
* returns 'true' if the service is running, 'false' if stopped.
*/
bool
-virNetlinkEventServiceIsRunning(void) {
+virNetlinkEventServiceIsRunning(void)
+{
return (server != NULL);
}
@@ -295,14 +324,14 @@ virNetlinkEventServiceIsRunning(void) {
* returns -1 if the monitor cannot be registered, 0 upon success
*/
int
-virNetlinkEventServiceStart(void) {
+virNetlinkEventServiceStart(void)
+{
virNetlinkEventSrvPrivatePtr srv;
int fd;
int ret = -1;
- if (server) {
+ if (server)
return 0;
- }
VIR_INFO("starting netlink event service");
@@ -311,7 +340,7 @@ virNetlinkEventServiceStart(void) {
goto error;
}
- if ( virMutexInit(&srv->lock) < 0)
+ if (virMutexInit(&srv->lock) < 0)
goto error;
virNetlinkEventServerLock(srv);
@@ -340,35 +369,34 @@ virNetlinkEventServiceStart(void) {
}
if (nl_socket_set_nonblocking(srv->netlinknh)) {
- netlinkError(errno,
- "%s", _("cannot set netlink socket nonblocking"));
+ netlinkError(errno, "%s",
+ _("cannot set netlink socket nonblocking"));
goto error_server;
}
if ((srv->eventwatch = virEventAddHandle(fd,
- VIR_EVENT_HANDLE_READABLE,
- virNetlinkEventCallback,
- srv, NULL)) < 0) {
+ VIR_EVENT_HANDLE_READABLE,
+ virNetlinkEventCallback,
+ srv, NULL)) < 0) {
netlinkError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to add netlink event handle watch"));
-
goto error_server;
}
srv->netlinkfd = fd;
- VIR_DEBUG("netlink event listener on fd: %i running",fd);
+ VIR_DEBUG("netlink event listener on fd: %i running", fd);
ret = 0;
- server=srv;
+ server = srv;
error_server:
- if (ret == -1){
+ if (ret < 0) {
nl_close(srv->netlinknh);
nl_handle_destroy(srv->netlinknh);
}
error_locked:
virNetlinkEventServerUnlock(srv);
- if (ret == -1) {
+ if (ret < 0) {
virMutexDestroy(&srv->lock);
VIR_FREE(srv);
}
@@ -379,7 +407,8 @@ error:
/**
* virNetlinkEventAddClient:
*
- * @cb: callback to invoke when an event occurs
+ * @handleCB: callback to invoke when an event occurs
+ * @removeCB: callback to invoke when removing a client
* @opaque: user data to pass to callback
* @macaddr: macaddr to store with the data. Used to identify callers. May be null.
*
@@ -390,18 +419,19 @@ error:
* returns -1 if the file handle cannot be registered, number of monitor upon success
*/
int
-virNetlinkEventAddClient(virNetlinkEventHandleCallback cb,
- void *opaque,
- const unsigned char *macaddr) {
- int i,r, result;
+virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB,
+ virNetlinkEventRemoveCallback removeCB,
+ void *opaque, const unsigned char *macaddr)
+{
+ int i, r, ret = -1;
virNetlinkEventSrvPrivatePtr srv = server;
- if ( cb == NULL )
+ if (handleCB == NULL)
return -1;
virNetlinkEventServerLock(srv);
- VIR_DEBUG("adding client: %d.",nextWatch);
+ VIR_DEBUG("adding client: %d.", nextWatch);
r = 0;
/* first try to re-use deleted free slots */
@@ -414,31 +444,31 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback cb,
/* Resize the eventLoop array if needed */
if (srv->handlesCount == srv->handlesAlloc) {
VIR_DEBUG("Used %zu handle slots, adding at least %d more",
- srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT);
+ srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT);
if (VIR_RESIZE_N(srv->handles, srv->handlesAlloc,
srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) {
- result = -1;
goto error;
}
}
r = srv->handlesCount++;
addentry:
- srv->handles[r].watch = nextWatch;
- srv->handles[r].cb = cb;
- srv->handles[r].opaque = opaque;
- srv->handles[r].deleted = VIR_NETLINK_HANDLE_VALID;
+ srv->handles[r].watch = nextWatch;
+ srv->handles[r].handleCB = handleCB;
+ srv->handles[r].removeCB = removeCB;
+ srv->handles[r].opaque = opaque;
+ srv->handles[r].deleted = VIR_NETLINK_HANDLE_VALID;
if (macaddr)
memcpy(srv->handles[r].macaddr, macaddr, VIR_MAC_BUFLEN);
+ else
+ memset(srv->handles[r].macaddr, 0, VIR_MAC_BUFLEN);
VIR_DEBUG("added client to loop slot: %d. with macaddr ptr=%p", r, macaddr);
- result = nextWatch++;
-
+ ret = nextWatch++;
error:
virNetlinkEventServerUnlock(srv);
-
- return result;
+ return ret;
}
/**
@@ -446,7 +476,6 @@ error:
*
* @watch: watch whose handle to remove
* @macaddr: macaddr whose handle to remove
- * &cb: callback for the destruction of local data
*
* Unregister a callback from a netlink monitor.
* The handler function referenced will no longer receive netlink messages.
@@ -455,14 +484,13 @@ error:
* returns -1 if the file handle was not registered, 0 upon success
*/
int
-virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
- virNetlinkEventRemoveCallback cb) {
+virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr)
+{
int i;
int ret = -1;
virNetlinkEventSrvPrivatePtr srv = server;
- VIR_DEBUG("removing client watch=%d, mac=%p.",
- watch, macaddr);
+ VIR_DEBUG("removing client watch=%d, mac=%p.", watch, macaddr);
if (watch <= 0 && !macaddr) {
VIR_WARN("Ignoring invalid netlink client id: %d", watch);
@@ -473,34 +501,22 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
for (i = 0; i < srv->handlesCount; i++) {
if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID)
- continue;
+ continue;
- if (watch && srv->handles[i].watch == watch) {
- void *cpopaque = srv->handles[i].opaque;
- (cb)( watch, macaddr, cpopaque);
+ if ((watch && srv->handles[i].watch == watch) ||
+ (!watch &&
+ memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0)) {
- VIR_FREE(srv->handles[i].opaque);
- srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED;
- VIR_DEBUG("removed client: %d by index.",
- srv->handles[i].watch);
- ret = 0;
- goto error;
- }
- if (!watch && memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0) {
- void *cpopaque = srv->handles[i].opaque;
- (cb)( watch, macaddr, cpopaque);
-
- VIR_FREE(srv->handles[i].opaque);
- srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED;
- VIR_DEBUG("removed client: %d by mac.",
- srv->handles[i].watch);
+ virNetlinkEventRemoveClientPrimitive(i);
+ VIR_DEBUG("removed client: %d by %s.", srv->handles[i].watch,
+ srv->handles[i].watch ? "index" : "mac");
ret = 0;
- goto error;
+ goto cleanup;
}
}
- VIR_DEBUG("client not found to remove.");
+ VIR_DEBUG("no client found to remove.");
-error:
+cleanup:
virNetlinkEventServerUnlock(srv);
return ret;
}
@@ -524,7 +540,8 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED,
/**
* stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd
*/
-int virNetlinkEventServiceStop(void) {
+int virNetlinkEventServiceStop(void)
+{
netlinkError(VIR_ERR_INTERNAL_ERROR,
"%s",
# if defined(__linux__) && !defined(HAVE_LIBNL)
@@ -536,7 +553,8 @@ int virNetlinkEventServiceStop(void) {
/**
* startNetlinkEventServer: start a monitor to receive netlink messages for libvirtd
*/
-int virNetlinkEventServiceStart(void) {
+int virNetlinkEventServiceStart(void)
+{
# if defined(__linux__) && !defined(HAVE_LIBNL)
netlinkError(VIR_ERR_INTERNAL_ERROR,
"%s",
@@ -548,7 +566,8 @@ int virNetlinkEventServiceStart(void) {
/**
* virNetlinkEventServiceIsRunning: returns if the netlink event service is running.
*/
-int virNetlinkEventServiceIsRunning(void) {
+int virNetlinkEventServiceIsRunning(void)
+{
# if defined(__linux__) && !defined(HAVE_LIBNL)
netlinkError(VIR_ERR_INTERNAL_ERROR,
"%s",
@@ -560,8 +579,10 @@ int virNetlinkEventServiceIsRunning(void) {
/**
* virNetlinkEventAddClient: register a callback for handling of netlink messages
*/
-int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque,
- const unsigned char *macaddr) {
+int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb,
+ virNetlinkEventRemoveCallback cb,
+ void *opaque, const unsigned char *macaddr)
+{
netlinkError(VIR_ERR_INTERNAL_ERROR,
"%s",
# if defined(__linux__) && !defined(HAVE_LIBNL)
@@ -575,8 +596,8 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque,
/**
* virNetlinkEventRemoveClient: unregister a callback from a netlink monitor
*/
-int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
- virNetlinkEventRemoveCallback cb) {
+int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr)
+{
netlinkError(VIR_ERR_INTERNAL_ERROR,
"%s",
# if defined(__linux__) && !defined(HAVE_LIBNL)
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index b787a8f..75533a3 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -60,12 +60,13 @@ bool virNetlinkEventServiceIsRunning(void);
/**
* virNetlinkEventAddClient: register a callback for handling of netlink messages
*/
-int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, const unsigned char *macaddr);
+int virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB,
+ virNetlinkEventRemoveCallback removeCB,
+ void *opaque, const unsigned char *macaddr);
/**
* virNetlinkEventRemoveClient: unregister a callback from a netlink monitor
*/
-int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
- virNetlinkEventRemoveCallback cb);
+int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr);
#endif /* __VIR_NETLINK_H__ */
--
1.7.7.6
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list