fix(multipath): check for all required binaries

[LaszloGombos]

trafficstars

Introduce a new pattern to list required binaries once and use it both at check() and install() time.

I picked the multipath module as multipath module is actively developed, so I hope to get some meaningful feedback on this new pattern before extending it to more modules.

Fixed depends() call.

Removed dmsetup from here as it is already checked in the dm module.

As a result of this PR, if e.g. multipathd is not installed on the host check() would now fail early (instead of install).

The goal is NOT to make check() perfect, but simply make it better. Some tests for the environment will remain only inside the install() function.

CC @mwilck

[mwilck]

Maybe you should distinguish between required and optional binaries? Note that pkill is only necessary in the non-systemd case.

[LaszloGombos]

Maybe you should distinguish between required and optional binaries?

This is one of the motivation, to find and mark more binaries optional if they are in fact optional.

I marked pkill optional.

[mwilck]

The correct way would probably be to make it depend on dracut_module_included "systemd". But I am unsure if this value is already available in the check() stage.

[LaszloGombos]

Thanks for the help @mwilck

The correct way would probably be to make it depend on dracut_module_included "systemd"

Agreed. I kept it inside install() instead of moving it to check() as the main goal is to try to roll out this pattern to all modules, which is quite an ambitions goal and I hope to keep that goal somewhat realistic.

As a side effect, I find that this PR makes the code much more readable and easier to reason about.

[mwilck]

Sorry, I have another question. Why do you need a shell function here? Wouldn't it be sufficient to simply set a variable?

BINARIES="multipath multipathd ..."

[LaszloGombos]

Sorry, I have another question.

Thanks for the review. I like questions.

Why do you need a shell function here? Wouldn't it be sufficient to simply set a variable?

It would be sufficient. I do not have a strong preference on variable vs. function. The reason I choose function is to follow the existing check(), install() pattern. If the approach proposed here is useful enough, than over time I can see binaries() called from https://github.com/dracutdevs/dracut/blob/master/dracut-init.sh#L779 . However even this longer term goal could be achieved with a variable.

I am open to change it based on your or other contributors feedback, as long as the the approach will scale to most dracut modules, not just this one.

[mwilck]

AFAIK, The reason that check() etc are functions is that they are called from the main dracut binary. But binaries() seems to be called only from functions in the same module_setup.sh file.

But I would suggest that @johannbg or some other core developer comment on this.

[LaszloGombos]

But binaries() seems to be called only from functions in the same module_setup.sh file.

I am proposing to consider calling also binaries() from the main dracut binary (optionally, if the function is available). This seems like a common need most modules have.

I would propose to do this in two steps - first step is just to call binaries() from the same module_setup.sh .

But I would suggest that @johannbg or some other core developer comment on this.

Agreed. If the larger idea of calling also binaries() from the main dracut binary does not get traction, I will change the function to a variable for the purpose of this PR.

[stale[bot]]

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

[stale[bot]]

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.