Re: ole32: Print error message when IPersistStorage_Load fails (Coverity)

[Huw Davies]

On Tue, Dec 22, 2015 at 07:35:08PM +0100, André Hentschel wrote:
> Signed-off-by: André Hentschel <nerv(a)dawncrow.de>
> ---
> CID 1336207
>
> dlls/ole32/compobj.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c
> index 2f55076..b146376 100644
> --- a/dlls/ole32/compobj.c
> +++ b/dlls/ole32/compobj.c
> @@ -3443,7 +3443,9 @@ HRESULT WINAPI CoGetInstanceFromIStorage(
>
> if (ps)
>    {
> -      IPersistStorage_Load(ps, storage);
> +      hr = IPersistStorage_Load(ps, storage);
> +      if (FAILED(hr))
> +          ERR("failed to load storage\n");
>        IPersistStorage_Release(ps);
>    }

I suspect Coverity is expecting us to do something
more useful with the return value ;-)

How about returning the failure in this case (obviously having
released ps first)?  Also, just above this hunk, we should probably
just return if QI fails.

While you're at it, there are other CoCreateInstanceFrom*
functions that could do with similar love.

Huw.

ole32: Print error message when IPersistStorage_Load fails (Coverity)

[Nikolay Sivov]

On 23.12.2015 13:37, Huw Davies wrote:
> On Tue, Dec 22, 2015 at 07:35:08PM +0100, André Hentschel wrote:
> > Signed-off-by: André Hentschel <nerv(a)dawncrow.de>
> > ---
> > CID 1336207
> >
> > dlls/ole32/compobj.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c
> > index 2f55076..b146376 100644
> > --- a/dlls/ole32/compobj.c
> > +++ b/dlls/ole32/compobj.c
> > @@ -3443,7 +3443,9 @@ HRESULT WINAPI CoGetInstanceFromIStorage(
> >
> > if (ps)
> >    {
> > -      IPersistStorage_Load(ps, storage);
> > +      hr = IPersistStorage_Load(ps, storage);
> > +      if (FAILED(hr))
> > +          ERR("failed to load storage\n");
> >        IPersistStorage_Release(ps);
> >    }
> > I suspect Coverity is expecting us to do something
> > more useful with the return value ;-)
>
> How about returning the failure in this case (obviously having
> released ps first)?  Also, just above this hunk, we should probably
> just return if QI fails.
>
> While you're at it, there are other CoCreateInstanceFrom*
> functions that could do with similar love.
> Ideally we want this tested, it should be trivial to do with temporary
> registered class. I'm not working on this right now, so anyone is
> welcome to go ahead. But I agree failed Load() is most likely important,
> as it's the main purpose of this api.
>
> Huw.