Re: Implement directory object in wineserver.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> +/* get parts of an OBJECT_ATTRIBUTES into object_attr */
> +#define GET_OBJECT_ATTR(a,r,n) \
> - get_req_unicode_str( n ); \
> - (a)->rootdir    = (r)->rootdir; \
> - (a)->attributes = (r)->attributes; \
> - (a)->name       = (n);
> -

That's ugly, please pass parameters explicitly to the functions, don't
hide them inside an object_attr structure. Also directories inside the
server have to be specified as directory objects, not as handles.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 4:53:23 AM, Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > +/* get parts of an OBJECT_ATTRIBUTES into object_attr */
> > +#define GET_OBJECT_ATTR(a,r,n) \
> > - get_req_unicode_str( n ); \
> > - (a)->rootdir    = (r)->rootdir; \
> > - (a)->attributes = (r)->attributes; \
> > - (a)->name       = (n);
> > -
> That's ugly, please pass parameters explicitly to the functions, don't
> hide them inside an object_attr structure. Also directories inside the
> server have to be specified as directory objects, not as handles.

So instead of:

void *create_named_object_dir( const struct object_attr *attr,
                               const struct object_ops *ops );

DECL_HANDLER([open/close]_object)
{
...
    GET_OBJECT_ATTR(&attr, req, &name)
    if ((dir = create_directory( &attr )))
...

you want this:

void *create_named_object_dir( const struct object *rootdir,
                               const struct unicode_str *name,
                               unsigned int attr,
                               const struct object_ops *ops );

DECL_HANDLER([open/create]_directory)
{
...
    get_req_unicode_str( &name );
    if (req->rootdir) root_obj = get_handle_obj( current->process, req->rootdir, 0, &directory_ops );
    dir = create_directory( root_obj, &name, req->attributes );
    if (root_obj) release_object( root_obj );
...

For each named object?

Vitaliy

Implement directory object in wineserver.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> void *create_named_object_dir( const struct object *rootdir,
>                                const struct unicode_str *name,
>                                unsigned int attr,
>                                const struct object_ops *ops );
>
> DECL_HANDLER([open/create]_directory)
> {
> ...
>     get_req_unicode_str( &name );
>     if (req->rootdir) root_obj = get_handle_obj( current->process, req->rootdir, 0, &directory_ops );
>     dir = create_directory( root_obj, &name, req->attributes );
>     if (root_obj) release_object( root_obj );
> ...
>
> For each named object?

Something like that yes (plus proper error handling of course). Also
the get_handle_obj needs to be encapsulated in a get_directory_obj so
that you don't export directory_ops, and create_named_object_dir
should take a struct directory, not a struct object.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 9:14:28 AM, Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > void *create_named_object_dir( const struct object *rootdir,
> >                                const struct unicode_str *name,
> >                                unsigned int attr,
> >                                const struct object_ops *ops );
> >
> > DECL_HANDLER([open/create]_directory)
> > {
> > ...
> >     get_req_unicode_str( &name );
> >     if (req->rootdir) root_obj = get_handle_obj( current->process,
> > req->rootdir, 0, &directory_ops );
> >     dir = create_directory( root_obj, &name, req->attributes );
> >     if (root_obj) release_object( root_obj );
> > ...
> >
> > For each named object?
>
> Something like that yes (plus proper error handling of course). Also
> the get_handle_obj needs to be encapsulated in a get_directory_obj so

Just want to make clear for myself. You want me to add all this code to
each object's create/open instead of putting it in the one place?
> that you don't export directory_ops, and create_named_object_dir
> should take a struct directory, not a struct object.

Mm I don't want to do that because find_object_dir could and will be used
on any object. I'm planning on renaming our current find_object into
something like find_object_ns only to be used by objects with their own
namespace. And they will use it only in lookup_name.

To drop it from everywhere else I will need query_type_name added to
object_ops. So I can use create_named_object_dir on any named object.

Implement directory object in wineserver.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> Just want to make clear for myself. You want me to add all this code to
> each object's create/open instead of putting it in the one place?

Yes. If there's common code you can of course move it to a shared
function. But please not an ugly macro that accesses the request
structure behind the scenes.
> Mm I don't want to do that because find_object_dir could and will be used
> on any object. I'm planning on renaming our current find_object into
> something like find_object_ns only to be used by objects with their own
> namespace. And they will use it only in lookup_name.

I'm not sure there's much point in making all objects directories. It
would probably be better to have a base directory object with its own
ops function table.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 9:59:42 AM, Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > Mm I don't want to do that because find_object_dir could and will be used
> > on any object. I'm planning on renaming our current find_object into
> > something like find_object_ns only to be used by objects with their own
> > namespace. And they will use it only in lookup_name.
>
> I'm not sure there's much point in making all objects directories. It
> would probably be better to have a base directory object with its own
> ops function table.

No, I don't want to make all objects a directory. But I do want to put
all named objects into directories <g>

Basically event, mutex, section, timer, etc can go there with minor
changes to code. Named pipes, mail slots and winstations with desktops
are a bit different. They have their own name space, that I'm planning on
using current namespace mechanism for. With the sample code I've sent to
you some days ago to show how that would work with named pipes.
find_object_dir worked perfectly on named pipes. The problem is object
creation. Actually not a creation itself, but insertion into the local
name space. I need to call different functions to insert an object into
directory or local name space (struct namespace). To do that I will need
to know what object type are we creating and what object type is the
parent.

About RootDirectory part of the OBJECT_ATTRIBUTES: I need to make some
more tests, but so far I see that it have to be a handle (on ntdll side)
to a directory object. Same is also stated on MSDN. Otherwise it fails.

Implement directory object in wineserver.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> Basically event, mutex, section, timer, etc can go there with minor
> changes to code. Named pipes, mail slots and winstations with desktops
> are a bit different. They have their own name space, that I'm planning on
> using current namespace mechanism for. With the sample code I've sent to
> you some days ago to show how that would work with named pipes.
> find_object_dir worked perfectly on named pipes. The problem is object
> creation. Actually not a creation itself, but insertion into the local
> name space. I need to call different functions to insert an object into
> directory or local name space (struct namespace). To do that I will need
> to know what object type are we creating and what object type is the
> parent.

Sorry, but that's wrong. You should *never* check the object type
explicitly. You need a generic function to insert an object into a
directory, and a generic directory type that can implement the various
types of directories.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 10:34:05 AM, Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > Basically event, mutex, section, timer, etc can go there with minor
> > changes to code. Named pipes, mail slots and winstations with desktops
> > are a bit different. They have their own name space, that I'm planning on
> > using current namespace mechanism for. With the sample code I've sent to
> > you some days ago to show how that would work with named pipes.
> > find_object_dir worked perfectly on named pipes. The problem is object
> > creation. Actually not a creation itself, but insertion into the local
> > name space. I need to call different functions to insert an object into
> > directory or local name space (struct namespace). To do that I will need
> > to know what object type are we creating and what object type is the
> > parent.
>
> Sorry, but that's wrong. You should *never* check the object type
> explicitly. You need a generic function to insert an object into a
> directory, and a generic directory type that can implement the various
> types of directories.

I'm not sure if that's possible. You can't insert mutex or even into
named pipe's name space. Same, you can't insert named pipe or desktop
into a directory object.

Because we don't have object type objects, I can't think of any other way
to check that we are inserting the right object type into the correct
place.

Implement directory object in wineserver.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> I'm not sure if that's possible. You can't insert mutex or even into
> named pipe's name space. Same, you can't insert named pipe or desktop
> into a directory object.
>
> Because we don't have object type objects, I can't think of any other way
> to check that we are inserting the right object type into the correct
> place.

You shouldn't need a generic check, each object knows its type when
it's created so it can do the check. In fact you don't need a generic
directory, you can use completely different types for the different
kinds of directories.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 11:30:18 AM, Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > I'm not sure if that's possible. You can't insert mutex or even into
> > named pipe's name space. Same, you can't insert named pipe or desktop
> > into a directory object.
> >
> > Because we don't have object type objects, I can't think of any other way
> > to check that we are inserting the right object type into the correct
> > place.
> You shouldn't need a generic check, each object knows its type when
> it's created so it can do the check. In fact you don't need a generic

The problem is not with an object creation. The problem is with inserting
an object into the parent's name space. Basically I need to know when to
call "insert_into_dir" and when to call "set_object_name".
> directory, you can use completely different types for the different
> kinds of directories.

Mm no, directory is a directory. Native does not have different types of
directories. So we shouldn't either. The trick here is that OM doesn't
care about each object's internal name space. It can be implemented in
any way driver want to implement it. But because we don't have such a
flexible implementation, we have to cross the line in some places.

Implement directory object in wineserver.

[Robert Shearman]

Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > Basically event, mutex, section, timer, etc can go there with minor
> > changes to code. Named pipes, mail slots and winstations with desktops
> > are a bit different. They have their own name space, that I'm planning on
> > using current namespace mechanism for. With the sample code I've sent to
> > you some days ago to show how that would work with named pipes.
> > find_object_dir worked perfectly on named pipes. The problem is object
> > creation. Actually not a creation itself, but insertion into the local
> > name space. I need to call different functions to insert an object into
> > directory or local name space (struct namespace). To do that I will need
> > to know what object type are we creating and what object type is the
> > parent.
>
> Sorry, but that's wrong. You should *never* check the object type
> explicitly. You need a generic function to insert an object into a
> directory, and a generic directory type that can implement the various
> types of directories.

I think there is a bit of a communication problem here. Vitaliy isn't 
proposing different types of directories. There will only every be one 
type - one that holds zero or more objects of any type. I also don't see 
any problem with what Vitaliy is doing with the object ops. Am I right 
in think this function is the one that you are commenting on:
> +/* open a new handle to an existing object */
> +obj_handle_t open_object_dir( const struct object_attr *attr, const struct object_ops *ops,
> -
>                          unsigned int access )
>
> +{
> - obj_handle_t handle = 0;
> - struct unicode_str name_left;
> - struct object *obj;
> -
> - if ((obj = find_object_dir( attr, &name_left )))
> - {
> -
>    if (name_left.len) /* not fully parsed */
> -
>        set_error( STATUS_OBJECT_PATH_NOT_FOUND );
> -
>    else if (ops && obj->ops != ops)
> -
>        set_error( STATUS_OBJECT_TYPE_MISMATCH );
> -
>    else
> -
>        handle = alloc_handle( current->process, obj, access, attr->attributes & OBJ_INHERIT );
> -
> -
>    release_object( obj );
> -
>    free_unicode_str( &name_left );
> - }
> - return handle;
>
> +}

If so, I don't see much of a problem with it. Maybe we shouldn't return 
a handle, but instead return a "struct object *" or a "void *", but 
passing in struct object_ops * into the function isn't doing anything 
other than eliminating common code in most of the objects. It doesn't 
force anything to be exported outside of the implementation of the object.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 11:05:05 AM, Robert Shearman wrote:
> Alexandre Julliard wrote:
> > Vitaliy Margolen  writes:
> >
> > +/* open a new handle to an existing object */
> > +obj_handle_t open_object_dir( const struct object_attr *attr, const struct object_ops *ops,
> > -
> >                          unsigned int access )
> If so, I don't see much of a problem with it. Maybe we shouldn't return
> a handle, but instead return a "struct object *" or a "void *", but 
> passing in struct object_ops * into the function isn't doing anything 
> other than eliminating common code in most of the objects. It doesn't 
> force anything to be exported outside of the implementation of the object.

The part I'm talking about is this:
Using object_attr:
static struct object *create_mapping( const struct object_attr *attr, file_pos_t size, int protect,
                                      obj_handle_t handle )

No using object_attr:
static struct object *create_mapping( const struct object *root, const struct unicode_str *name,
                                      unsigned int attr, file_pos_t size, int protect,
                                      obj_handle_t handle )

And this:
struct object_attr
{
    obj_handle_t              rootdir;     /* RootDirectory */
    struct unicode_str       *name;        /* ObjectName */
    unsigned int              attributes;  /* Attributes */
};

I can replace handle with object * but that will just add more code to
each
[open|close]_[even|mapping|semaphore|timer|namedpipe|winstation|desktop]

Implement directory object in wineserver.

[Robert Shearman]

Vitaliy Margolen wrote:
> Wednesday, November 23, 2005, 11:05:05 AM, Robert Shearman wrote:
> > Alexandre Julliard wrote:
> > > Vitaliy Margolen  writes:
> > >
> > > +/* open a new handle to an existing object */
> > > +obj_handle_t open_object_dir( const struct object_attr *attr, const struct object_ops *ops,
> > > -
> > >                          unsigned int access )
> > If so, I don't see much of a problem with it. Maybe we shouldn't return
> > a handle, but instead return a "struct object *" or a "void *", but 
> > passing in struct object_ops * into the function isn't doing anything 
> > other than eliminating common code in most of the objects. It doesn't 
> > force anything to be exported outside of the implementation of the object.
>
> The part I'm talking about is this:
> Using object_attr:
> static struct object *create_mapping( const struct object_attr *attr, file_pos_t size, int protect,
>                                      obj_handle_t handle )
>
> No using object_attr:
> static struct object *create_mapping( const struct object *root, const struct unicode_str *name,
>                                      unsigned int attr, file_pos_t size, int protect,
>                                      obj_handle_t handle )
>
> And this:
> struct object_attr
> {
>    obj_handle_t              rootdir;     /* RootDirectory */
>    struct unicode_str       *name;        /* ObjectName */
>    unsigned int              attributes;  /* Attributes */
> };
>
> I can replace handle with object * but that will just add more code to
> each
> [open|close]_[even|mapping|semaphore|timer|namedpipe|winstation|desktop]

That obviously should be a "struct directory *" (note: not const since 
you will be inserting it into the directory) if it isn't an 
obj_handle_t. However, I agree that the check would be best inside of 
create_named_object_dir, since nothing else in the creation paths is 
going to use the root directory and the root directory will always be of 
type "directory". I don't really have an opinion on whether a struct 
object_attr is better than passing parameters explicitly.

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 11:59:24 AM, Robert Shearman wrote:
> Vitaliy Margolen wrote:
> > Wednesday, November 23, 2005, 11:05:05 AM, Robert Shearman wrote:
> > Using object_attr:
> > static struct object *create_mapping( const struct object_attr
> > *attr, file_pos_t size, int protect,
> >                                      obj_handle_t handle )
> >
> > No using object_attr:
> > static struct object *create_mapping( const struct object *root, const struct unicode_str *name,
> >                                      unsigned int attr, file_pos_t size, int protect,
> >                                      obj_handle_t handle )
>
> That obviously should be a "struct directory *" (note: not const since
> you will be inserting it into the directory) if it isn't an 
> obj_handle_t. However, I agree that the check would be best inside of 
> create_named_object_dir, since nothing else in the creation paths is 
> going to use the root directory and the root directory will always be of
> type "directory". I don't really have an opinion on whether a struct 
> object_attr is better than passing parameters explicitly.

Ok, then this is what we have:
1. Pass arguments explicitly to each function (rootdir, name,
   attributes).
2. Don't use object_attr structure.

Only left is to decide what rootdir should be (struct object*,
struct directory * or obj_handle_t). The only place that knows enough and
does all the checking is find_object_dir (create_object_dir calls it to
do most of the work). It can't be (struct directory*) because I'm planning
on using these functions for named pipes, mail slots, win stations and
desktops.

So we have only chose of (struct object *) or (obj_handle_t). I'm
thinking we should use (struct object *) then. And call get_directory_obj
in each [open|create]_*

Now, about inserting a new object into a name space. Will it be ok to add
one more function to object_ops? Something like

int obj_insert(struct object *parent, struct object *new_obj); /* 1 on success */

Implement directory object in wineserver.

[Vitaliy Margolen]

Wednesday, November 23, 2005, 12:37:05 PM, Vitaliy Margolen wrote:
> Ok, then this is what we have:
> - Pass arguments explicitly to each function (rootdir, name,
> attributes).
> - Don't use object_attr structure.
>
> Only left is to decide what rootdir should be (struct object*,
> struct directory * or obj_handle_t). The only place that knows enough and

Sorry, answered myself. It have to be (struct directory *). It turns out
you really can't use any other objects for RootDirectory. And to keep
everything in-line that's what it will be.

So I'm guessing all the questions are answered for a time being.

Thank you Alexandre and Robert.