[Mono-winforms-list] Extended error reporting for libgdiplus/System.Drawing
Sebastien Pouliot
sebastien at ximian.com
Tue Sep 5 08:51:03 EDT 2006
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 ;-)
> I didn't think it would be too hard to make them a
> lot better, even without breaking cross-platform compatibility, but I
> didn't have the free time to sit down and work it out until now.
>
> My proposal is two added functions to libgdiplus, along with a new GpStatus
> value.
>
> GpStatus.ExtendedError (1000000): Indicates that the last error which
> occurred has an extended description accessible through
> GdipGetErrorEx ().
>
> GpStatus GdipSetErrorEx (GpStatus status, char *description): Places
> the specified status code & description into TLS slots allocated
> for the purpose and returns GpStatus.ExtendedError. In the event
> that TLS slots could not be allocated (a non-fatal error during
> GdipStartup ()), GdipSetErrorEx actually simply returns its first
> argument, and the feature is transparently disabled.
>
> int GdipGetErrorEx (GpStatus *status, char *description): Copies the
> last error set with GdipSetErrorEx into the parameters. Either
> parameter can be NULL. If description is not NULL, then it is
> assumed to be large enough to store the string. The caller
> determines this size by first calling the function with a NULL
> pointer for description; the return value is the necessary number
> of bytes (including null-terminator) for the buffer.
>
> This extended error status code (GpStatus.ExtendedError) is then detected
> in the System.Drawing side. Of course, we would break compatibility if we
> added an entry for it to the managed enumeration, but it's easy enough to
> test for by casting the Status result to an int. If the GDIPlus.CheckStatus
> function finds this extended error status value, then it uses the
> GdipGetErrorEx function to get the actual error details. When
> System.Drawing is running against Microsoft's GDIPLUS.DLL, it will never
> get this value (or so we can reasonably assume) and thus it won't ever try
> to call GdipGetErrorEx (which won't be exported by GDIPLUS.DLL, of course).
>
> I have attached a pair of patches which implement this functionality,
> paving the way for the translation of error return codes in libgdiplus to
> have more meaningful descriptions.
>
> extendederrorreporting.libgdiplus.diff: Apply in "/source/trunk/libgdiplus"
> extendederrorreporting.System.Drawing.diff: Apply in
> "/source/trunk/mcs/class/System.Drawing"
>
> Thoughts?
I don't see why this wouldn't work.
> 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.
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).
* 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.
> 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 ?
> Thanks,
>
> Jonathan Gilbert
>
> -----
> I have included the patches inline in the e-mail as well as attaching them
> in gzipped form, because my e-mail client
> isn't smart enough to properly encode text attachments with LF-only line
> endings. Enjoy.
>
> === extendederrorreporting.libgdiplus.diff ===
> Index: general.c
> ===================================================================
> --- general.c (revision 64773)
> +++ general.c (working copy)
> @@ -52,12 +52,21 @@
>
> extern void cairo_test_xlib_disable_render();
>
> +static pthread_key_t last_error_status_tls_key,
> last_error_description_tls_key;
> +static BOOL use_extended_error_reporting = TRUE;
> +
> GpStatus
> GdiplusStartup(unsigned long *token, const struct startupInput *input,
> struct startupOutput *output)
> {
> /* don't initialize multiple time, e.g. for each appdomain */
> if (!startup) {
> startup = TRUE;
> +
> + /* 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
> g_mem_allocations = NULL;
> initCodecList ();
> FcInit ();
> @@ -693,3 +702,41 @@
>
> cairo_curve_to (graphics->ct, x1, y1, x2, y2, x3, y3);
> }
> +
> +/* 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. */
> + }
> +}
> +
> Index: gdip.h
> ===================================================================
> --- gdip.h (revision 64773)
> +++ gdip.h (working copy)
> @@ -189,7 +189,14 @@
> UnsupportedGdiplusVersion = 17,
> GdiplusNotInitialized = 18,
> PropertyNotFound = 19,
> - PropertyNotSupported = 20
> + PropertyNotSupported = 20,
> +
> + /* 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
> } GpStatus;
>
> typedef enum {
> @@ -1057,6 +1064,9 @@
> int gdip_from_RGB_to_ARGB (BYTE *src, int width, int height, int stride,
> BYTE **dest, int* dest_stride);
> int gdip_from_ARGB_to_RGB (BYTE *src, int width, int height, int stride,
> BYTE **dest, int* dest_stride);
>
> +/* 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
> /* Pen */
> void gdip_pen_init (GpPen *pen);
> GpPen *gdip_pen_new (void);
>
> === extendederrorreporting.System.Drawing.diff ===
> Index: System.Drawing/gdipFunctions.cs
> ===================================================================
> --- System.Drawing/gdipFunctions.cs (revision 64908)
> +++ System.Drawing/gdipFunctions.cs (working copy)
> @@ -161,9 +161,38 @@
> return dest;
> }
>
> + // 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 ?
> + 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).
> // Converts a status into exception
> static internal void CheckStatus (Status status)
> {
> + string error_text = null;
> +
would be simpler if 1000000 was a mask
> + 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);
> + }
> +
> switch (status) {
>
> case Status.Ok:
> @@ -172,43 +201,43 @@
> // TODO: Add more status code mappings here
>
> case Status.GenericError:
> - throw new Exception ("Generic Error.");
> + throw new Exception (coalesce (error_text, "Generic Error."));
>
> case Status.InvalidParameter:
> - throw new ArgumentException ("Invalid Parameter. A null reference or
> invalid value was found.");
> + throw new ArgumentException (coalesce (error_text, "Invalid
> Parameter. A null reference or invalid value was found."));
>
> case Status.OutOfMemory:
> - throw new OutOfMemoryException ("Out of memory.");
> + throw new OutOfMemoryException (coalesce (error_text, "Out of
> memory."));
>
> case Status.ObjectBusy:
> - throw new MemberAccessException ("Object busy.");
> + throw new MemberAccessException (coalesce (error_text, "Object busy."));
>
> case Status.InsufficientBuffer:
> - throw new IO.InternalBufferOverflowException ("Insufficient buffer.");
> + throw new IO.InternalBufferOverflowException (coalesce (error_text,
> "Insufficient buffer."));
>
> case Status.PropertyNotSupported:
> - throw new NotSupportedException ("Property not supported.");
> + throw new NotSupportedException (coalesce (error_text, "Property not
> supported."));
>
> case Status.FileNotFound:
> - throw new IO.FileNotFoundException ("File not found.");
> + throw new IO.FileNotFoundException (coalesce (error_text, "File not
> found."));
>
> case Status.AccessDenied:
> - throw new UnauthorizedAccessException ("Access denied.");
> + throw new UnauthorizedAccessException (coalesce (error_text, "Access
> denied."));
>
> case Status.UnknownImageFormat:
> - throw new NotSupportedException ("Either image format is unknown or
> you don't have the required libraries for this format.");
> + throw new NotSupportedException (coalesce (error_text, "Either image
> format is unknown or you don't have the required libraries for this
> format."));
>
> case Status.NotImplemented:
> - throw new NotImplementedException ("Feature not implemented.");
> + throw new NotImplementedException (coalesce (error_text, "Feature not
> implemented."));
>
> case Status.WrongState:
> - throw new ArgumentException ("Properties not set properly.");
> + throw new ArgumentException (coalesce (error_text, "Properties not
> set properly."));
>
> case Status.FontFamilyNotFound:
> - throw new ArgumentException ("FontFamily wasn't found.");
> + throw new ArgumentException (coalesce (error_text, "FontFamily wasn't
> found."));
>
> default:
> - throw new Exception ("Unknown Error.");
> + throw new Exception (coalesce (error_text, "Unknown Error."));
> }
> }
>
> _______________________________________________
> Mono-winforms-list maillist - Mono-winforms-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-winforms-list
--
Sebastien Pouliot <sebastien at ximian.com>
Blog: http://pages.infinit.net/ctech/
More information about the Mono-winforms-list
mailing list