Cascaded VFS patch
Simo Sorce
idra at samba.org
Tue Jun 18 01:25:28 GMT 2002
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
More information about the samba-technical
mailing list