Re: patch - cgdconfig do_all mode gracefully ignore detached devices

[Jason High <json.high%gmail.com@localhost>]

Thanks for the quick chat.  Per suggestions, I'm using open rather
than opendisk.  Ok?

Index: cgdconfig.c
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/cgdconfig.c,v
retrieving revision 1.50
diff -b -u -r1.50 cgdconfig.c
--- cgdconfig.c    10 Apr 2019 06:11:37 -0000    1.50
+++ cgdconfig.c    10 Nov 2019 15:56:40 -0000
@@ -125,6 +125,7 @@
 static int     verify_reenter(struct params *);
 static int     verify_mbr(int);
 static int     verify_gpt(int);
+static int     verify_attached(const char * opath);

 __dead static void     usage(void);

@@ -540,6 +541,19 @@
         return -1;
     }

+    /* if called from do_all, then ignore if device is not attached */
+    if (flags == CONFIG_FLAGS_FROMALL) {
+
+        ret = verify_attached(argv[1]);
+
+        /* device unattached */
+        if (ret == 0) {
+            VPRINTF(1, ("skipping %s: device is not attached\n", argv[1]));
+            return 0;
+        }
+        /* pass-through success and errors */
+    }
+
     if ((
       fd = opendisk1(*argv, O_RDWR, cgdname, sizeof(cgdname), 1, prog_open)
         ) != -1) {
@@ -1282,3 +1296,19 @@
     if (setrlimit(RLIMIT_CORE, &rlp) == -1)
         err(EXIT_FAILURE, "Can't disable cores");
 }
+
+static int
+verify_attached(const char * opath)
+{
+    int fd;
+
+    if (fd = open(opath,O_RDWR) == -1) {
+        if (errno == ENXIO) {
+            return 0;
+        }
+        return -1;
+    }
+    close(fd);
+
+    return 1;
+}

On Sun, Nov 10, 2019 at 9:37 AM Jason High <json.high%gmail.com@localhost> wrote:
>
> Thanks for your feedback!  We cannot fail silently in configure as-is
> due to the config/verification flow.  However, if what was meant was
> to check for ENXIO in the verification function, then that works, and
> is a much simpler solution.  In the below diff, we removed the check
> on -U per the issues raised by Robert.  For -C, we simply try to open
> the device path.  On error, if we get ENXIO we return 0.
> Otherwise we let -1 pass through to be handled as expected in the
> flow.  Is this any closer to your liking?  Tested and working as
> expected.  Thanks so much for your review
>
> Index: cgdconfig.c
> ===================================================================
> RCS file: /cvsroot/src/sbin/cgdconfig/cgdconfig.c,v
> retrieving revision 1.50
> diff -b -u -r1.50 cgdconfig.c
> --- cgdconfig.c    10 Apr 2019 06:11:37 -0000    1.50
> +++ cgdconfig.c    10 Nov 2019 15:09:33 -0000
> @@ -125,6 +125,7 @@
>  static int     verify_reenter(struct params *);
>  static int     verify_mbr(int);
>  static int     verify_gpt(int);
> +static int     verify_attached(const char * opath);
>
>  __dead static void     usage(void);
>
> @@ -540,6 +541,19 @@
>          return -1;
>      }
>
> +    /* if called from do_all, then ignore if device is not attached */
> +    if (flags == CONFIG_FLAGS_FROMALL) {
> +
> +        ret = verify_attached(argv[1]);
> +
> +        /* device unattached */
> +        if (ret == 0) {
> +            VPRINTF(1, ("skipping %s: device is not attached\n", argv[1]));
> +            return 0;
> +        }
> +        /* pass-through success and errors */
> +    }
> +
>      if ((
>        fd = opendisk1(*argv, O_RDWR, cgdname, sizeof(cgdname), 1, prog_open)
>          ) != -1) {
> @@ -1282,3 +1296,25 @@
>      if (setrlimit(RLIMIT_CORE, &rlp) == -1)
>          err(EXIT_FAILURE, "Can't disable cores");
>  }
> +
> +static int
> +verify_attached(const char * opath)
> + check if opath is accessible
> +{
> +    int fd;
> +    char path[MAXPATHLEN];
> +
> +    fd = opendisk(opath,O_RDWR,path,sizeof(path),0);
> +
> +    if (fd == -1) {
> +        if (errno == ENXIO) {
> +            return 0;
> +        }
> +        return -1;
> +    }
> +
> +    close(fd);
> +
> +    return 1;
> +}
>
> On Sun, Nov 10, 2019 at 1:42 AM Michael van Elst <mlelstv%serpens.de@localhost> wrote:
> >
> > On Sun, Nov 10, 2019 at 01:21:21AM -0600, Jason High wrote:
> > > For -U, that could work (was actually how we handled it in an earlier
> > > version).  For -C, no.  Our goal is to avoid cgd trying to configure a
> > > device, like those backed by USB drives, on boot when the backing
> > > device isn't attached.
> >
> > Why? Whether you rely on magic (== non-standard interfaces with
> > unspecified semantics) to skip the configuration or the configuration
> > just fails silently shouldn't make a difference.
> >
> >
> > Greetings,
> > --
> >                                 Michael van Elst
> > Internet: mlelstv%serpens.de@localhost
> >                                 "A potential Snark may lurk in every tree."