hotplug: Improve internal structure and deregistration mechanism
authorChris Dickens <christopher.a.dickens@gmail.com>
Thu, 28 Dec 2017 07:30:16 +0000 (23:30 -0800)
committerChris Dickens <christopher.a.dickens@gmail.com>
Thu, 28 Dec 2017 07:30:16 +0000 (23:30 -0800)
This commit shrinks the size of the internal hotplug callback structure
by removing unused fields, using the correctly sized types for matching
fields, and adding a new flags field whose bits control how the callback
structure should behave.

The hotplug callback handle ID counter has also been moved to the
context structure instead of being a global variable shared amongst all
contexts. This lets each context independently manage handle IDs and use
the maximum range of possible IDs.

Finally, the hotplug callback deregistration mechanism has been improved
to signal to the event handler that an explicit deregistration needs to
be handled. This removes the need to send a dummy hotplug message, which
was using an invalid libusb_hotplug_event value anyway that was causing
some compilers to complain.

Closes #373

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>
libusb/core.c
libusb/hotplug.c
libusb/hotplug.h
libusb/io.c
libusb/libusbi.h
libusb/version_nano.h

index b85721a..587a2d1 100644 (file)
@@ -2168,6 +2168,7 @@ int API_EXPORTED libusb_init(libusb_context **context)
        list_init(&ctx->usb_devs);
        list_init(&ctx->open_devs);
        list_init(&ctx->hotplug_cbs);
+       ctx->next_hotplug_cb_handle = 1;
 
        usbi_mutex_static_lock(&active_contexts_lock);
        if (first_init) {
@@ -2256,7 +2257,7 @@ void API_EXPORTED libusb_exit(struct libusb_context *ctx)
        usbi_mutex_static_unlock(&active_contexts_lock);
 
        if (libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) {
-               usbi_hotplug_deregister_all(ctx);
+               usbi_hotplug_deregister(ctx, 1);
 
                /*
                 * Ensure any pending unplug events are read from the hotplug
index bbfd6e7..508c9a1 100644 (file)
@@ -154,36 +154,30 @@ int main (void) {
 \endcode
  */
 
-static int usbi_hotplug_match_cb (struct libusb_context *ctx,
+static int usbi_hotplug_match_cb(struct libusb_context *ctx,
        struct libusb_device *dev, libusb_hotplug_event event,
        struct libusb_hotplug_callback *hotplug_cb)
 {
-       /* Handle lazy deregistration of callback */
-       if (hotplug_cb->needs_free) {
-               /* Free callback */
-               return 1;
-       }
-
-       if (!(hotplug_cb->events & event)) {
+       if (!(hotplug_cb->flags & event)) {
                return 0;
        }
 
-       if (LIBUSB_HOTPLUG_MATCH_ANY != hotplug_cb->vendor_id &&
+       if ((hotplug_cb->flags & USBI_HOTPLUG_VENDOR_ID_VALID) &&
            hotplug_cb->vendor_id != dev->device_descriptor.idVendor) {
                return 0;
        }
 
-       if (LIBUSB_HOTPLUG_MATCH_ANY != hotplug_cb->product_id &&
+       if ((hotplug_cb->flags & USBI_HOTPLUG_PRODUCT_ID_VALID) &&
            hotplug_cb->product_id != dev->device_descriptor.idProduct) {
                return 0;
        }
 
-       if (LIBUSB_HOTPLUG_MATCH_ANY != hotplug_cb->dev_class &&
+       if ((hotplug_cb->flags & USBI_HOTPLUG_DEV_CLASS_VALID) &&
            hotplug_cb->dev_class != dev->device_descriptor.bDeviceClass) {
                return 0;
        }
 
-       return hotplug_cb->cb (ctx, dev, event, hotplug_cb->user_data);
+       return hotplug_cb->cb(ctx, dev, event, hotplug_cb->user_data);
 }
 
 void usbi_hotplug_match(struct libusb_context *ctx, struct libusb_device *dev,
@@ -195,8 +189,13 @@ void usbi_hotplug_match(struct libusb_context *ctx, struct libusb_device *dev,
        usbi_mutex_lock(&ctx->hotplug_cbs_lock);
 
        list_for_each_entry_safe(hotplug_cb, next, &ctx->hotplug_cbs, list, struct libusb_hotplug_callback) {
+               if (hotplug_cb->flags & USBI_HOTPLUG_NEEDS_FREE) {
+                       /* process deregistration in usbi_hotplug_deregister() */
+                       continue;
+               }
+
                usbi_mutex_unlock(&ctx->hotplug_cbs_lock);
-               ret = usbi_hotplug_match_cb (ctx, dev, event, hotplug_cb);
+               ret = usbi_hotplug_match_cb(ctx, dev, event, hotplug_cb);
                usbi_mutex_lock(&ctx->hotplug_cbs_lock);
 
                if (ret) {
@@ -206,15 +205,13 @@ void usbi_hotplug_match(struct libusb_context *ctx, struct libusb_device *dev,
        }
 
        usbi_mutex_unlock(&ctx->hotplug_cbs_lock);
-
-       /* the backend is expected to call the callback for each active transfer */
 }
 
 void usbi_hotplug_notification(struct libusb_context *ctx, struct libusb_device *dev,
        libusb_hotplug_event event)
 {
        int pending_events;
-       libusb_hotplug_message *message = calloc(1, sizeof(*message));
+       struct libusb_hotplug_message *message = calloc(1, sizeof(*message));
 
        if (!message) {
                usbi_err(ctx, "error allocating hotplug message");
@@ -240,55 +237,66 @@ int API_EXPORTED libusb_hotplug_register_callback(libusb_context *ctx,
        libusb_hotplug_callback_fn cb_fn, void *user_data,
        libusb_hotplug_callback_handle *callback_handle)
 {
-       libusb_hotplug_callback *new_callback;
-       static int handle_id = 1;
-
-       /* check for hotplug support */
-       if (!libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) {
-               return LIBUSB_ERROR_NOT_SUPPORTED;
-       }
+       struct libusb_hotplug_callback *new_callback;
 
        /* check for sane values */
-       if ((LIBUSB_HOTPLUG_MATCH_ANY != vendor_id && (~0xffff & vendor_id)) ||
+       if ((!events || (~(LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED | LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT) & events)) ||
+           (flags && (~LIBUSB_HOTPLUG_ENUMERATE & flags)) ||
+           (LIBUSB_HOTPLUG_MATCH_ANY != vendor_id && (~0xffff & vendor_id)) ||
            (LIBUSB_HOTPLUG_MATCH_ANY != product_id && (~0xffff & product_id)) ||
            (LIBUSB_HOTPLUG_MATCH_ANY != dev_class && (~0xff & dev_class)) ||
            !cb_fn) {
                return LIBUSB_ERROR_INVALID_PARAM;
        }
 
+       /* check for hotplug support */
+       if (!libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) {
+               return LIBUSB_ERROR_NOT_SUPPORTED;
+       }
+
        USBI_GET_CONTEXT(ctx);
 
-       new_callback = (libusb_hotplug_callback *)calloc(1, sizeof (*new_callback));
+       new_callback = calloc(1, sizeof(*new_callback));
        if (!new_callback) {
                return LIBUSB_ERROR_NO_MEM;
        }
 
-       new_callback->ctx = ctx;
-       new_callback->vendor_id = vendor_id;
-       new_callback->product_id = product_id;
-       new_callback->dev_class = dev_class;
-       new_callback->flags = flags;
-       new_callback->events = events;
+       new_callback->flags = (uint8_t)events;
+       if (LIBUSB_HOTPLUG_MATCH_ANY != vendor_id) {
+               new_callback->flags |= USBI_HOTPLUG_VENDOR_ID_VALID;
+               new_callback->vendor_id = (uint16_t)vendor_id;
+       }
+       if (LIBUSB_HOTPLUG_MATCH_ANY != product_id) {
+               new_callback->flags |= USBI_HOTPLUG_PRODUCT_ID_VALID;
+               new_callback->product_id = (uint16_t)product_id;
+       }
+       if (LIBUSB_HOTPLUG_MATCH_ANY != dev_class) {
+               new_callback->flags |= USBI_HOTPLUG_DEV_CLASS_VALID;
+               new_callback->dev_class = (uint8_t)dev_class;
+       }
        new_callback->cb = cb_fn;
        new_callback->user_data = user_data;
-       new_callback->needs_free = 0;
 
        usbi_mutex_lock(&ctx->hotplug_cbs_lock);
 
-       /* protect the handle by the context hotplug lock. it doesn't matter if the same handle
-        * is used for different contexts only that the handle is unique for this context */
-       new_callback->handle = handle_id++;
+       /* protect the handle by the context hotplug lock */
+       new_callback->handle = ctx->next_hotplug_cb_handle++;
+
+       /* handle the unlikely case of overflow */
+       if (ctx->next_hotplug_cb_handle < 0)
+               ctx->next_hotplug_cb_handle = 1;
 
        list_add(&new_callback->list, &ctx->hotplug_cbs);
 
        usbi_mutex_unlock(&ctx->hotplug_cbs_lock);
 
+       usbi_dbg("new hotplug cb %p with handle %d", new_callback, new_callback->handle);
 
-       if (flags & LIBUSB_HOTPLUG_ENUMERATE) {
-               int i, len;
+       if ((flags & LIBUSB_HOTPLUG_ENUMERATE) && (events & LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED)) {
+               ssize_t i, len;
                struct libusb_device **devs;
 
-               len = (int) libusb_get_device_list(ctx, &devs);
+               len = libusb_get_device_list(ctx, &devs);
                if (len < 0) {
                        libusb_hotplug_deregister_callback(ctx,
                                                        new_callback->handle);
@@ -311,10 +319,11 @@ int API_EXPORTED libusb_hotplug_register_callback(libusb_context *ctx,
        return LIBUSB_SUCCESS;
 }
 
-void API_EXPORTED libusb_hotplug_deregister_callback (struct libusb_context *ctx,
+void API_EXPORTED libusb_hotplug_deregister_callback(struct libusb_context *ctx,
        libusb_hotplug_callback_handle callback_handle)
 {
        struct libusb_hotplug_callback *hotplug_cb;
+       int deregistered = 0;
 
        /* check for hotplug support */
        if (!libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) {
@@ -323,28 +332,42 @@ void API_EXPORTED libusb_hotplug_deregister_callback (struct libusb_context *ctx
 
        USBI_GET_CONTEXT(ctx);
 
+       usbi_dbg("deregister hotplug cb %d", callback_handle);
+
        usbi_mutex_lock(&ctx->hotplug_cbs_lock);
-       list_for_each_entry(hotplug_cb, &ctx->hotplug_cbs, list,
-                           struct libusb_hotplug_callback) {
+       list_for_each_entry(hotplug_cb, &ctx->hotplug_cbs, list, struct libusb_hotplug_callback) {
                if (callback_handle == hotplug_cb->handle) {
                        /* Mark this callback for deregistration */
-                       hotplug_cb->needs_free = 1;
+                       hotplug_cb->flags |= USBI_HOTPLUG_NEEDS_FREE;
+                       deregistered = 1;
                }
        }
        usbi_mutex_unlock(&ctx->hotplug_cbs_lock);
 
-       usbi_hotplug_notification(ctx, NULL, 0);
+       if (deregistered) {
+               int pending_events;
+
+               usbi_mutex_lock(&ctx->event_data_lock);
+               pending_events = usbi_pending_events(ctx);
+               ctx->event_flags |= USBI_EVENT_HOTPLUG_CB_DEREGISTERED;
+               if (!pending_events)
+                       usbi_signal_event(ctx);
+               usbi_mutex_unlock(&ctx->event_data_lock);
+       }
 }
 
-void usbi_hotplug_deregister_all(struct libusb_context *ctx) {
+void usbi_hotplug_deregister(struct libusb_context *ctx, int forced)
+{
        struct libusb_hotplug_callback *hotplug_cb, *next;
 
        usbi_mutex_lock(&ctx->hotplug_cbs_lock);
-       list_for_each_entry_safe(hotplug_cb, next, &ctx->hotplug_cbs, list,
-                                struct libusb_hotplug_callback) {
-               list_del(&hotplug_cb->list);
-               free(hotplug_cb);
+       list_for_each_entry_safe(hotplug_cb, next, &ctx->hotplug_cbs, list, struct libusb_hotplug_callback) {
+               if (forced || (hotplug_cb->flags & USBI_HOTPLUG_NEEDS_FREE)) {
+                       usbi_dbg("freeing hotplug cb %p with handle %d", hotplug_cb,
+                                hotplug_cb->handle);
+                       list_del(&hotplug_cb->list);
+                       free(hotplug_cb);
+               }
        }
-
        usbi_mutex_unlock(&ctx->hotplug_cbs_lock);
 }
index 2bec81b..dbadbcb 100644 (file)
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#if !defined(USBI_HOTPLUG_H)
+#ifndef USBI_HOTPLUG_H
 #define USBI_HOTPLUG_H
 
-#ifndef LIBUSBI_H
 #include "libusbi.h"
-#endif
+
+enum usbi_hotplug_flags {
+       /* This callback is interested in device arrivals */
+       USBI_HOTPLUG_DEVICE_ARRIVED = LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED,
+
+       /* This callback is interested in device removals */
+       USBI_HOTPLUG_DEVICE_LEFT = LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT,
+
+       /* IMPORTANT: The values for the below entries must start *after*
+        * the highest value of the above entries!!!
+        */
+
+       /* The vendor_id field is valid for matching */
+       USBI_HOTPLUG_VENDOR_ID_VALID = (1 << 3),
+
+       /* The product_id field is valid for matching */
+       USBI_HOTPLUG_PRODUCT_ID_VALID = (1 << 4),
+
+       /* The dev_class field is valid for matching */
+       USBI_HOTPLUG_DEV_CLASS_VALID = (1 << 5),
+
+       /* This callback has been unregistered and needs to be freed */
+       USBI_HOTPLUG_NEEDS_FREE = (1 << 6),
+};
 
 /** \ingroup hotplug
  * The hotplug callback structure. The user populates this structure with
  * to receive notification of hotplug events.
  */
 struct libusb_hotplug_callback {
-       /** Context this callback is associated with */
-       struct libusb_context *ctx;
-
-       /** Vendor ID to match or LIBUSB_HOTPLUG_MATCH_ANY */
-       int vendor_id;
-
-       /** Product ID to match or LIBUSB_HOTPLUG_MATCH_ANY */
-       int product_id;
+       /** Flags that control how this callback behaves */
+       uint8_t flags;
 
-       /** Device class to match or LIBUSB_HOTPLUG_MATCH_ANY */
-       int dev_class;
+       /** Vendor ID to match (if flags says this is valid) */
+       uint16_t vendor_id;
 
-       /** Hotplug callback flags */
-       libusb_hotplug_flag flags;
+       /** Product ID to match (if flags says this is valid) */
+       uint16_t product_id;
 
-       /** Event(s) that will trigger this callback */
-       libusb_hotplug_event events;
+       /** Device class to match (if flags says this is valid) */
+       uint8_t dev_class;
 
        /** Callback function to invoke for matching event/device */
        libusb_hotplug_callback_fn cb;
@@ -59,15 +75,10 @@ struct libusb_hotplug_callback {
        /** User data that will be passed to the callback function */
        void *user_data;
 
-       /** Callback is marked for deletion */
-       int needs_free;
-
        /** List this callback is registered in (ctx->hotplug_cbs) */
        struct list_head list;
 };
 
-typedef struct libusb_hotplug_callback libusb_hotplug_callback;
-
 struct libusb_hotplug_message {
        /** The hotplug event that occurred */
        libusb_hotplug_event event;
@@ -79,9 +90,7 @@ struct libusb_hotplug_message {
        struct list_head list;
 };
 
-typedef struct libusb_hotplug_message libusb_hotplug_message;
-
-void usbi_hotplug_deregister_all(struct libusb_context *ctx);
+void usbi_hotplug_deregister(struct libusb_context *ctx, int forced);
 void usbi_hotplug_match(struct libusb_context *ctx, struct libusb_device *dev,
                        libusb_hotplug_event event);
 void usbi_hotplug_notification(struct libusb_context *ctx, struct libusb_device *dev,
index 07e0ab8..a03bfaa 100644 (file)
@@ -2166,6 +2166,7 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv)
        if (fds[0].revents) {
                struct list_head hotplug_msgs;
                struct usbi_transfer *itransfer;
+               int hotplug_cb_deregistered = 0;
                int ret = 0;
 
                list_init(&hotplug_msgs);
@@ -2184,6 +2185,12 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv)
                        ctx->event_flags &= ~USBI_EVENT_USER_INTERRUPT;
                }
 
+               if (ctx->event_flags & USBI_EVENT_HOTPLUG_CB_DEREGISTERED) {
+                       usbi_dbg("someone unregistered a hotplug cb");
+                       ctx->event_flags &= ~USBI_EVENT_HOTPLUG_CB_DEREGISTERED;
+                       hotplug_cb_deregistered = 1;
+               }
+
                /* check if someone is closing a device */
                if (ctx->device_close)
                        usbi_dbg("someone is closing a device");
@@ -2211,10 +2218,13 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv)
 
                usbi_mutex_unlock(&ctx->event_data_lock);
 
+               if (hotplug_cb_deregistered)
+                       usbi_hotplug_deregister(ctx, 0);
+
                /* process the hotplug messages, if any */
                while (!list_empty(&hotplug_msgs)) {
-                       libusb_hotplug_message *message =
-                               list_first_entry(&hotplug_msgs, libusb_hotplug_message, list);
+                       struct libusb_hotplug_message *message =
+                               list_first_entry(&hotplug_msgs, struct libusb_hotplug_message, list);
 
                        usbi_hotplug_match(ctx, message->device, message->event);
 
index ac57312..31d6ce9 100644 (file)
@@ -308,6 +308,7 @@ struct libusb_context {
 
        /* A list of registered hotplug callbacks */
        struct list_head hotplug_cbs;
+       libusb_hotplug_callback_handle next_hotplug_cb_handle;
        usbi_mutex_t hotplug_cbs_lock;
 
        /* this is a list of in-flight transfer handles, sorted by timeout
@@ -379,6 +380,9 @@ enum usbi_event_flags {
 
        /* The user has interrupted the event handler */
        USBI_EVENT_USER_INTERRUPT = 1 << 1,
+
+       /* A hotplug callback deregistration is pending */
+       USBI_EVENT_HOTPLUG_CB_DEREGISTERED = 1 << 2,
 };
 
 /* Macros for managing event handling state */
index 0029bd2..423c3df 100644 (file)
@@ -1 +1 @@
-#define LIBUSB_NANO 11231
+#define LIBUSB_NANO 11232