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.

-- 
Rob Shearman