[Mono-list] [PATCH] robust mono-handle-d?

[Dick Porter]

On Fri, Jun 07, 2002 at 04:35:52PM +0200, Dennis Haney wrote:
> Dick> Not just file handles, there are many sorts.  Please change this
> Dick> wording throughout the doc and the comments.
> 
> I believe that was the only place I was wrong. All other places I talk
> about filehandles I refer to the filehandle of the client-sockets...

They're called "file descriptors" in Linux :)

> Dick> Some typos here
> 
> All I can see is the Free having a <sp> before instead of
> after. Anymore?

That was all.

> I now do the check both places. Point was that given a struct like:
> 
> struct p {
>    int a;
>    union b {
>         int c[1000];
>         int d;
>    }
> }
> 
> If the client only fills out d, then it might only send a package
> which is two ints long.

The sendto is given the sizeof the whole struct, not just one of the
union parts.

> Dick> Please move the Error enum to the top of the list, to be
> Dick> symmetrical with the responses.
> 
> Okay, this breaks binary compat though...

Yeah, I know, but we only exec the installed binary.  Paolo did suggest
building the daemon into the library and just fork() without execing
anything.  That's a possible fix for any binary compat problems.

> Dick> I prefer to not use default in switch statements like this, so
> Dick> the compiler will remind me when I've forgotten to add a case.
> 
> It is to catch faulty packages...

OK.

> 
> I just have one question about the daemon... Why does it exist? Isn't
> it better performancewise[2] to just protect the shared area with a
> mutex when allocation a new handle/shared mem segment or changing
> refcnt? It will however be a less resilient to clients that crash (the
> deamon cleans up ref'd handles if socket closes)

It's precisely because with a mutex the shared memory segment can be left
in a locked state. Also, it's not so easy to clean up shared memory without
it (you can't just mark it deleted when creating it, because you can't
attach any more readers to the same segment after that).

I did some minimal performance testing, and I don't think the daemon is
particularly slow.

> Index: daemon.c
> @@ -120,9 +160,7 @@
>  	}
>  	
>  	if(_wapi_shared_data->handles[handle].ref==0) {
> -		if (open_handles[handle]!=0) {
> -			g_warning (G_GNUC_PRETTY_FUNCTION ": per-process open_handles mismatch, set to %d, should be 0", open_handles[handle]);
> -		}
> +	        g_assert(open_handles[handle] == 0);

This change shouldnt go in.  The daemon shouldn't quit if it's still servicing
clients, and it should clean up when it does quit.

If anything, it should just moan with a g_warning and set
open_handles[handle]=0;

> @@ -314,9 +421,15 @@
>  		process_new (idx, req.u.new.type);
>  		break;
>  	case WapiHandleRequestType_Open:
> +#ifdef DEBUG
> +		g_assert(req.u.open.handle < _WAPI_MAX_HANDLES);
> +#endif

I'd prefer g_warning()s instead of g_assert()s here, even though it's marked
DEBUG.

>  		process_open (idx, req.u.open.handle);
>  		break;
>  	case WapiHandleRequestType_Close:
> +#ifdef DEBUG
> +		g_assert(req.u.close.handle < _WAPI_MAX_HANDLES);
> +#endif

And here.

>  		process_close (idx, req.u.close.handle);
>  		break;
>  	case WapiHandleRequestType_Scratch:


Everything else is fine to commit.

Thanks,

- Dick