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

[Martin Wilck <mwilck@suse.com>]

On Thu, 2021-02-04 at 08:16 +0100, Hannes Reinecke wrote:
> 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.

Sagi said the same, I'll drop this. I thought it was acceptable because
I'd be masking a rule from the same package, and preferable because
making the user responsible for it would be cumbersome and error-prone
on the part of the user.

Thanks,
Martin

> 
> Cheers,
> 
> Hannes



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme