Re: gerror.c revisited

[Havoc Pennington <hp redhat com>]

Note that the recent gexec.[hc] I posted has some GError examples, as
well (may be useful).

Sebastian Wilhelmi <wilhelmi@ira.uka.de> writes: 
> * I ended up defining 
> 
>   #define glib_error_domain() g_quark_from_static_string ("glib-error-domain")
> 
>   in gerror.h. As far as I can tell it is quite hard to get such a
>   glib_error_domain statically defined. So in essence the question is, whether 
>   an approach similiar to GLogDomain (i.e. take just a string) might be better
>   suited for the API. The domain is only accessed inside g_error_matches,
>   which in turn is only called after an error has occured, so the overhead of
>   doing strcmp there shouldn't be too bad. We might as well just take
>   GLogDomain for added consistency!

I'm doing it this way in gexec.h:
 #define G_EXEC_ERROR g_exec_error_quark ()

 GQuark g_exec_error_quark();

Then the convention for error codes is G_EXEC_ERROR_* members in
GExecErrorType.

I guess since you already can't use switch(), it may as well be a
string. That makes sense to me. Anyone see a problem with it?

> * I would very much like to see a function
>   g_print_error (GError **err, gboolean fatal), that prints out the error in
>   some canonical form and does abort, if fatal is true. Or two functions, one
>   for error, one for warning (without abort).

Seems reasonable. On fatal, I would fprintf(stderr, message); then
exit(1) rather than abort(), though. abort() means "should not
happen", i.e. g_assert(), and GError should not be used to indicate
programming bugs.

> * g_set_error should call g_print_error (the_error, TRUE), when err is NULL.
>   Because if I supply NULL as an err argument, I actually don't want to know,
>   in the calling code whether it fails, but I'm not calling out for silent
>   failure, when the error condition is occuring. Or let g_set_error take a
>   fatal argument as above.
> 

No, NULL means you want to _ignore_ the error, because the error
doesn't matter. If the error matters, you need to handle it. :-P If
the error "should not happen", then don't return it as a GError, just
print a g_warning(). 

Part of the purpose of GError is to make our error handling better
than just spewing crap to stderr and then crashing. ;-) The contents
of ~/.xsession-errors is one of the worst features of GNOME.
  
> +#define glib_error_domain() g_quark_from_static_string ("glib-error-domain")
> +#define GLIB_ERROR_AGAIN   1   /* Resource temporarily unavailable */
> +

I would do this as G_THREAD_ERROR (for the domain) and an enum
GThreadErrorType with member G_THREAD_ERROR_AGAIN. The convention is
debatable, but we should use the same one throughout glib.

> +  G_THREAD_UF (thread_create, (g_thread_create_proxy, result, 
> +			       stack_size, joinable, bound, priority,
> +			       &result->system_thread, err));
>    G_UNLOCK (g_thread_create);
> +  if (err && *err)
> +    {
> +      g_free (result);
> +      return NULL;
> +    }
>    return (GThread*) result;
>  }

This code is probably wrong (I'm not sure, it depends on exactly what
thread_create does). Basically the return value should not be affected
by whether the user passsed in NULL for err. If you need to know
whether an error occurred in _your_ code, then create your own error
variable, check that it's non-NULL, then assign to the user's error if
needed. I'm pretty sure gexec.c has an example, but something like:

GError *local_err = NULL;
foo (&local_err);
if (local_err != NULL)
{
  if (err) 
   *err = local_err;
  else
   g_error_free (local_err);

  return NULL;
}

> +	{
> +	  g_thread_pool_start_thread (retval, err);
> +	  if (err && *err)
> +	    {
> +	      g_free (retval);
> +	      /* FIXME: free all other threads */
> +	      return NULL;
> +	    }
> +	}
>

Same comment, the user ignoring errors should not affect function
semantics.

> +      g_thread_pool_start_thread (real, err);
> +      if (err && *err)
> +	{
> +	  g_async_queue_unlock (real->queue);
> +	  return;
> +	}

And again.

> +      if (err)
> +	g_error ("%d, %d, %s.\n", err->domain, err->code,
> err->message);

g_error() should be for "should not happen" cases, not for reportable
errors, because it dumps core.

Havoc