Cascaded VFS patch

[Simo Sorce]

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