Re: [PATCH 13/35] monitor: disable nvmf-autoconnect udev rules in autoconnect mode

Hannes Reinecke
2021-02-04T07:16:35Z

On 1/26/21 9:33 PM, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> If autoconnect is enabled, disable the respective udev rules
> by symlinking /run/udev/rules.d to /dev/null, in order to avoid
> the connections being set up by the monitor and the udev workers
> at the same time. This is probably the preferred mode of operation
> for the monitor.
>
> Users can override this by copying 70-nvmf-autoconnect.rules
> from /usr/lib/udev/rules.d to /etc/udev/rules.d (/etc/udev/rules.d
> takes precedence over /run/udev/rules.d).
>
> If the symlink can't be created for some reason, autoconnect will
> be disabled. There is only one exception: If
> /run/udev/rules.d/70-nvmf-autoconnect.rules already points to
> /dev/null at startup, autoconnect can be left on, but the symlink
> isn't removed on exit.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> monitor.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index ecf3be2..2a906db 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -17,14 +17,18 @@
>
> #include <stddef.h>
> #include <stdio.h>
> +#include <stdlib.h>
> #include <unistd.h>
> #include <errno.h>
> #include <libudev.h>
> #include <signal.h>
> #include <time.h>
> +#include <limits.h>
> #include <syslog.h>
> +#include <sys/stat.h>
> #include <sys/epoll.h>
>
> +#include "common.h"
> #include "nvme-status.h"
> #include "util/argconfig.h"
> #include "monitor.h"
> @@ -33,6 +37,7 @@
>
> static struct monitor_config {
> bool autoconnect;
> + bool skip_udev_on_exit;
> } mon_cfg;
>
> static struct udev *udev;
> @@ -45,6 +50,8 @@ static void close_ptr(int *p)
> }
> }
>
> +CLEANUP_FUNC(char)
> +
> static void cleanup_monitor(struct udev_monitor **pmon)
> {
> if (*pmon) {
> @@ -174,12 +181,64 @@ static int monitor_main_loop(struct udev_monitor *monitor)
> return ret;
> }
>
> +static const char autoconnect_rules[] = "/run/udev/rules.d/70-nvmf-autoconnect.rules";
> +
> +static int monitor_disable_udev_rules(void)
> +{
> + CLEANUP(char, path) = strdup(autoconnect_rules);
> + char *s1, *s2;
> + int rc;
> +
> + if (!path)
> + return -ENOMEM;
> +
> + s2 = strrchr(path, '/');
> + for (s1 = s2 - 1; s1 > path && *s1 != '/'; s1--);
> +
> + *s2 = *s1 = '\0';
> + rc = mkdir(path, 0755);
> + if (rc == 0 || errno == EEXIST) {
> + *s1 = '/';
> + rc = mkdir(path, 0755);
> + if (rc == 0 || errno == EEXIST) {
> + *s2 = '/';
> + rc = symlink("/dev/null", path);
> + }
> + }
> + if (rc) {
> + if (errno == EEXIST) {
> + char target[PATH_MAX];
> +
> + if (readlink(path, target, sizeof(target)) != -1 &&
> + !strcmp(target, "/dev/null")) {
> + log(LOG_INFO,
> + "symlink %s -> /dev/null exists already\n",
> + autoconnect_rules);
> + return 1;
> + }
> + }
> + log(LOG_ERR, "error creating %s: %m\n", autoconnect_rules);
> + } else
> + log(LOG_INFO, "created %s\n", autoconnect_rules);
> +
> + return rc ? (errno ? -errno : -EIO) : 0;
> +}
> +
> +static void monitor_enable_udev_rules(void)
> +{
> + if (unlink(autoconnect_rules) == -1 && errno != ENOENT)
> + log(LOG_ERR, "error removing %s: %m\n", autoconnect_rules);
> + else
> + log(LOG_INFO, "removed %s\n", autoconnect_rules);
> +}
> +
> static int monitor_parse_opts(const char *desc, int argc, char **argv)
> {
> bool quiet = false;
> bool verbose = false;
> bool debug = false;
> - int ret;
> + int ret = 0;
> +
> OPT_ARGS(opts) = {
> OPT_FLAG("autoconnect", 'A', &mon_cfg.autoconnect, "automatically connect newly discovered controllers"),
> OPT_FLAG("silent", 'S', &quiet, "log level: silent"),
> @@ -198,7 +257,17 @@ static int monitor_parse_opts(const char *desc, int argc, char **argv)
> log_level = LOG_INFO;
> if (debug)
> log_level = LOG_DEBUG;
> -
> + if (mon_cfg.autoconnect) {
> + ret = monitor_disable_udev_rules();
> + if (ret < 0) {
> + mon_cfg.autoconnect = false;
> + log(LOG_WARNING, "autoconnect disabled\n");
> + ret = 0;
> + } else if (ret > 0) {
> + mon_cfg.skip_udev_on_exit = true;
> + ret = 0;
> + }
> + }
> return ret;
> }
>
> @@ -221,6 +290,8 @@ int aen_monitor(const char *desc, int argc, char **argv)
> udev_monitor_unref(monitor);
> }
> udev = udev_unref(udev);
> + if (mon_cfg.autoconnect && !mon_cfg.skip_udev_on_exit)
> + monitor_enable_udev_rules();
> out:
> return nvme_status_to_errno(ret, true);
> }
>
What on earth ...

No way.
We should _not_ reference individual files or mess with udev rules from
a program. If a file needs to be modified, modify it.
But do not try to modify a file from a program context, unless the
program itself has created it.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer