Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Vladimir Sementsov-Ogievskiy]

17.09.2020 19:32, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy 
> <vsementsov@virtuozzo.com> wrote:
>
> > + * On success return true with bm_list set (probably to NULL, if no bitmaps),
>
> " probably " ? :-)

I note this as "set to NULL" is not obvious thing (is it "unset" ? :).. And by "probably" I mean 
"may be", i.e. NULL is just one of possible cases. Probably I use "probably" in a wrong way?

> > + * on failure return false with errp set.
> >    */
> > -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> > -                                                Error **errp)
> > +bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
> > +                                Qcow2BitmapInfoList **info_list, Error **errp)
> >   {
> >       BDRVQcow2State *s = bs->opaque;
> >       Qcow2BitmapList *bm_list;
> >       Qcow2Bitmap *bm;
> > -    Qcow2BitmapInfoList *list = NULL;
> > -    Qcow2BitmapInfoList **plist = &list;
>
> So here 'list' points at NULL and 'plist' at &list.

Hmm, to be precise, list _is_ NULL (and points nowhere), and plist points to 
list.

> > -        *plist = obj;
> > -        plist = &obj->next;
>
> In the original code 'plist' is updated when you add a new element, so
> it always points at the end of the list. But 'list' is unchanged and it
> still points at the first element.
>
> So the caller receives a pointer to the first element.
>
> > +        *info_list = obj;
> > +        info_list = &obj->next;
>
> But in the new code there is only one variable (passed by the caller),
> which always points at the end of the list.

No: at first "*info_list = obj", we set the result which user will get, users 
pointer now points to the first object in the list.
Then, at "info_list = &obj->next", we reassign info_list to another pointer: to "next" 
field of first list item. So, all further "*info_list = obj" are note visible to the caller.

Actually, the logic is not changed, just instead of plist we use info_list, and instead of list - a variable which 
should be defined in the caller. Look: in old code, first "*plist = obj" sets "list" variable, but 
all further "*plist = obj" don't change "list" variable.


--
Best regards,
Vladimir