[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Dmitry Timoshkov]

Qian Hong <qhong at codeweavers.com> wrote:

> >> +    *bitmap = NULL;
> >
> > Is there an application that depends on this behaviour?
> 
> No, I don't know yet. I hope maybe some app will be benefited from
> this change. Any advice?

Probably this change is OK since it matches what GdipLoadImageFromFile does.

> >>      stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
> >>
> >>      if(stat != Ok)
> >> -        return stat;
> >> +        return InvalidParameter;
> >
> > Probably GdipCreateStreamOnFile should be fixed instead (with tests
> > of course).
> 
> 1. I have some tests in [Patch 3/3] which show that the error code
> from GdipCreateStreamOnFile doesn't match the error code from
> GdipCreateBitmapFromFile.
> 2. I haven't found any real world app rely on return code from
> GdipCreateStreamOnFile, so I leave those todo_wine there.
> 3. I haven't found any real world app rely on return code from
> GdipCreateBitmapFromFile as well, I don't have strong opinion here.
> 
> What's your advice regarding the return code here?

A common rule is to propagate errors from lower level APIs.

> >>      stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
> >>
> >>      if (stat != Ok)
> >> -        return stat;
> >> +        return OutOfMemory;
> >
> > Same note about GdipCreateStreamOnFile. And I don't think that's a good
> > idea to replicate weird return error codes without actually getting any
> > memory allocation errors.
> 
> The same situation here:
> 1. return code from GdipCreateStreamOnFile doesn't match return code
> from GdipLoadImageFromFile
> 2. haven't found real world app reply on the return code yet.
> 
> Any advice?

There is no need to change something just to replicate behaviour that
you don't really understand.

-- 
Dmitry.