From: Amos Jeffries Date: Wed, 4 Feb 2015 21:37:28 +0000 (-0800) Subject: Drop unused cbdata.h definitions and re-document X-Git-Tag: merge-candidate-3-v1~287 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ffc6d4e9f4ae9f36e9b3d77a83b202d8ef3dae26;p=thirdparty%2Fsquid.git Drop unused cbdata.h definitions and re-document Remove the now unused cbdataFree, cbdataAlloc, CBDATA_TYPE, CBDATA_INIT_TYPE, CBDATA_INIT_TYPE_FREECB symbols. Re-write CBDATA documentation to reflect the current available API symbols, their usage, and mechanisms that should be used instead of CBDATA such as AsyncJob/Call and RefCount. Along with some doxygen polishing to meet currently agreed style for how to document major code features. Make generic_cbdata::data a private member. The constructor and unwrap() operator provide all necessary public API. Replace store_client.cc use of cbdataInternalLock/Unlock with a CbcPointer<> smart pointer equivalent. The use remains an abuse of CBDATA, just no longer directly referencing the internal API functions. --- diff --git a/src/cbdata.cc b/src/cbdata.cc index 6c46a5de64..c125eb03f0 100644 --- a/src/cbdata.cc +++ b/src/cbdata.cc @@ -8,19 +8,6 @@ /* DEBUG: section 45 Callback Data Registry */ -/** - \defgroup CBDATAInternal Callback Data Allocator Internals - \ingroup CBDATAAPI - * - * These routines manage a set of registered callback data pointers. - * One of the easiest ways to make Squid coredump is to issue a - * callback to for some data structure which has previously been - * freed. With these routines, we register (add) callback data - * pointers, lock them just before registering the callback function, - * validate them before issuing the callback, and then free them - * when finished. - */ - #include "squid.h" #include "cbdata.h" #include "Generic.h" @@ -58,10 +45,17 @@ public: #endif -/// \ingroup CBDATAInternal #define OFFSET_OF(TYPE, MEMBER) ((size_t) &(((TYPE) *)0)->(MEMBER)) -/// \ingroup CBDATAInternal +/** + * Manage a set of registered callback data pointers. + * One of the easiest ways to make Squid coredump is to issue a + * callback to for some data structure which has previously been + * freed. With this class, we register (add) callback data + * pointers, lock them just before registering the callback function, + * validate them before issuing the callback, and then free them + * when finished. + */ class cbdata { #if !HASHED_CBDATA @@ -140,14 +134,12 @@ static OBJH cbdataDump; static OBJH cbdataDumpHistory; #endif -/// \ingroup CBDATAInternal struct CBDataIndex { MemAllocator *pool; FREE *free_func; } *cbdata_index = NULL; -/// \ingroup CBDATAInternal int cbdata_types = 0; #if HASHED_CBDATA diff --git a/src/cbdata.h b/src/cbdata.h index cf4c4a48af..c28c1c4b3c 100644 --- a/src/cbdata.h +++ b/src/cbdata.h @@ -6,41 +6,45 @@ * Please see the COPYING and CONTRIBUTORS files for details. */ -#ifndef SQUID_CBDATA_H -#define SQUID_CBDATA_H +#ifndef SQUID_SRC_CBDATA_H +#define SQUID_SRC_CBDATA_H #include "typedefs.h" /** - \defgroup CBDATAAPI Callback Data Allocator API - \ingroup Components +\page CBDATA Callback Data Allocator API + + \section Introduction + \par - * Squid's extensive use of callback functions makes it very - * susceptible to memory access errors. To address this all callback - * functions make use of a construct called cbdata. This allows - * functions doing callbacks to verify that the caller is still - * valid before making the callback. - * - \note cbdata is intended for callback data and is tailored specifically - * to make callbacks less dangerous leaving as few windows of errors as - * possible. It is not suitable or intended as a generic RefCount - * memory allocator. - * - * + Squid's extensive use of callback functions makes it very + susceptible to memory access errors. To address this all callback + functions make use of a construct called cbdata. This allows + functions doing callbacks to verify that the caller is still + valid before making the callback. + + \note cbdata is intended for callback data and is tailored specifically + to make callbacks less dangerous leaving as few windows of errors as + possible. It is not suitable or intended as a generic RefCount + memory allocator. + + \par + The AsyncJob/AsyncCall mechanism is preferred over CBDATA. + It replaces cbdata with an AsyncCall::Pointer object which + performs the same memory protection duties via other means. + \section Examples Examples \par - * Here you can find some examples on how to use cbdata, and why. - * + Here you can find some examples on how to use cbdata, and why. + \subsection AsyncOpWithoutCBDATA Asynchronous operation without cbdata, showing why cbdata is needed \par - * For a asyncronous operation with callback functions, the normal - * sequence of events in programs NOT using cbdata is as follows: - * + For a asyncronous operation with callback functions, the normal + sequence of events in programs NOT using cbdata is as follows: + \code // initialization - type_of_data our_data; - ... - our_data = malloc(...); + type_of_data our_data = new ...; ... // Initiate a asyncronous operation, with our_data as callback_data fooOperationStart(bar, callback_func, our_data); @@ -48,54 +52,50 @@ // The asyncronous operation completes and makes the callback callback_func(callback_data, ....); // Some time later we clean up our data - free(our_data); + delete our_data; \endcode - * + \par - * However, things become more interesting if we want or need - * to free the callback_data, or otherwise cancel the callback, - * before the operation completes. In constructs like this you - * can quite easily end up with having the memory referenced - * pointed to by callback_data freed before the callback is invoked - * causing a program failure or memory corruption: - * + However, things become more interesting if we want or need + to free the callback_data, or otherwise cancel the callback, + before the operation completes. In constructs like this you + can quite easily end up with having the memory referenced + pointed to by callback_data freed before the callback is invoked + causing a program failure or memory corruption: + \code // initialization - type_of_data our_data; - ... - our_data = malloc(...); + type_of_data our_data = new ...; ... // Initiate a asyncronous operation, with our_data as callback_data fooOperationStart(bar, callback_func, our_data); ... // ouch, something bad happened elsewhere.. try to cleanup // but the programmer forgot there is a callback pending from - // fooOperationsStart() (an easy thing to forget when writing code + // fooOperationsStart(). An easy thing to forget when writing code // to deal with errors, especially if there may be many different - // pending operation) - free(our_data); + // pending operations. + delete our_data; ... // The asyncronous operation completes and makes the callback callback_func(callback_data, ....); // CRASH, the memory pointer to by callback_data is no longer valid // at the time of the callback \endcode - * + \subsection AsyncOpWithCBDATA Asyncronous operation with cbdata - * + \par - * The callback data allocator lets us do this in a uniform and - * safe manner. The callback data allocator is used to allocate, - * track and free memory pool objects used during callback - * operations. Allocated memory is locked while the asyncronous - * operation executes elsewhere, and is freed when the operation - * completes. The normal sequence of events is: - * + The callback data allocator lets us do this in a uniform and + safe manner. The callback data allocator is used to allocate, + track and free memory pool objects used during callback + operations. Allocated memory is locked while the asyncronous + operation executes elsewhere, and is freed when the operation + completes. The normal sequence of events is: + \code // initialization - type_of_data our_data; - ... - our_data = cbdataAlloc(type_of_data); + type_of_data our_data = new type_of_data; ... // Initiate a asyncronous operation, with our_data as callback_data fooOperationStart(..., callback_func, our_data); @@ -107,102 +107,130 @@ void *cbdata; if (cbdataReferenceValidDone(local_pointer, &cbdata)) callback_func(...., cbdata); - ... - cbdataFree(our_data); + delete our_data; \endcode - * + \subsection AsynchronousOpCancelledByCBDATA Asynchronous operation cancelled by cbdata - * + \par - * With this scheme, nothing bad happens if cbdataFree() gets called - * before fooOperantionComplete(...). - * + With this scheme, nothing bad happens if delete gets called + before fooOperantionComplete(...). + \par Initalization \code - type_of_data our_data; + // initialization + type_of_data our_data = new type_of_data; ... - our_data = cbdataAlloc(type_of_data); - \endcode - * Initiate a asyncronous operation, with our_data as callback_data - \code + // Initiate a asyncronous operation, with our_data as callback_data fooOperationStart(..., callback_func, our_data); - \endcode - * do some stuff with it - \code + ... + // do some stuff with it void *local_pointer = cbdataReference(callback_data); - \endcode - * something bad happened elsewhere.. cleanup - \code - cbdataFree(our_data); - \endcode - * The asyncronous operation completes and tries to make the callback - \code + ... + // something bad happened elsewhere.. cleanup + delete our_data; + .... + // The asyncronous operation completes and makes the callback void *cbdata; if (cbdataReferenceValidDone(local_pointer, &cbdata)) - { - \endcode - * won't be called, as the data is no longer valid - \code + // won't be called, as the data is no longer valid callback_func(...., cbdata); - } + delete our_data; \endcode - * + \par - * In this case, when cbdataFree() is called before - * cbdataReferenceValidDone(), the callback_data gets marked as invalid. - * When the callback_data is invalid before executing the callback - * function, cbdataReferenceValidDone() will return 0 and - * callback_func is never executed. - * + In this case, when delete is called before cbdataReferenceValidDone(), + the callback_data gets marked as invalid. + When the callback_data is invalid before executing the callback + function, cbdataReferenceValidDone() will return 0 and + callback_func is never executed. + \subsection AddingCBDATAType Adding a new cbdata registered type - * + + \par + To add new module specific data types to the allocator one uses + the macro CBDATA_CLASS() in the class private section, and + CBDATA_CLASS_INIT() or CBDATA_NAMESPACED_CLASS_INIT() in the + class .cc file. + + \code + class Foo + { + CBDATA_CLASS(Foo); + + public: + Foo() {} + ~Foo() {} + }; + ... + CBDATA_CLASS_INIT(Foo); + \endcode + + \par + These macros create new(), delete() and toCbdata() methods + definition in class scope. Any allocate calls must be made with + new() and destruction with delete(), they may be called from + anywhere. + + \par + The class constructor must make sure that all member + variables are initialized, and the class destructor that all + dynamic memory is released. + \par - * To add new module specific data types to the allocator one uses the - * macro CBDATA_CLASS() in the class private section, and CBDATA_CLASS_INIT() - * or CBDATA_NAMESPACED_CLASS_INIT() in the .cc file. - * This creates new(), delete() and toCbdata() methods - * definition in class scope. Any allocate calls must be made with - * new() and destruction with delete(), they may be called from anywhere. + The CbcPointer<> template should be used to create a smart-pointer + type for simple reference tracking. It provides get() and valid() + accessors for use instead of cbdataReferenceValid(), and performs + reliable automatic cbdataReference() and cbdataReferenceDone() + tracking. + Note that it does NOT provide a replacement for cbdataReferenceValidDone(). + */ /** - *\ingroup CBDATAAPI * cbdata types. Similar to the MEM_* types, but managed in cbdata.cc * A big difference is that cbdata types are dynamically allocated. - * Initially only UNKNOWN type is predefined. Other types are added runtime. + * + * Initially only UNKNOWN type is predefined. + * Other types are added at runtime by CBDATA_CLASS(). */ typedef int cbdata_type; static const cbdata_type CBDATA_UNKNOWN = 0; -/// \ingroup CBDATAAPI +/** + * Create a run-time registration of CBDATA component with + * the Squid cachemgr + */ void cbdataRegisterWithCacheManager(void); /** * Allocates a new entry of a registered CBDATA type. - * \deprecated use CBDATA_CLASS() instead + * + * \note For internal CBDATA use only. */ void *cbdataInternalAlloc(cbdata_type type, const char *, int); -/// \deprecated use CBDATA_CLASS() instead -#define cbdataAlloc(type) ((type *)cbdataInternalAlloc(CBDATA_##type,__FILE__,__LINE__)) /** - * Frees a entry allocated by cbdataAlloc(). + * Frees a entry allocated by cbdataInternalAlloc(). + * + * Once this has been called cbdataReferenceValid() and + * cbdataReferenceValidDone() will return false regardless + * of whether there are remaining cbdata references. * - \note If there are active references to the entry then the entry - * will be freed with the last reference is removed. However, - * cbdataReferenceValid() will return false for those references. - * \deprecated use CBDATA_CLASS() instead + * cbdataReferenceDone() must still be called for any active + * references to the cbdata entry. The cbdata entry will be freed + * only when the last reference is removed. + * + * \note For internal CBDATA use only. */ void *cbdataInternalFree(void *p, const char *, int); -/// \deprecated use CBDATA_CLASS() instead -#define cbdataFree(var) do {if (var) {cbdataInternalFree(var,__FILE__,__LINE__); var = NULL;}} while(0) #if USE_CBDATA_DEBUG void cbdataInternalLockDbg(const void *p, const char *, int); -#define cbdataInternalLock(a) cbdataInternalLockDbg(a,__FILE__,__LINE__) +#define cbdataInternalLock(a) cbdataInternalLockDbg(a,__FILE__,__LINE__) void cbdataInternalUnlockDbg(const void *p, const char *, int); -#define cbdataInternalUnlock(a) cbdataInternalUnlockDbg(a,__FILE__,__LINE__) +#define cbdataInternalUnlock(a) cbdataInternalUnlockDbg(a,__FILE__,__LINE__) int cbdataInternalReferenceDoneValidDbg(void **p, void **tp, const char *, int); #define cbdataReferenceValidDone(var, ptr) cbdataInternalReferenceDoneValidDbg((void **)&(var), (ptr), __FILE__,__LINE__) @@ -223,8 +251,8 @@ void cbdataInternalUnlock(const void *p); callback(..., cbdata); \endcode * - \param var The reference variable. Will be automatically cleared to NULL. - \param ptr A temporary pointer to the referenced data (if valid). + * \param var The reference variable. Will be automatically cleared to NULL. + * \param ptr A temporary pointer to the referenced data (if valid). */ int cbdataInternalReferenceDoneValid(void **p, void **tp); #define cbdataReferenceValidDone(var, ptr) cbdataInternalReferenceDoneValid((void **)&(var), (ptr)) @@ -234,109 +262,91 @@ int cbdataInternalReferenceDoneValid(void **p, void **tp); /** * \param p A cbdata entry reference pointer. * - * \retval 0 A reference is stale. The pointer refers to a entry freed by cbdataFree(). + * \retval 0 A reference is stale. The pointer refers to a entry already freed. * \retval true The reference is valid and active. */ int cbdataReferenceValid(const void *p); -/// \ingroup CBDATAAPI +/** + * Create a run-time registration for the class type with cbdata memory allocator. + * + * \note For internal CBDATA use only. + */ cbdata_type cbdataInternalAddType(cbdata_type type, const char *label, int size, FREE * free_func); /** * This needs to be defined FIRST in the class definition. * It plays with private/public states in C++. */ -#define CBDATA_CLASS(type) \ +#define CBDATA_CLASS(type) \ public: \ void *operator new(size_t size) { \ assert(size == sizeof(type)); \ - if (!CBDATA_##type) \ - CBDATA_##type = cbdataInternalAddType(CBDATA_##type, #type, sizeof(type), NULL); \ + if (!CBDATA_##type) CBDATA_##type = cbdataInternalAddType(CBDATA_##type, #type, sizeof(type), NULL); \ return (type *)cbdataInternalAlloc(CBDATA_##type,__FILE__,__LINE__); \ } \ void operator delete (void *address) { \ - if (address) cbdataInternalFree(address,__FILE__,__LINE__);\ + if (address) cbdataInternalFree(address,__FILE__,__LINE__); \ } \ - void *toCbdata() { return this; } \ + void *toCbdata() { return this; } \ private: \ - static cbdata_type CBDATA_##type; + static cbdata_type CBDATA_##type; /** - \par - * Creates a new reference to a cbdata entry. Used when you need to - * store a reference in another structure. The reference can later - * be verified for validity by cbdataReferenceValid(). + * Creates a global instance pointer for the CBDATA memory allocator + * to allocate and free objects for the matching CBDATA_CLASS(). * - \param var - * The reference variable is a pointer to the entry, in all - * aspects identical to the original pointer. But semantically it - * is quite different. It is best if the reference is thought of - * and handled as a "void *". - */ -#define cbdataReference(var) (cbdataInternalLock(var), var) - -/** - \ingroup CBDATAAPI - * Removes a reference created by cbdataReference(). + * Place this in the appropriate .cc file for the class being registered. * - \param var The reference variable. Will be automatically cleared to NULL. + * May be placed inside an explicit namespace scope declaration, + * or CBDATA_NAMESPACED_CLASS_INIT() used instead. */ -#define cbdataReferenceDone(var) do {if (var) {cbdataInternalUnlock(var); var = NULL;}} while(0) - -/// \ingroup CBDATAAPI #define CBDATA_CLASS_INIT(type) cbdata_type type::CBDATA_##type = CBDATA_UNKNOWN -#define CBDATA_NAMESPACED_CLASS_INIT(namespace, type) cbdata_type namespace::type::CBDATA_##type = CBDATA_UNKNOWN /** - * Macro that defines a new cbdata datatype. Similar to a variable - * or struct definition. Scope is always local to the file/block - * where it is defined and all calls to cbdataAlloc() for this type - * must be within the same scope as the CBDATA_TYPE declaration. - * Allocated entries may be referenced or freed anywhere with no - * restrictions on scope. - * \deprecated Use CBDATA_CLASS() instead + * Creates a global instance pointer for the CBDATA memory allocator + * to allocate and free objects for the matching CBDATA_CLASS(). + * + * Place this in the appropriate .cc file for the class being registered. */ -#define CBDATA_TYPE(type) static cbdata_type CBDATA_##type = CBDATA_UNKNOWN +#define CBDATA_NAMESPACED_CLASS_INIT(namespace, type) cbdata_type namespace::type::CBDATA_##type = CBDATA_UNKNOWN /** - \ingroup CBDATAAPI + * Creates a new reference to a cbdata entry. Used when you need to + * store a reference in another structure. The reference can later + * be verified for validity by cbdataReferenceValid(). * - * Initializes the cbdatatype. Must be called prior to the first use of cbdataAlloc() for the type. + * \deprecated Prefer the use of CbcPointer<> smart pointer. * - \par - * Alternative to CBDATA_INIT_TYPE() - * - \param type Type being initialized - \param free_func The freehandler called when the last known reference to an allocated entry goes away. + * \param var + * The reference variable is a pointer to the entry, in all + * aspects identical to the original pointer. But semantically it + * is quite different. It is best if the reference is thought of + * and handled as a "void *". */ -#define CBDATA_INIT_TYPE_FREECB(type, free_func) do { if (!CBDATA_##type) CBDATA_##type = cbdataInternalAddType(CBDATA_##type, #type, sizeof(type), free_func); } while (false) +#define cbdataReference(var) (cbdataInternalLock(var), var) /** - * Initializes the cbdatatype. Must be called prior to the first use of cbdataAlloc() for the type. - * - \par - * Alternative to CBDATA_INIT_TYPE_FREECB() + * Removes a reference created by cbdataReference(). * - \param type Type being initialized + * \deprecated Prefer the use of CbcPointer<> smart pointer. * - * \deprecated Use CBDATA_CLASS() instead + * \param var The reference variable. Will be automatically cleared to NULL. */ -#define CBDATA_INIT_TYPE(type) CBDATA_INIT_TYPE_FREECB(type, NULL) +#define cbdataReferenceDone(var) do {if (var) {cbdataInternalUnlock(var); var = NULL;}} while(0) /** - \ingroup CBDATA - * - * A generic wrapper for passing objects through cbdata. + * A generic wrapper for passing object pointers through cbdata. * Use this when you need to pass callback data to a blocking - * operation, but you don't want to/cannot have that pointer be cbdata itself. + * operation, but you don't want to/cannot have that pointer be + * cbdata itself. */ class generic_cbdata { CBDATA_CLASS(generic_cbdata); public: - - generic_cbdata(void * aData) : data(aData) {} + generic_cbdata(void *aData) : data(aData) {} templatevoid unwrap(wrapped_type **output) { *output = static_cast(data); @@ -344,14 +354,7 @@ public: } private: - /** - * The wrapped data - only public to allow the mild abuse of this facility - * done by store_swapout - it gives a wrapped StoreEntry to StoreIO as the - * object to be given to the callbacks. That needs to be fully cleaned up! - * - RBC 20060820 - \todo CODE: make this a private field. - */ - void *data; /* the wrapped data */ + void *data; }; #endif /* SQUID_CBDATA_H */ diff --git a/src/store_client.cc b/src/store_client.cc index 6bff6689db..fd677f2887 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -308,13 +308,11 @@ storeClientCopy2(StoreEntry * e, store_client * sc) * this function * XXX: Locking does not prevent calling sc destructor (it only prevents * freeing sc memory) so sc may become invalid from C++ p.o.v. - * */ - cbdataInternalLock(sc); + CbcPointer tmpLock = sc; assert (!sc->flags.store_copying); sc->doCopy(e); - assert (!sc->flags.store_copying); - cbdataInternalUnlock(sc); + assert(!sc->flags.store_copying); } void