Since I found a couple other problems, but have made you suffer through
enough back and forth already, I've made some final suggested changes
myself, and am sending a diff patch as a response to this message
(differences from your latest version to mine), as well as doing a
repost of the original two patches with your latest changes and mine
squashed in.
Please 1) take a look at the changes in my diff patches , then 2) test
the updated versions of the full patches (I'm unable to test beyond
compiling), and send your ACK if they're okay. Once I have your ACK
back, I'll push (I promise, I won't find any new issues *this* time :-)
On 02/28/2012 10:34 AM, D. Herrendoerfer wrote:
> From: "D. Herrendoerfer" <d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
>
> this patch adds the changes proposed by Laine Stump to
> netlink event code.
>
> Signed-off-by: D. Herrendoerfer <d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
> ---
> src/util/virnetdevmacvlan.c | 47 +++++++++++++++++++++++++++++++++---------
> src/util/virnetlink.c | 13 ++++++++++-
> src/util/virnetlink.h | 7 ++++-
> 3 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 4256349..155ce79 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -448,7 +448,7 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = {
>
> /* Struct to hold the state and configuration of a 802.1qbg port */
> struct virNetlinkCallbackData {
> - char cr_ifname[64];
> + char *cr_ifname;
> virNetDevVPortProfilePtr virtPortProfile;
> const unsigned char *macaddress;
> const char *linkdev;
> @@ -606,15 +606,13 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg,
> if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN))
> {
> /* Repeat the same check for a broadcast mac */
> - unsigned char broadcastmac[VIR_MAC_BUFLEN];
> int i;
>
> - for (i = 0;i < VIR_MAC_BUFLEN; i++)
> - broadcastmac[i] = 0xFF;
> -
> - if (memcmp(calld->macaddress, broadcastmac, VIR_MAC_BUFLEN)) {
> - VIR_DEBUG("MAC address match failed.");
> - return;
> + for (i = 0;i < VIR_MAC_BUFLEN; i++) {
> + if (calld->macaddress[i] != 0xff) {
> + VIR_DEBUG("MAC address match failed (wasn't broadcast)");
> + return;
> + }
> }
> }
> }
> @@ -728,6 +726,30 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg,
> }
>
> /**
> + * virNetDevMacVLanVPortProfileDestroyCallback:
> + *
> + * @watch: watch whose handle to remove
> + * @macaddr: macaddr whose handle to remove
> + * @opaque: Contains vital information regarding the associated vm
> + *
> + * This function is called when a netlink message handler is terminated.
> + * The function frees locally allocated data referenced in the opaque
> + * data.
> + */
> +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);
> + }
> +}
You also need to do "VIR_FREE(calld) here, right?
> +
> +
> +/**
> * virNetDevMacVLanCreateWithVPortProfile:
> * Create an instance of a macvtap device and open its tap character
> * device.
> @@ -879,7 +901,12 @@ create_name:
> goto disassociate_exit;
> }
>
> - strncpy(calld->cr_ifname, cr_ifname, 64);
> + calld->cr_ifname=strdup(cr_ifname);
> + if (calld->cr_ifname == NULL) {
> + virReportOOMError();
> + goto disassociate_exit;
> + }
> +
> calld->virtPortProfile = virtPortProfile;
> calld->macaddress = macaddress;
> calld->linkdev = linkdev;
> @@ -938,7 +965,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
> ret = -1;
> }
>
> - virNetlinkEventRemoveClient(0, macaddr);
> + virNetlinkEventRemoveClient(0, macaddr, virNetDevMacVLanVPortProfileDestroyCallback);
>
> return ret;
> }
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index 751aa67..de6a135 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -446,6 +446,7 @@ 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.
> @@ -454,7 +455,8 @@ error:
> * returns -1 if the file handle was not registered, 0 upon success
> */
> int
> -virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
> +virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
> + virNetlinkEventRemoveCallback cb) {
Once I actually looked at my suggestion in practice, I realized that, in
order for virNetlinkEventServiceStart to be able to properly remove
clients (and thus avoid memory leaks), the remove callback needs to be
given when *adding* the client, and stored in the client data. I made
that modification, and will be sending a diff patch, as well as updated
final versions of both original patches.
> int i;
> int ret = -1;
> virNetlinkEventSrvPrivatePtr srv = server;
> @@ -474,6 +476,9 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
> continue;
>
> if (watch && srv->handles[i].watch == watch) {
> + void *cpopaque = srv->handles[i].opaque;
> + (cb)( watch, macaddr, cpopaque);
This should only be called if non-NULL.
> +
> VIR_FREE(srv->handles[i].opaque);
Ah, I missed that line before. Having this in the server code makes it
not exactly opaque - you're requiring that it point to data that is
dynamically allocated. I think it would be better to leave this free up
to the callback (possibly a client will want to point to static data, or
maybe use it to store a handle rather than an actual pointer).
> srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED;
> VIR_DEBUG("removed client: %d by index.",
> @@ -482,6 +487,9 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
> 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.",
Also, I missed all this duplicated code - it's easier to maintain if you
combine the two if conditions and just do the body of the loop once (the
only difference is the last word of the debug message, and that can
easily be handled.
> @@ -567,7 +575,8 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque,
> /**
> * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor
> */
> -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
> +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
> + virNetlinkEventRemoveCallback cb) {
> 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 1365afa..b787a8f 100644
> --- a/src/util/virnetlink.h
> +++ b/src/util/virnetlink.h
> @@ -38,7 +38,9 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
> unsigned char **respbuf, unsigned int *respbuflen,
> int nl_pid);
>
> -typedef void (*virNetlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque);
> +typedef void (*virNetlinkEventHandleCallback)(unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque);
> +
> +typedef void (*virNetlinkEventRemoveCallback)(int watch, const unsigned char *macaddr, void *opaque);
>
> /**
> * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd
> @@ -63,6 +65,7 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, con
> /**
> * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor
> */
> -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr);
> +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr,
> + virNetlinkEventRemoveCallback cb);
>
> #endif /* __VIR_NETLINK_H__ */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list