Re: [PATCH 2/8] block: add BlockParentClass class

[Vladimir Sementsov-Ogievskiy]

16.09.2021 11:34, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
> > Add a class that will unify block parents for blockdev-replace
> > functionality we are going to add.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   include/block/block-parent.h | 32 +++++++++++++++++
> >   block/block-parent.c         | 66 ++++++++++++++++++++++++++++++++++++
> >   block/meson.build            |  1 +
> >   3 files changed, 99 insertions(+)
> >   create mode 100644 include/block/block-parent.h
> >   create mode 100644 block/block-parent.c
> >
> > diff --git a/include/block/block-parent.h b/include/block/block-parent.h
> > new file mode 100644
> > index 0000000000..bd9c9d91e6
> > --- /dev/null
> > +++ b/include/block/block-parent.h
> > @@ -0,0 +1,32 @@
> > +/*
> > + * Block Parent class
> > + *
> > + * Copyright (c) 2021 Virtuozzo International GmbH.
> > + *
> > + * Authors:
> > + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef BLOCK_PARENT_H
> > +#define BLOCK_PARENT_H
> > +
> > +#include "block/block.h"
> > +
> > +typedef struct BlockParentClass {
> > +    const char *name;
> > +
> > +    int (*find_child)(const char *parent_id, const char *child_name,
> > +                      BlockDriverState *child_bs, BdrvChild **child,
> > +                      Error **errp);
>
> Callbacks should come with a contract.

will add

> > +    QTAILQ_ENTRY(BlockParentClass) next;
> > +} BlockParentClass;
> > +
> > +void block_parent_class_register(BlockParentClass *cls);
> > +
> > +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> > +                            BlockDriverState *child_bs, Error **errp);
> > +
> > +#endif /* BLOCK_PARENT_H */
> > diff --git a/block/block-parent.c b/block/block-parent.c
> > new file mode 100644
> > index 0000000000..73b6026c42
> > --- /dev/null
> > +++ b/block/block-parent.c
> > @@ -0,0 +1,66 @@
> > +/*
> > + * Block Parent class
> > + *
> > + * Copyright (c) 2021 Virtuozzo International GmbH.
> > + *
> > + * Authors:
> > + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "block/block-parent.h"
> > +#include "qapi/error.h"
> > +
> > +static QTAILQ_HEAD(, BlockParentClass) block_parent_classes =
> > +    QTAILQ_HEAD_INITIALIZER(block_parent_classes);
> > +
> > +void block_parent_class_register(BlockParentClass *cls)
> > +{
> > +    QTAILQ_INSERT_HEAD(&block_parent_classes, cls, next);
> > +}
> > +
> > +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> > +                            BlockDriverState *child_bs, Error **errp)
> > +{
> > +    BdrvChild *found_child = NULL;
> > +    BlockParentClass *found_cls = NULL, *cls;
> > +
> > +    QTAILQ_FOREACH(cls, &block_parent_classes, next) {
> > +        int ret;
> > +        BdrvChild *c;
> > +
> > +        /*
> > +         * Note that .find_child must fail if parent is found but doesn't have
> > +         * corresponding child.
> > +         */
> > +        ret = cls->find_child(parent_id, child_name, child_bs, &c, errp);
> > +        if (ret < 0) {
> > +            return NULL;
> > +        }
> > +        if (ret == 0) {
> > +            continue;
> > +        }
> > +
> > +        if (!found_child) {
> > +            found_cls = cls;
> > +            found_child = c;
> > +            continue;
> > +        }
> > +
> > +        error_setg(errp, "{%s, %s} parent-child pair is ambiguous: it match "
> > +                   "both %s and %s", parent_id, child_name, found_cls->name,
> > +                   cls->name);
> > +        return NULL;
> > +    }
> > +
> > +    if (!found_child) {
> > +        error_setg(errp, "{%s, %s} parent-child pair not found", parent_id,
> > +                   child_name);
> > +        return NULL;
> > +    }
> > +
> > +    return found_child;
> > +}
>
> Especially when the callback can return NULL with and without setting an
> error!

Hmm. Callback returns int.

And this block_find_child() function return NULL only together with errp set.

> > diff --git a/block/meson.build b/block/meson.build
> > index 0450914c7a..5200e0ffce 100644
> > --- a/block/meson.build
> > +++ b/block/meson.build
> > @@ -10,6 +10,7 @@ block_ss.add(files(
> >     'blkverify.c',
> >     'block-backend.c',
> >     'block-copy.c',
> > +  'block-parent.c',
> >     'commit.c',
> >     'copy-on-read.c',
> >     'preallocate.c',


--
Best regards,
Vladimir