On Tue, 2024-12-03 at 13:40 +0100, Martin Wilck wrote:
> On Mon, 2024-12-02 at 19:17 -0500, Benjamin Marzinski wrote:
> > On Thu, Nov 28, 2024 at 12:04:27AM +0100, Martin Wilck wrote:
> > >
> > > @@ -763,11 +795,7 @@ void select_action (struct multipath *mpp,
> > > const struct vector_s *curmp,
> > > select_reload_action(mpp, "minio change");
> > > return;
> > > }
> > > - if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) !=
> > > VECTOR_SIZE(mpp-
> > > > pg)) {
> > > - select_reload_action(mpp, "path group number
> > > change");
> > > - return;
> > > - }
> > > - if (pgcmp(mpp, cmpp)) {
> > > + if (!cmpp->pg || pgcmp(mpp, cmpp)) {
> >
> > I know that the code did this before, but is there a reason why we
> > always reload if cmpp->pg is NULL? I'm pretty sure it's possible to
> > call
> > select action when neither mpp nor cmpp have any paths. (cli_reload
> > on a
> > pathless device should do that). In this case, the topology isn't
> > changing, and AFAICS pgcmp() should work with no pathgroups.
>
>
> It dates back to b0991a8 ("Allow zero paths for multipath map"),
> multipath-tools 0.4.9.
>
> I guess back then Hannes either encountered in crash in some
> situation,
> or he was just being cautious.
>
> I can remove this check.
As all our vector macros handle the (V==NULL) case, we can rely on them
doing the "right thing" if we just check VECTOR_SIZE(). While I kind of
dislike these hidden checks (other code bases like systemd use assert()
instead, which probably helps finding bugs), we rely on this behavior
basically everywhere in our code, so why not here.
Regards
Martin
