Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

[Ilya Dryomov]

On Tue, Nov 24, 2015 at 8:23 PM, SF Markus Elfring
<elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>> @@ -5157,14 +5157,14 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>>>         if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
>>>                 pr_info("parent chain is too long (%d)\n", depth);
>>>                 ret = -EINVAL;
>>> -               goto out_err;
>>> +               goto unparent_device;
>>>         }
>>>
>>>         parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
>>>                                 NULL);
>>>         if (!parent) {
>>>                 ret = -ENOMEM;
>>> -               goto out_err;
>>> +               goto unparent_device;
>>>         }
>>>
>>>         /*
>>> @@ -5176,15 +5176,15 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>>>
>>>         ret = rbd_dev_image_probe(parent, depth);
>>>         if (ret < 0)
>>> -               goto out_err;
>>> +               goto destroy_device;
>>>
>>>         rbd_dev->parent = parent;
>>>         atomic_set(&rbd_dev->parent_ref, 1);
>>>         return 0;
>>> -
>>> -out_err:
>>> -       rbd_dev_unparent(rbd_dev);
>>> +destroy_device:
>>>         rbd_dev_destroy(parent);
>>> +unparent_device:
>>> +       rbd_dev_unparent(rbd_dev);
>>>         return ret;
>>>  }
>>
>> Cleanup here is (and should be) done in reverse order.
>
> I have got an other impression about the appropriate order for the corresponding
> clean-up function calls.
>
>
>> We allocate parent rbd_device and then link it with what we already have,
>
> I guess that we have got a different understanding about the relevant "linking".

Well, there isn't any _literal_ linking (e.g. adding to a link list,
etc) in this case.  We just bump some refs and do probe to fill in the
newly allocated parent.  If probe fails, we put refs and free parent,
reversing the "alloc parent, bump refs" order.

The actual linking (rbd_dev->parent = parent) is done right before
returning so we never have to undo it in rbd_dev_probe_parent() and
that's the only reason your patch probably doesn't break anything.
Think about what happens if, after your patch is applied, someone moves
that assignment up or adds an extra step that can fail after it...

>
>
>> so the order in which we cleanup is unlink ("unparent"), destroy.
>
> I interpreted the eventual passing of a null pointer to the rbd_dev_destroy()
> function as an indication for further source code adjustments.

If all error paths could be adjusted so that NULL pointers are never
passed in, destroy functions wouldn't need to have a NULL check, would
they?

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/