G'day Thanks for the mention Brian. Yeah, I totally agree that the if/else approach not being a swarve solution since this creates extra execution paths. Though, mind you this was my first attempt on seeing the link in action.
As for abstracting the storage behind a common interface, I'm thinking that I should really forget about external engines for now and concentrate on getting the current storage behind a interface, that is to be used in a common fashion later on. Making this loadable is probably not so difficult once we have this working. It is definitely on my todo list :-) Trond: Thanks for the feedback! I'm thinking using a char string for setting the storage parameters might be neat, such that: int (*sb_settings)(void*, const char*); where const char* can be something along the line of: "database_name:option1=foo:option2=foo:option3=foo" (somewhat like the DBI). Not sure if this is a good solution but it looks promising in my head. Any comments? Toru On Feb 5, 2008 2:51 AM, Trond Norbye <[EMAIL PROTECTED]> wrote: > Brian Aker wrote: > > Hi! > > I've been studying the patch from Toru Maesaka on making memcached have a > plugable storage backend: > http://alpha.mixi.co.jp/dist/memcached-1.2.4_modularexp-0.0.5.tar.gz > > What I am wondering is how to get this into the main distribution. I see > some problems in the way that he went about what he did, but mostly it looks > good. What he has done is refactored the main storage functions such that > the function calls come from this structure: > > typedef struct { > void *opaque_engine; /* pointer to the database handle */ > void *dlink_handler; /* dynamic library handler */ > char *dboject_name; /* name of the db object to link to */ > int max_dbsize; /* maximum database size */ > int curr_size; /* current size of the database */ > int bucket_num; /* optional value for buckets */ > > /* functions to be dynamically loaded */ > void *(*ext_new)(void); > int (*ext_open)(void*, const char*); > void (*ext_close)(void*); > int (*ext_conf)(void*, const int, const char*); > void *(*ext_get)(void*, const void*, const int, int*); > int (*ext_put)(void*, const void*, const int, const void*, const int); > int (*ext_del)(void*, const void*, const int); > int (*ext_flush)(void*, const char*); > } MMCSTORAGE; > > He has left out increment/decrement, but I believe those would need to be > handled as well (if you want to push atomic operations into the engine that > is). > > Thoughts? Toru's method involves if/else around calls, personally I would > prefer putting all logic behind the API. > > > I like this idea :-) I agree that we should not clutter the code with > if/else around the calls to the storage, so we should implement a "default > memory storage" that we use if no other is requested. > > Just by looking at the struct, I feel that the first part of the struct is > specialized towards his needs and should be removed. I would probably go for > something like: > > typedef struct { > int sb_version; /* to allow modifications to the structure */ > > /* > ** The following member may be removed because the backend may store > it's internal data in it's own > ** static struct. If we choose to remove it, we should aslo remove the > first parameter in the functions > ** below > */ > void *sb_data; /* Pointer to a block where the storage backend may > store internal data */ > > /* The functions we need.. The first parameter should be sb_data */ > void *(*sb_get)(void*, const void*, const int, int*); > int (*sb_put)(void*, const void*, const int, const void*, const int); > int (*sb_del)(void*, const void*, const int); > int (*sb_flush)(void*, const char*); > ... > } MMCSTORAGE; > > Each "module" should contain the following functions: > > MMCSTORAGE* create_instance(settings*, int version, int *error); > settings - The backend should be able to pick up the configuration > parameters it needsversion - The "protocol" version requested from the > backend > error - A place the backend can store a predefined error code > > void destroy_instance(MMCSTORAGE*); > > memcached.c should include something like (not complete/compiled code): > > MMCSTORAGE* initialize_storage(const char *library, const char *config) > { > MMCSTORAGE* ret; > void *handle = dlopen(library, RTLD_LAZY); > if (handle == NULL) { > /* ERROR, report it to the user */ > return NULL; > } > > MMCSTORAGE* (*create_instance)(void*, int, int*) = dlsym(handle, > "create_instance"); > if (create_instance == NULL) { > /* ERROR, report it to the user */ > return NULL; > } > > int error; > /* request a instance with protocol version 1 */ > ret = (*create_instance)(&settings, 1, &error); > if (ret == NULL) { > /* report this error message to the user */ > } > > return ret; > } > > If no library is requested, we should use a built-in version. Well, that's > my ideas.. Comments anyone? > > Trond > >
