Hi Ben,
On Mon, 2018-02-12 at 17:18 -0600, Benjamin Marzinski wrote:
> On Sat, Feb 10, 2018 at 08:55:53PM +0100, Martin Wilck wrote:
> > Hi Ben,
> >
> > thanks a lot for this. I have only a few minor nitpicks (see
> > below).
> > I suppose you've tested this already?
>
> Yes. I do plan on doing some more testing after I look into making
> libdevmapper support re-arming the polling interface and grabbing the
> event number from the names listing, before I repost this without the
> RFC tag. I was also thinking of trying out cmocka by mocking up a
> device-mapper interface that let me test this code in isolation.
Great idea.
Am I understanding correctly that you are working on libdevmapper in
parallel? If yes, would it make sense to have libmultipath use the
newly developed libdevmapper API right away, rather than using a
custom-made ioctl interface until libdevmapper is ready?
> > > I haven't touched any of the existing event waiting code, since
> > > event
> > > polling was only added to device-mapper in version
> > > 4.37.0. multipathd
> > > checks this version, and defaults to using the polling code if
> > > device-mapper supports it. This can be overridden by running
> > > multipathd
> > > with "-w", to force it to use the old event waiting code.
> >
> > Why use a command line option here rather than a config file
> > option?
>
> Mostly because it was faster, and I wanted to get to testing it. The
> other reason is that I don't see any benefit for the work involved in
> making this be changeable in
>
> # multipathd reconfigure
>
> However, we already have configuration settings that can't get
> changed
> on reconfigure, so making this another one is not a big deal. I agree
> that it is easier for users to change if it is a configuration
> setting,
> but I'm hoping that this change will be invisible to users. If you
> would
> prefer it as a configuration setting, I have no problem with changing
> that
> .
Right. It doesn't need to be user-configurable. We may want to leave a
compile-time option to disable it for the time being.
> > >
> > > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > > ---
> > > multipathd/Makefile | 3 +-
> > > multipathd/dmevents.c | 396
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > multipathd/dmevents.h | 13 ++
> > > multipathd/main.c | 58 +++++++-
> > > 4 files changed, 461 insertions(+), 9 deletions(-)
> > > create mode 100644 multipathd/dmevents.c
> > > create mode 100644 multipathd/dmevents.h
> > >
> > > diff --git a/multipathd/Makefile b/multipathd/Makefile
> > > index 85f29a7..4c438f0 100644
> > > --- a/multipathd/Makefile
> > > +++ b/multipathd/Makefile
> > > @@ -22,7 +22,8 @@ ifdef SYSTEMD
> > > endif
> > > endif
> > >
> > > -OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o
> > > waiter.o
> > > +OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o
> > > waiter.o \
> > > + dmevents.o
> > >
> > > EXEC = multipathd
> > >
> > > diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
> > > new file mode 100644
> > > index 0000000..a56c055
> > > --- /dev/null
> > > +++ b/multipathd/dmevents.c
> > >
> > > +
> > > +
> > > +int alloc_dmevent_waiter(struct vectors *vecs)
> > > +{
> > > + if (!vecs) {
> > >
> > Nitpick: conventionally, an "alloc"-type function would return the
> > pointer, and NULL on failure.
>
> Is this a naming complaint, or an interface complaint? I'm fine with
> changing the names so they follow the lead of checkers and prio, i.e.
> init_dmevents_waiter() and cleanup_dmevents_waiter(). The init and
> cleanup functions for checkers and prio have the same returns as the
> dmevent functions (well the init functions return 1 for failure, and
> I
> can do that as well)
I'm fine with simply changing the names.
Regards
Martin
--
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
