On Tue, 2002-06-18 at 09:05, Andrew Bartlett wrote: > Simo Sorce wrote: > > + /* Handles on dlopen() call */ > > + smb_vfs_dl_handle *dl_handle; > > + void **vfs_private; > > Why a void** ?
Because you really do not know what the module wants to put there, can be anything. > > +typedef enum _vfs_op_layer { > > + SMB_VFS_LAYER_NOOP = -1, /* - For using in VFS module to indicate >end of array */ > > + /* of operations description */ > > + SMB_VFS_LAYER_OPAQUE = 0, /* - Final level, does not call anything >beyond itself */ > > + SMB_VFS_LAYER_TRANSPARENT, /* - Normal operation, calls underlying >layer after */ > > + /* possibly changing passed data */ > > + SMB_VFS_LAYER_LOGGER, /* - Logs data, calls underlying layer, >logging does not */ > > + /* use Samba VFS */ > > + SMB_VFS_LAYER_SPLITTER, /* - Splits operation, calls underlying >layer _and_ own facility, */ > > + /* then combines result */ > > + SMB_VFS_LAYER_SCANNER /* - Checks data and possibly initiates >additional */ > > + /* file activity like logging to files >_inside_ samba VFS */ > > +} vfs_op_layer; > > I'm still not conviced on this stuff. I am and this is required for a relly functional cascading mode. Trivial schemes will not address the needs of modules makers. > > SAFE_FREE() > ok, ok > > {"vfs object", P_STRING, P_LOCAL, &sDefault.szVfsObjectFile, >handle_vfs_object, NULL, FLAG_SHARE}, > > {"vfs options", P_STRING, P_LOCAL, &sDefault.szVfsOptions, NULL, NULL, >FLAG_SHARE}, > > + {"vfs path", P_STRING, P_LOCAL, &sDefault.szVfsPath, NULL, NULL, >FLAG_SHARE}, > > Why do we need a new smb.conf option? Why can't we just have full paths > on objects? simple, generally parameters are 1024 bytes limited, leaving out the path, make it both more readable and less space consuming and make u easy to move modules to another path, or make it parametrical. > > If this option does what I think it does, then it really should be > global. no because you can have a different path for any different share, each with it's own set of modules. > > + for(dl_handle = conn->dl_handle; dl_handle->handle; dl_handle++) { > > + /* Close dlopen() handle */ > > + done_fptr = (void (*)(connection_struct >*))sys_dlsym(dl_handle->handle, dl_handle->done_method); > > + > > + if (done_fptr == NULL) { > > + DEBUG(3, ("No %s() symbol found in module with >handle %p, ignoring\n", dl_handle->done_method, dl_handle->handle)); > > + } else { > > + done_fptr(conn); > > + } > > + sys_dlclose(dl_handle->handle); > > + string_free(&dl_handle->done_method); > > Why a string_free()? nice catch, need to look more closely to this point. > > -static BOOL vfs_init_default(connection_struct *conn) > > +static void vfs_init_default(connection_struct *conn) > > { > > DEBUG(3, ("Initialising default vfs hooks\n")); > > > > memcpy(&conn->vfs_ops, &default_vfs_ops, sizeof(struct vfs_ops)); > > - return True; > > + conn->dl_handle = NULL; > > } > > Why? I think we should return errors on failure... if the dl_handle is null we have an error. > > /**************************************************************************** > > initialise custom vfs hooks > > ****************************************************************************/ > > > > -static BOOL vfs_init_custom(connection_struct *conn) > > +static BOOL vfs_init_custom(connection_struct *conn, int vfs_number, const char >*vfs_object) > > { > > int vfs_version = -1; > > - struct vfs_ops *ops, *(*init_fptr)(int *, struct vfs_ops *); > > + vfs_op_tuple *ops, *(*init_fptr)(int *, const struct vfs_ops *, int); > > + fstring method_init_name; > > + fstring vfs_object_name; > > fstrings are BAD. We might use a lot of them, but *all* new code needs > to use > asprintf() etc. it really depends on where they are, but I agree in this case, we have to change these bits, before a commit. > I'm really not conviced that we want to maintain 'backward > compatibility'. It sets a precedent, and I'm not comfortable with it. I think it is important. we already are backward compatible for lot of things, why not modules that will likely be built outside the samba team and need a bit more stable interface OR backwards compatibility? > Watch that indenting - 8 space tabs please. oh andrew ... > > + if (string_set(&vfsobj, lp_vfsobj(SNUM(conn)))) { > > + /* Parse passed modules specification to array of modules >*/ > > + set_first_token(vfsobj); > > + /* We are using default separators: ' \t\r\n' */ > > + vfs_objects = toktocliplist(&nobj, NULL); > > What are you trying here? Why can't this be a normal lp_list? yes can be, but it was not so spread when the patch has been made, and the right name now is str_list_ and is located in util_str.c > > As you can see, I'm not happy with a few things in this patch. I see :) > > idra: Can you hold off on your commit? While I don't mind (to much) > the vfsclient issue, these things need addressing. Yes they are simple, > and yes, nobody will fix them later... yes I will hold, until this problems get addressed. Simo. -- Simo Sorce ---------- Una scelta di liberta': Software Libero. A choice of freedom: Free Software. http://www.softwarelibero.it