Re: [PATCH 1/2] ieframe: Simplify IPersist_Load, make error handling more straightforward (Coverity)

[Jacek Caban]

Hi Nikolay,

On 04/08/16 21:56, Nikolay Sivov wrote:
> +    /* Now we're going to read in the iconfile and iconindex.
> +       If we don't find them, that's not a failure case -- it's possible
> +       that they just aren't in there. */
> +    hr = IPropertySetStorage_Open(This->property_set_storage, &FMTID_Intshcut,
> +                STGM_READWRITE | STGM_SHARE_EXCLUSIVE, &pPropStg);

It's an already existing bug, but we should handle Open failure here.

Thanks,
Jacek

[PATCH 1/2] ieframe: Simplify IPersist_Load, make error handling more straightforward (Coverity)

[Nikolay Sivov]

On 11.04.2016 18:30, Jacek Caban wrote:
> Hi Nikolay,
>
> On 04/08/16 21:56, Nikolay Sivov wrote:
> > +    /* Now we're going to read in the iconfile and iconindex.
> > +       If we don't find them, that's not a failure case -- it's possible
> > +       that they just aren't in there. */
> > +    hr = IPropertySetStorage_Open(This->property_set_storage, &FMTID_Intshcut,
> > +                STGM_READWRITE | STGM_SHARE_EXCLUSIVE, &pPropStg);
>
> It's an already existing bug, but we should handle Open failure here.

I think I kept it intentionally, to keep original logic intact in
cleanup patch. By handling it you mean making Load() fail if Open()
fails? Do you want me to make this change in the same patch? I can make
it 3/3 too.
> Thanks,
> Jacek

[PATCH 1/2] ieframe: Simplify IPersist_Load, make error handling more straightforward (Coverity)

[Jacek Caban]

On 04/11/16 18:00, Nikolay Sivov wrote:
> On 11.04.2016 18:30, Jacek Caban wrote:
> > Hi Nikolay,
> >
> > On 04/08/16 21:56, Nikolay Sivov wrote:
> > > +    /* Now we're going to read in the iconfile and iconindex.
> > > +       If we don't find them, that's not a failure case -- it's possible
> > > +       that they just aren't in there. */
> > > +    hr = IPropertySetStorage_Open(This->property_set_storage, &FMTID_Intshcut,
> > > +                STGM_READWRITE | STGM_SHARE_EXCLUSIVE, &pPropStg);
> > > It's an already existing bug, but we should handle Open failure here.
>
> I think I kept it intentionally, to keep original logic intact in
> cleanup patch.

That makes sense, adding 3/3 is fine then.
> By handling it you mean making Load() fail if Open() fails?

Save() fails in such case, so I'd say we should fail here.

Thanks,
Jacek