[Mono-dev] [Mono-winforms-list] Extended error reportingforlibgdiplus/System.Drawing

[Jonathan Gilbert]

At 08:51 AM 05/09/2006 -0400, Sebastien Pouliot wrote:
>Hello Jonathan,
>
>On Mon, 2006-09-04 at 17:57 -0500, Jonathan Gilbert wrote:
>> Ever since I started working on libgdiplus and the related System.Drawing
>> bits, I've always been somewhat unimpressed by the error reporting
>> capabilities of GDI+. 
>
>True, still very similar to most of the Win32 API ;-)

Well yes, except that the Win32 API typically does not do a great deal in
any given function call, whereas GDI+ has many layers & modules which can
be involved in a single call (e.g. GdipLoadImageFromStream).

For instance, while loading a BMP file, if the stream is cut off early (or
in fact if any partial fread() occurs), the error code returned will be
InvalidParameter, to indicate that the filename provided refers to an
invalid file. At present, this will be turned into an exception of type
ArgumentException with message:

  Invalid Parameter. A null reference or invalid value was found.

This really doesn't convey what actually went wrong, and with extended
error reporting, the code which currently does:

  status = InvalidParameter;

..could be changed to something like:

  status = GdipSetErrorEx(InvalidParameter, "Unexpected end-of-file while
reading BMP data.");

>> Comments? 
>
>Yes.
>
>On a general note, I don't like making big changes this late in the 1.2
>beta cycle. No the patch itself isn't big, however it won't be useful
>until other large changes are made.

I wasn't aware that 1.2 was approaching; if it's going to be soon, then I
would recommend applying this patch to HEAD now (or if a branch for 1.2 has
not yet been made, just after one has been made) but deferring any changes
to make use of the new function until after the release.

>On a technical level:
>
>* this introduce user-visible strings inside libgdiplus - something we
>do not have right now. While translation hasn't been a big issue yet (in
>Mono source code) it will become one (sooner than later) and should be
>addressed as a "day-1" issue (i.e. before the patch gets approved).

I think GdipSetErrorEx() would be the logical place for any
internationalization efforts, since all of the custom strings go through
there. Of course, this would prevent the use of tools which look for calls
through the _() macro...

>* while error reporting is weak I can't recall many times where an error
>needed a more a detailed explanation. So I'm unsure how much we really
>gain (on the Linux side). Evidently this doesn't affect (nor help) the
>Windows side (Mono or MS runtime) where most of the System.Drawing
>development currently occurs.

Well, specific errors indicating why an image file couldn't be loaded is
one area that would really get a benefit out of this. Other places wouldn't
have to use the system until someone found a strong reason for it.

>> If nobody thinks this is a bad idea (and some people
>> think it is a good idea), I can commit it.
>
>My feeling is that it's not a bad idea *but* this should wait after Mono
>1.2 release because it won't help immediately, other things needs to be
>addressed and (finally) if we should add this it would be nice to do it
>with some refactoring of the current source code.
>
>However it's worth a longer discussion, along with other current issues
>in libgdiplus. Are you coming, by any chance, to the Mono Summit ?

I'm afraid I won't be able to make it.

>> +
>> +		/* Ensure TLS storage for error information. */
>> +		if (pthread_key_create(&last_error_status_tls_key, NULL)
>> +		 || pthread_key_create(&last_error_description_tls_key, free))
>> +			use_extended_status = FALSE;
>> +
>
>been a while ? ;-) 
>this (and more) doesn't follow Mono source code conventions

I'm far more accustomed to no-spare-before-paren styles, but I did catch
myself while making this patch and I thought I had fixed it. Maybe I made
the diff before that point and forgot to re-create it. Certainly, I'll
review it closely for this one.

>> +/* New Error-Reporting Functionality */
>> +GpStatus
>> +GdipSetErrorEx(GpStatus status, char *description)
>> +{
>> +	if (!use_extended_error_reporting)
>> +		return status;
>> +
>> +	free(pthread_getspecific(last_error_description_tls_key));
>> +
>> +	pthread_setspecific(last_error_status_tls_key, (void *)status);
>> +	pthread_setspecific(last_error_description_tls_key, strdup(description));
>> +
>> +	return ExtendedError;
>> +}
>> +
>> +int
>> +GdipGetErrorEx(GpStatus *status, char *description)
>> +{
>> +	char *last_error_description;
>> +
>> +	if (status != NULL)
>> +		*status = (GpStatus)pthread_getspecific(last_error_status_tls_key);
>> +
>> +	last_error_description =
>> pthread_getspecific(last_error_description_tls_key);
>> +
>> +	if (last_error_description != NULL) {
>> +		if (description != NULL)
>> +			strcpy(description, last_error_description);
>> +		return strlen(last_error_description) + 1;
>> +	}
>> +	else {
>> +		if (description != NULL)
>> +			description[0] = '\0';
>> +		return 1; /* Null-terminator only. */
>> +	}
>> +}

Yeah, the spaces before parentheses are completely missing from the patch I
submitted. My bad :-)

(Personally, I actually think it looks pretty ugly that way, but it's not a
big issue...)

>> +    /* New error-reporting mechanism: This value means that the caller
>> +     * can assume that GdipGetErrorEx () will be present and will
>> +     * return a meaningful error string along with the correct GpStatus
>> +     * value for the last operation on the current thread.
>> +     */
>> +    ExtendedError = 1000000
>
>maybe we should use this as a flag, and not a value

I prefer it to be a value, because existing code wouldn't know what to do
with a flag anyway, and I would very much like to keep the status stored
alongside its description; it doesn't feel right to have descriptions
stranded with no indication of what the error was at the time.

>> +/* Error */
>> +int GdipGetErrorEx(GpStatus *status, char *description);
>
>please move this into general.h
>
>note: someday we'll need to clean up public/internal API definitions so
>that libgdiplus can be used outside Mono

Yeah, I was going to put it into general.h, but it didn't look like that
would be visible to outsiders who #include'd <gdip.h>.

>> +		// Extended error reporting
>> +		[DllImport("gdiplus")]
>> +		static extern int GdipGetErrorEx (out Status status, StringBuilder
>> description);
>> +
>> +		[DllImport("gdiplus")]
>> +		static extern int GdipGetErrorEx (IntPtr status_ptr, StringBuilder
>> description);
>
>Shouldn't this one be GdipSetErrorEx ?

Nope, these are both GdipGetErrorEx. One of them takes an "out Status" for
the first parameter; the other one permits passing NULL in (IntPtr.Zero),
when the status value is not being retrieved (in this case, because it has
already been retrieved).

The extended error is currently never set from within System.Drawing, and
though there's no reason why it *couldn't* be, I can't see any reason why
it *should* be either.

>> +		static private string coalesce (string t1, string t2)
>> +		{
>> +			return (t1 != null) ? t1 : t2;
>> +		}
>> +
>
>That could be a place to introduce localization, however this would mean
>libgdiplus itself wouldn't be translated (there's good and bad in both
>cases).

I think this function should be kept simple, so that its function is what
its name says it is. Certainly, though, the default text should be
localized at the same time as any localization occurring within libgdiplus.

>>  		// Converts a status into exception
>>  		static internal void CheckStatus (Status status)
>>  		{
>> +			string error_text = null;
>> +
>> +			if (status == (Status)1000000) { /* GpStatus.ExtendedError */
>> +				int text_length = GdipGetErrorEx (out status, null);
>> +
>> +				StringBuilder text_buffer = new StringBuilder ();
>> +				text_buffer.Length = text_length;
>> +
>> +				GdipGetErrorEx (IntPtr.Zero, text_buffer);
>> +
>> +				for (text_length = 0; text_length < text_buffer.Length; text_length++)
>> +					if (text_buffer[text_length] == '\0')
>> +						break;
>> +
>> +				error_text = text_buffer.ToString(0, text_length);
>> +			}
>
>would be simpler if 1000000 was a mask

Not really; it still needs to make two calls to GdipGetErrorEx, because it
needs to get the number of chars to allocate in the StringBuilder before
the call. I'd prefer to keep it this way, too, along with the strdup() in
the TLS slot assignment, because it makes it possible for people to
construct error text that includes parameter data (such as a filename).

Using it as a mask would also require a range comparison instead of just
straight equality.

Thanks for the detailed review :-)

Jonathan Gilbert