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

Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; 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(a)virtuozzo.com&gt; > --- > 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(a)virtuozzo.com&gt; > + * > + * 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.

> + 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(a)virtuozzo.com&gt; > + * > + * 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!

> 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',