Re: Implement directory object in wineserver.

[Robert Shearman]

Vitaliy Margolen wrote:
> ChangeLog:
> Implement directory object in wineserver.

I like the design, but I have a few comments on the patch.
> server/Makefile.in  |    1 server/directory.c  |  384 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> server/main.c       |    1 server/object.c     |   64 +++++++--
> server/object.h     |   36 +++++
> server/protocol.def |   21 +++
> server/request.c    |    1 server/request.h    |   11 +
> server/unicode.c    |   14 ++
> server/unicode.h    |    2 10 files changed, 521 insertions(+), 14 
> deletions(-)
> create mode 100644 server/directory.c
>
> 760fd4b8fd36abf1b281bfa2f963bf499bc35be5
> diff --git a/server/Makefile.in b/server/Makefile.in
> index 47687c0..50a5049 100644
> --- a/server/Makefile.in
> +++ b/server/Makefile.in
> @@ -17,6 +17,7 @@ C_SRCS = \
>    context_sparc.c \
>    context_x86_64.c \
>    debugger.c \
> - directory.c \
> event.c \
> fd.c \
> file.c \
>
> diff --git a/server/directory.c b/server/directory.c
> new file mode 100644
> index 0000000..d8f168d
> --- /dev/null
> +++ b/server/directory.c
> @@ -0,0 +1,384 @@
> +/*
> -
> - Server-side directory object management
> -
> -
> -
> - Copyright (C) 2005 Vitaliy Margolen
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> -
> -
> - */
> -
>
> +#include "config.h"
> +#include "wine/port.h"
> -
>
> +#include <assert.h>
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> -
>
> +#include "windef.h"
> +#include "winternl.h"
> -
>
> +#include "handle.h"
> +#include "request.h"
> +#include "process.h"
> +#include "unicode.h"
> +#include "wine/list.h"
> +#include "object.h"
> -
>
> +#define HASH_SIZE 37
> -
>
> +#define DIRECTORY_QUERY                 (0x0001)
> +#define DIRECTORY_TRAVERSE              (0x0002)
> +#define DIRECTORY_CREATE_OBJECT         (0x0004)
> +#define DIRECTORY_CREATE_SUBDIRECTORY   (0x0008)
> +#define DIRECTORY_ALL_ACCESS (STANDARD_RIGHTS_REQUIRED | 0xF)

Should these go into winternl.h?
> -
>
> +/* Global name space */
> +struct directory *root_directory;
> -
>
> +struct directory
> +{
> - struct object obj;              /* object header */
> - struct list entries[HASH_SIZE]; /* array of hash entry lists */
>
> +};
> -
>
> +static void directory_dump( struct object *obj, int verbose );
> +static struct object *directory_lookup_name( struct object *obj, struct unicode_str *name,
> -
>                                         unsigned int attr );
> -
>
> +static const struct object_ops directory_ops =
> +{
> - sizeof(struct directory),     /* size */
> - directory_dump,               /* dump */
> - no_add_queue,                 /* add_queue */
> - NULL,                         /* remove_queue */
> - NULL,                         /* signaled */
> - NULL,                         /* satisfied */
> - no_signal,                    /* signal */
> - no_get_fd,                    /* get_fd */
> - directory_lookup_name,        /* lookup_name */
> - no_close_handle,              /* close_handle */
> - no_destroy                    /* destroy */
>
> +};
> -
>
> +static void directory_dump( struct object *obj, int verbose )
> +{
> - struct directory *dir = (struct directory *)obj;
> - assert( obj->ops == &directory_ops );
> -
> - fputs( "Directory ", stderr );
> - dump_object_name( obj );
> - if (verbose)
> - {
> -
>    unsigned int i;
> -
> -
>    fputs( " entries:\n", stderr );
> -
>    for (i = 0; i < HASH_SIZE; i++)
> -
>    {
> -
>        struct list *p = &dir->entries[i];
> -
>        struct object_name *ptr;
> -
> -
>        LIST_FOR_EACH_ENTRY(ptr, p, struct object_name, entry)
> -
>        {
> -
>            fputs( " ", stderr );
> -
>            dump_object_name( ptr->obj );
> -
>            fputc( '\n', stderr );
> -
>        }
> -
>    }
> - }
> - else
> -
>    fputc( '\n', stderr );
>
> +}
> -
>
> +/* find an object by its name in a given directory; the refcount is incremented
> -
> - name - is a relative name */
>
> +static struct object *find_object_in_dir( struct directory *dir, const WCHAR *name,
> -
>                                      size_t len, unsigned int attr )
>
> +{
> - const struct list *list, *p;
> -
> - if (!name || !len) return NULL;
> -
> - list = &dir->entries[ get_name_hash( name, len ) % HASH_SIZE ];
> - LIST_FOR_EACH( p, list )
> - {
> -
>    const struct object_name *ptr = LIST_ENTRY( p, const struct object_name, entry );

You can replace LIST_FOR_EACH and LIST_ENTRY with LIST_FOR_EACH_ENTRY.
> -
>    if (!ptr || ptr->len != len) continue;
> -
>    if (attr & OBJ_CASE_INSENSITIVE)
> -
>    {
> -
>        if (!strncmpiW( ptr->name, name, len/sizeof(WCHAR) )) return grab_object( ptr->obj );
> -
>    }
> -
>    else
> -
>    {
> -
>        if (!memcmp( ptr->name, name, len )) return grab_object( ptr->obj );
> -
>    }
> - }
> - return NULL;
>
> +}
> -
>
> +static struct object *directory_lookup_name( struct object *obj, struct unicode_str *name,
> -
>                                         unsigned int attr )
>
> +{
> - struct object *found;
> - WCHAR *p;
> - size_t sz;
> - assert( obj->ops == &directory_ops );
> -
> - if (!(p = memchrW( name->str, '\', name->len / sizeof(WCHAR) )))
> -
>    /* Last element in the path name */
> -
>    sz = name->len;
> - else
> -
>    sz = (p - name->str) * sizeof(WCHAR);
> -
> - if ((found = find_object_in_dir( (struct directory *)obj, name->str, sz, attr )))
> - {
> -
>    /* Skip trailing \\ */
> -
>    if (p)
> -
>    {
> -
>        p++;
> -
>        sz += sizeof(WCHAR);
> -
>    }
> -
>    /* Move to the next element*/
> -
>    name->str = p;
> -
>    name->len -= sz;
> -
>    return found;
> - }
> -
> - if (name->str)
> - {
> -
>    if (sz == 0) /* Double backslash */
> -
>        set_error( STATUS_OBJECT_NAME_INVALID );
> -
>    else if (p)  /* Path still has backslashes */
> -
>        set_error( STATUS_OBJECT_PATH_NOT_FOUND );
> - }
> - return NULL;
>
> +}
> -
>
> +/******************************************************************************/
> -
>
> +/* Create root directory - it's a special case when no namespace exists yet.*/
> +struct directory *create_root( void )
> +{
> - static const WCHAR rootW[]={'\'};
> - static const struct unicode_str root = {rootW, sizeof(rootW)};
> - struct directory *dir;
> - struct object_name *name_ptr;
> -
> - if (!(name_ptr = alloc_name( &root ))) return NULL;
> - if ((dir = alloc_object( &directory_ops )))
> - {
> -
>    unsigned int i;
> -
>    /* root has no parents */
> -
>    list_init( &name_ptr->entry );
> -
>    name_ptr->obj = &dir->obj;
> -
>    dir->obj.name = name_ptr;
> -
>    for (i = 0; i < HASH_SIZE; i++)
> -
>        list_init( &dir->entries[i] );
> - }
> - else free( name_ptr );
> - return dir;
>
> +}
> -
>
> +struct directory *create_directory( const struct object_attr *attr )
> +{
> - struct directory *dir;
> -
> - if ((dir = create_named_object_dir( attr, &directory_ops )))
> - {
> -
>    if (get_error() != STATUS_OBJECT_NAME_EXISTS)
> -
>    {
> -
>        unsigned int i;
> -
>        for (i = 0; i < HASH_SIZE; i++)
> -
>            list_init( &dir->entries[i] );
> -
>    }
> - }
> - return dir;
>
> +}
> -
>
> +static void insert_into_dir( struct directory* dir, struct object *obj )
> +{
> - int hash = get_name_hash( obj->name->name, obj->name->len) % HASH_SIZE;

Missing space after obj->name->len.
> -
> - grab_object( dir );
> - obj->name->parent = &dir->obj;
> - list_add_head( &dir->entries[hash], &obj->name->entry );
>
> +}
> -
>
> +/******************************************************************************
> -
> - Find an object by it's name in a given root object
> -
> -
> -
> - PARAMS
> -
> - attr      [I] rootdir, name and attributes
> -
> - name_left [O] [optional] leftover name if object is not found
> -
> -
> -
> - RETURNS
> -
> - NULL:      If params are invalid
> -
> - Found:     If object with exact name is found returns that object. (name_left->len == 0)
> -
> -
>         Object's refcount is incremented
> -
> - Not found: The last matched parent. (name_left->len > 0)
> -
> -
>         Parent's refcount is incremented.
> - */
>
> +struct object *find_object_dir( const struct object_attr *attr, struct unicode_str *name_left )
> +{
> - struct object *obj, *parent;
> - struct unicode_str name_l = *attr->name;

What does name_l stand for?
> -
> - /* Arguments check:
> -
> * - Either name or root have to be specified
> -
> * - If root is specified path shouldn't start with backslash */
> - if (!attr ||
> -
>    (!attr->rootdir && (!name_l.str || !name_l.len)) ||
> -
>    (name_l.len && ((*name_l.str == '\\' && attr->rootdir) ||
> -
>                    (*name_l.str != '\\' && !attr->rootdir))))
> - {
> -
>    set_error( STATUS_OBJECT_PATH_SYNTAX_BAD );
> -
>    return NULL;
> - }
> -
> - if (attr->rootdir)
> -
>    parent = get_handle_obj( current->process, attr->rootdir, 0, &directory_ops );
> - else
> -
>    parent = grab_object( root_directory );
> - if (!parent) return NULL;
> -
> - /* Skip leading backslash */
> - if (name_l.len && *name_l.str == '\')
> - {
> -
>    name_l.str++;
> -
>    name_l.len -= sizeof(WCHAR);
> - }
> -
> - /* Special case for opening RootDirectory */
> - if (!name_l.len) goto done;
> -
> - while ((obj = parent->ops->lookup_name( parent, &name_l, attr->attributes )))
> - {
> -
>    /* move to the next element */
> -
>    release_object ( parent );
> -
>    parent = obj;
> - }
> - if (get_error())
> - {
> -
>    release_object ( parent );
> -
>    return NULL;
> - }
> -
> - done:
> - if (name_left) dup_unicode_str(&name_l, name_left);
> - return parent;
>
> +}
> -
> @@ -311,3 +309,41 @@ int no_close_handle( struct object *obj,
> void no_destroy( struct object *obj )
> {
> }
> -
>
> +/* Name space initialization */
> -
>
> +struct object *permanent_obj[25];
> +int permanent_obj_cnt = 0;

This looks a bit ugly to me. Why not just keep track of the individual 
objects that need to be kept around in named variables?
> -
>
> +static inline void create_default_dir( const WCHAR *n, size_t nl)
> +{
> - struct unicode_str name;
> - struct object_attr attr = {NULL, &name, 0};
> - struct directory *dir;
> - name.str = n;
> - name.len = nl;
> - if ((dir = create_directory( &attr ) ))
> -
>    permanent_obj[permanent_obj_cnt++] = (struct object *)dir;
>
> +}
> -
>
> +static void init_namespace_dirs(void)
> +{
> - /* Directories */
> - static const WCHAR dir1W[]={'\','?','?'};
> - static const WCHAR dir2W[]={'\','B','a','s','e','N','a','m','e','d','O','b','j','e','c','t','s'};
> -
> - root_directory = create_root();
> - permanent_obj[permanent_obj_cnt++] = (struct object *)root_directory;
> - create_default_dir( dir1W, sizeof(dir1W) );
> - create_default_dir( dir2W, sizeof(dir2W) );
>
> +}
> -
>
> +void init_namespace( void )
> +{
> - init_namespace_dirs();
>
> +}
> -
>
> +void close_namespace( void )
> +{
> - while(permanent_obj_cnt) release_object( permanent_obj[--permanent_obj_cnt] );
>
> +}
> diff --git a/server/unicode.c b/server/unicode.c
> index 16aea3d..c275f86 100644
> --- a/server/unicode.c
> +++ b/server/unicode.c
> @@ -68,3 +68,17 @@ int dump_strW( const WCHAR *str, size_t 
>     count += pos - buffer;
>     return count;
> }
> -
>
> +void inline free_unicode_str( struct unicode_str *str )
> +{
> - if (str->str) free( (WCHAR*)str->str );
> - str->str = NULL;
> - str->len = 0;
>
> +}
> -
>
> +void dup_unicode_str( const struct unicode_str *src, struct unicode_str *dst )
> +{
> - if (!src || !dst) return;

I think it would probably be better to let the function crash on both of 
these conditions. If the client doesn't check if src is null then dst 
when be left unintialized and cause a crash later.
> - dst->str = memdup( src->str, src->len );
> - dst->len = src->len;
>
> +}
> diff --git a/server/unicode.h b/server/unicode.h
> index f745705..ebd1343 100644
> --- a/server/unicode.h
> +++ b/server/unicode.h
> @@ -34,5 +34,7 @@ static inline WCHAR *strdupW( const WCHA
> }
>
> extern int dump_strW( const WCHAR *str, size_t len, FILE *f, const char escape[2] );
> +extern void free_unicode_str( struct unicode_str *str );
> +extern void dup_unicode_str( const struct unicode_str *src, struct unicode_str *dst );

You could make these both be inline functions.

Implement directory object in wineserver.

[Vitaliy Margolen]

Tuesday, November 22, 2005, 1:51:05 PM, Robert Shearman wrote:
> > ChangeLog:
> > Implement directory object in wineserver.
>
> I like the design, but I have a few comments on the patch.

Thank you. That's like 6st revision of it.
> > +#define DIRECTORY_QUERY                 (0x0001)
> > +#define DIRECTORY_TRAVERSE              (0x0002)
> > +#define DIRECTORY_CREATE_OBJECT         (0x0004)
> > +#define DIRECTORY_CREATE_SUBDIRECTORY   (0x0008)
> > +#define DIRECTORY_ALL_ACCESS (STANDARD_RIGHTS_REQUIRED | 0xF)
>
> Should these go into winternl.h?

Well they are defined only in ddk/winddk.h in mingw (I don't have SDK or
DDK). But probably you are right, they should go into winternl.h
> > - LIST_FOR_EACH( p, list )
> > - {
> > -
> >    const struct object_name *ptr = LIST_ENTRY( p, const struct object_name, entry );
>
> You can replace LIST_FOR_EACH and LIST_ENTRY with LIST_FOR_EACH_ENTRY.

That was direct cut & paste from object.c I decided to keep it the same
as well.
> > - struct object *obj, *parent;
> > - struct unicode_str name_l = *attr->name;
>
> What does name_l stand for?

L for local. I don't like to give local variables to long of the names.
> > +/* Name space initialization */
> > -
> >
> > +struct object *permanent_obj[25];
> > +int permanent_obj_cnt = 0;
>
> This looks a bit ugly to me. Why not just keep track of the individual
> objects that need to be kept around in named variables?

Because there will be more. Potentially we might have device + symlink
for stuff like named pipes, mailslots, serial & paralel ports, etc. I
didn't want to create one extra variable for each permanent object. As
this is a compromise to what windows does. I tried to keep it as flexible
as it can be.
> > +void dup_unicode_str( const struct unicode_str *src, struct unicode_str *dst )
> > +{
> > - if (!src || !dst) return;
>
> I think it would probably be better to let the function crash on both of
> these conditions. If the client doesn't check if src is null then dst 
> when be left unintialized and cause a crash later.

Mm no. It is really not good to crash server because of invalid
parameters. The thing is, that I'm using NULL pointers when calling this
function, so I've added extra checks here to not crash. This is internal
to server function, so I don't really see a problem hawing an extra
check.
> > extern int dump_strW( const WCHAR *str, size_t len, FILE *f, const char escape[2] );
> > +extern void free_unicode_str( struct unicode_str *str );
> > +extern void dup_unicode_str( const struct unicode_str *src, struct unicode_str *dst );
>
> You could make these both be inline functions.

Ok will do.

Implement directory object in wineserver.

[Robert Shearman]

Vitaliy Margolen wrote:
> > > - struct object *obj, *parent;
> > > - struct unicode_str name_l = *attr->name;
> >
> > What does name_l stand for?
>
> L for local. I don't like to give local variables to long of the names.

I'd drop the "_l" suffix. It confused me when reading your patch into 
thinking it was some kind of special name, whereas it was just the name.
> > > +/* Name space initialization */
> > > -
> > >
> > > +struct object *permanent_obj[25];
> > > +int permanent_obj_cnt = 0;
> >
> > This looks a bit ugly to me. Why not just keep track of the individual
> > objects that need to be kept around in named variables?
>
> Because there will be more. Potentially we might have device + symlink
> for stuff like named pipes, mailslots, serial & paralel ports, etc.

Yes, and these should be created at startup.
> I didn't want to create one extra variable for each permanent object.

There should be less than ten objects altogether, so why not? It doesn't 
have a level of complexity that the "permanent object" concept does.
> As
> this is a compromise to what windows does. I tried to keep it as flexible
> as it can be.

Granted, but overdesign is as bad as not being flexible enough. Is there 
a need for this array of permanent objects outside of this patch?
> > > +void dup_unicode_str( const struct unicode_str *src, struct unicode_str *dst )
> > > +{
> > > - if (!src || !dst) return;
> >
> > I think it would probably be better to let the function crash on both of
> > these conditions. If the client doesn't check if src is null then dst 
> > when be left unintialized and cause a crash later.
>
> Mm no. It is really not good to crash server because of invalid
> parameters. The thing is, that I'm using NULL pointers when calling this
> function, so I've added extra checks here to not crash. This is internal
> to server function, so I don't really see a problem hawing an extra
> check.

I'm not talking about crashing because of bad input from an application, 
but bad input from a developer. The only use of this function is already 
guarded against using NULL pointers. This is an internal to server 
function, so I don't really see a problem in crashing if a future 
developer isn't careful about what he/she passes to it. Even a good 
developer might not consider that dup_unicode_str would leave dst 
uninitialized when a NULL src is passed in. If you make them do the 
check themselves, then they likely will think about it.

Implement directory object in wineserver.

[Vitaliy Margolen]

Tuesday, November 22, 2005, 3:14:15 PM, Robert Shearman wrote:
> > > > +struct object *permanent_obj[25];
> > > > +int permanent_obj_cnt = 0;
> > >
> > > This looks a bit ugly to me. Why not just keep track of the individual
> > > objects that need to be kept around in named variables?
> >
> > Because there will be more. Potentially we might have device + symlink
> > for stuff like named pipes, mailslots, serial & paralel ports, etc.
>
> Yes, and these should be created at startup.

I'm not sure yet. Those I need yes, they will be created at startup. I
don't know if we can create all the "permanent" objects in one place.
> > I didn't want to create one extra variable for each permanent object.
>
> There should be less than ten objects altogether, so why not? It doesn't
> have a level of complexity that the "permanent object" concept does.

And don't want to go there again pls. I had a long discussion with
Alexandre about permanent objects. The windows' concept was not
acceptable for server.
> > As this is a compromise to what windows does. I tried to keep it as
> > flexible as it can be.
>
> Granted, but overdesign is as bad as not being flexible enough. Is there
> a need for this array of permanent objects outside of this patch?

What overdesign? These all combined is less then 20% of what native OM
can do. I don't like to fix something for one day, knowing well and good
some one will be back later on to fix it again.

Having a global variable is bad. Having 10x the required amount is
really bad. Why create them for no good reason?

Implement directory object in wineserver.

[Saulius Krasuckas]

* On Tue, 22 Nov 2005, Vitaliy Margolen wrote:
> - Tuesday, November 22, 2005, 1:51:05 PM, Robert Shearman wrote:
> > > +#define DIRECTORY_QUERY                 (0x0001)
> > > +#define DIRECTORY_TRAVERSE              (0x0002)
> > > +#define DIRECTORY_CREATE_OBJECT         (0x0004)
> > > +#define DIRECTORY_CREATE_SUBDIRECTORY   (0x0008)
> > > +#define DIRECTORY_ALL_ACCESS (STANDARD_RIGHTS_REQUIRED | 0xF)
> >
> > Should these go into winternl.h?
>
> Well they are defined only in ddk/winddk.h in mingw (I don't have SDK or 
> DDK). But probably you are right, they should go into winternl.h

* Quote from MSDN [*]:
| 
| Headers
| Declared in wdm.h and ntddk.h. Include wdm.h or ntddk.h.

I wonder if AJ would accept MSDN approach, which is different from the two 
ones mentioned above.

[*] http://msdn.microsoft.com/library/shared/deeptree/asp/rightframe.asp?dtcfg=/...

Implement directory object in wineserver.

[Ivan Leo Puoti]

Vitaliy Margolen wrote:
> > > +#define DIRECTORY_QUERY                 (0x0001)
> > > +#define DIRECTORY_TRAVERSE              (0x0002)
> > > +#define DIRECTORY_CREATE_OBJECT         (0x0004)
> > > +#define DIRECTORY_CREATE_SUBDIRECTORY   (0x0008)
> > > +#define DIRECTORY_ALL_ACCESS (STANDARD_RIGHTS_REQUIRED | 0xF)
> >
> > Should these go into winternl.h?
>
> Well they are defined only in ddk/winddk.h in mingw (I don't have SDK or
> DDK). But probably you are right, they should go into winternl.h

No, they should go in include/ddk/wdm.h

Ivan.