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

[Dick Porter]

On Thu, Jun 06, 2002 at 10:42:38PM +0200, Dennis Haney wrote:
> 
> This is primarily a documentation patch, but it also adds a single
> error check (assert if unref is called with refcnt==0) and many
> g_warning with a exit(-1) right after was changed to g_critical. The
> states a daemon was running in was replaced by an enum to make it more
> clean what is what.

Mostly OK.  I've commented on some details below.

- Dick


> +
> +The mono_handle_d is a process which takes care of the (de)allocation
> +of scratch shared memory and filehandles and refcounts of the
> +filehandles. It is designed to be run by a user and to be fast, thus

Not just file handles, there are many sorts.  Please change this wording
throughout the doc and the comments.

> +
> +=head2 How to start the deamon

Please make sure you've spelled "daemon" correctly throughout (including in
typedefs :) )

> +
> +Deallocate a shared memory area, this must have been allocated before
> +deallocating. A L<WapiHandleResponse> with
> +.type=WapiHandleResponseType_Scratch Freewill be sent back (works just

Some typos here

> Index: daemon-messages.c
> @@ -71,7 +71,11 @@
>  	int ret;
>  	
>  	ret=recv (fd, req, sizeof(WapiHandleRequest), MSG_NOSIGNAL);
> +	//we cant check ret != sizeof(WapiHandleRequest) because its a
> +	//union

Please replace the // comments with /* */

>  	
>  	ret=send (fd, resp, sizeof(WapiHandleResponse), MSG_NOSIGNAL);
> -	if(ret==-1) {
>  #ifdef DEBUG
> +	if(ret==-1 || ret != sizeof(WapiHandleResponse)) {

I don't understand this bit.  Earlier you said you can't use sizeof on a 
union, but here you are doing it.

> Index: daemon-messages.h
> @@ -18,6 +18,7 @@
>  	WapiHandleRequestType_Close,
>  	WapiHandleRequestType_Scratch,
>  	WapiHandleRequestType_ScratchFree,
> +	WapiHandleRequestType_Error,
>  } WapiHandleRequestType;

Please move the Error enum to the top of the list, to be symmetrical with
the responses.

> Index: daemon.c
> ===================================================================
>  
> +// Array to hold the filehandles we are currently polling
>  static struct pollfd *pollfds=NULL;

Comments again...

> -static gpointer *handle_refs=NULL;
> +static guint32 **handle_refs=NULL;

Good catch.

>  static gboolean unref_handle (guint32 idx, guint32 handle)
>  {
>  	guint32 *open_handles=handle_refs[idx];
> @@ -104,6 +137,8 @@
>  		return(FALSE);
>  	}
>  	
> +	g_assert(open_handles[handle] != 0);
> +

Shouldn't use assert in the daemon.  Just g_warning and ignore the request if
the handle isn't open.

> @@ -325,9 +433,19 @@
>  	case WapiHandleRequestType_ScratchFree:
>  		process_scratch_free (idx, req.u.scratch_free.idx);
>  		break;
> +	case WapiHandleRequestType_Error: /* Falltrough */
> +	default:
> +		/*FIXME: call rem_fd?*/
> +		break;
>  	}

I prefer to not use default in switch statements like this, so the compiler
will remind me when I've forgotten to add a case.


The rest is fine though.