MINOR: event_hdl: dynamically allocated event data members

Add the ability to provide a cleanup function for event data passed
via the publishing function.

One use case could be the need to provide valid pointers in the safe
section of the data struct.
Cleanup function will be automatically called with data (or copy of data)
as argument when all handlers consumed the event, which provides an easy
way to release some memory or decrement refcounts to ressources that were
provided through the data struct.
data in itself may not be freed by the cleanup function, it is handled
by the API.

This would allow passing large (allocated) data blocks through the data
struct while keeping data struct size under the EVENT_HDL_ASYNC_EVENT_DATA
size limit.

To do so, when publishing an event, where we would currently do:

        struct event_hdl_cb_data_new_family event_data;

        /* safe data, available from both sync and async contexts
	 * may not use pointers to short-living resources
	 */
        event_data.safe.my_custom_data = x;

        /* unsafe data, only available from sync contexts */
        event_data.unsafe.my_unsafe_data = y;

        /* once data is prepared, we can publish the event */
        event_hdl_publish(NULL,
                          EVENT_HDL_SUB_NEW_FAMILY_SUBTYPE_1,
                          EVENT_HDL_CB_DATA(&event_data));

We could do:

        struct event_hdl_cb_data_new_family event_data;

        /* safe data, available from both sync and async contexts
	 * may not use pointers to short-living resources,
	 * unless EVENT_HDL_CB_DATA_DM is used to ensure pointer
	 * consistency (ie: refcount)
	 */
        event_data.safe.my_custom_static_data = x;
	event_data.safe.my_custom_dynamic_data = malloc(1);

        /* unsafe data, only available from sync contexts */
        event_data.unsafe.my_unsafe_data = y;

        /* once data is prepared, we can publish the event */
        event_hdl_publish(NULL,
                          EVENT_HDL_SUB_NEW_FAMILY_SUBTYPE_1,
                          EVENT_HDL_CB_DATA_DM(&event_data, data_new_family_cleanup));

With data_new_family_cleanup func which would look like this:

      void data_new_family_cleanup(const void *data)
      {
      	const struct event_hdl_cb_data_new_family *event_data = ptr;

	/* some data members require specific cleanup once the event
	 * is consumed
	 */
      	free(event_data.safe.my_custom_dynamic_data);
	/* don't ever free data! it is not ours */
      }

Not sure if this feature will become relevant in the future, so I prefer not
to mention it in the doc for now.

But given that the implementation is trivial and does not put a burden
on the existing API, it's a good thing to have it there, just in case.

diff --git a/include/haproxy/event_hdl-t.h b/include/haproxy/event_hdl-t.h
index 9efb548..a8062d1 100644 (file)
--- a/include/haproxy/event_hdl-t.h
+++ b/include/haproxy/event_hdl-t.h
@@ -56,10 +56,15 @@ struct event_hdl_cb_data_template {
  * refcount is used to keep track of references so that
  * data can be freed when not used anymore
  */
+typedef void (*event_hdl_data_free)(const void *data);
 struct event_hdl_async_event_data
 {
        /* internal storage */
        char data[EVENT_HDL_ASYNC_EVENT_DATA];
+       /* user-provided free function if event data relies on
+        * dynamic members that require specific cleanup
+        */
+       event_hdl_data_free mfree;
        uint32_t refcount;
 };
 
@@ -126,6 +131,10 @@ struct event_hdl_cb_data {
        void *_ptr;
        /* internal use: holds actual data size*/
        size_t _size;
+       /* user specified freeing function for event_hdl_cb_data_type
+        * struct members
+        */
+       event_hdl_data_free _mfree;
 };
 
 /* struct provided to event_hdl_cb_* handlers

diff --git a/include/haproxy/event_hdl.h b/include/haproxy/event_hdl.h
index 549a9f2..f174bbd 100644 (file)
--- a/include/haproxy/event_hdl.h
+++ b/include/haproxy/event_hdl.h
@@ -346,10 +346,34 @@ int event_hdl_lookup_resume(event_hdl_sub_list *sub_list,
                char __static_assert[(size <= EVENT_HDL_ASYNC_EVENT_DATA) ? 1 : -1];\
                (size);                                                 \
        })
-#define _EVENT_HDL_CB_DATA(data,size)                                  \
+#define _EVENT_HDL_CB_DATA(data,size,mfree)                            \
        (&(struct event_hdl_cb_data){ ._ptr = data,                     \
-                                     ._size = size })
-#define EVENT_HDL_CB_DATA(data)                _EVENT_HDL_CB_DATA(data, _EVENT_HDL_CB_DATA_ASSERT(sizeof(*data)))
+                                     ._size = size,                    \
+                                     ._mfree = mfree })
+
+/* Use this when 'safe' data is completely standalone */
+#define EVENT_HDL_CB_DATA(data)                                                \
+       _EVENT_HDL_CB_DATA(data,                                        \
+                          _EVENT_HDL_CB_DATA_ASSERT(sizeof(*data)),    \
+                          NULL)
+/* Use this when 'safe' data points to dynamically allocated members
+ * that require freeing when the event is completely consumed
+ * (data in itself may be statically allocated as with
+ *  EVENT_HDL_CB_DATA since the publish function will take
+ *  care of copying it for async handlers)
+ *
+ * mfree function will be called with data as argument
+ * (or copy of data in async context) when the event is completely
+ * consumed (sync and async handlers included). This will give you
+ * enough context to perform the required cleanup steps.
+ *
+ * mfree should be prototyped like this:
+ *    void (*mfree)(const void *data)
+ */
+#define EVENT_HDL_CB_DATA_DM(data, mfree)                              \
+       _EVENT_HDL_CB_DATA(data,                                        \
+                          _EVENT_HDL_CB_DATA_ASSERT(sizeof(*data)),    \
+                          mfree)
 
 /* event publishing function
  * this function should be called from anywhere in the code to notify

diff --git a/src/event_hdl.c b/src/event_hdl.c
index 14aed7a..5ec8123 100644 (file)
--- a/src/event_hdl.c
+++ b/src/event_hdl.c
@@ -177,6 +177,17 @@ static inline void _event_hdl_async_data_drop(struct event_hdl_async_event_data
 {
        if (HA_ATOMIC_SUB_FETCH(&data->refcount, 1) == 0) {
                /* we were the last one holding a reference to event data - free required */
+               if (data->mfree) {
+                       /* Some event data members are dynamically allocated and thus
+                        * require specific cleanup using user-provided function.
+                        * We directly pass a pointer to internal data storage but
+                        * we only expect the cleanup function to typecast it in the
+                        * relevant data type to give enough context to the function to
+                        * perform the cleanup on data members, and not actually freeing
+                        * data pointer since it is our internal buffer :)
+                        */
+                       data->mfree(&data->data);
+               }
                pool_free(pool_head_sub_event_data, data);
        }
 }
@@ -828,7 +839,8 @@ static int _event_hdl_publish(event_hdl_sub_list *sub_list, struct event_hdl_sub
                                new_event->sub_mgmt = EVENT_HDL_SUB_MGMT_ASYNC(cur_sub);
                                if (data) {
                                        /* if this fails, please adjust EVENT_HDL_ASYNC_EVENT_DATA in
-                                        * event_hdl-t.h file
+                                        * event_hdl-t.h file or consider providing dynamic struct members
+                                        * to reduce overall struct size
                                         */
                                        BUG_ON(data->_size > sizeof(async_data->data));
                                        if (!async_data) {
@@ -842,6 +854,7 @@ static int _event_hdl_publish(event_hdl_sub_list *sub_list, struct event_hdl_sub
 
                                                /* async data assignment */
                                                memcpy(async_data->data, data->_ptr, data->_size);
+                                               async_data->mfree = data->_mfree;
                                                /* Initialize refcount, we start at 1 to prevent async
                                                 * data from being freed by an async handler while we
                                                 * still use it. We will drop the reference when the
@@ -873,6 +886,12 @@ static int _event_hdl_publish(event_hdl_sub_list *sub_list, struct event_hdl_sub
        if (async_data) {
                /* we finished publishing, drop the reference on async data */
                _event_hdl_async_data_drop(async_data);
+       } else {
+               /* no async subscribers, we are responsible for calling the data
+                * member freeing function if it was provided
+                */
+               if (data && data->_mfree)
+                       data->_mfree(data->_ptr);
        }
        if (error) {
                event_hdl_report_hdl_state(ha_warning, &cur_sub->hdl, "PUBLISH", "memory error");