Merge pull request #6573 from grondo/uri-pathlike

libflux: support ancestor paths as alternative to URI in `flux_open(3)` and `flux_open_ex(3)`

Author: mergify[bot]

doc/man3/flux_open.rst

@@ -33,10 +33,18 @@ to communicate with the Flux message broker. :func:`flux_open_ex` takes an
 optional pointer to a :type:`flux_error_t` structure which, when non-NULL, will
 be used to store any errors which may have otherwise gone to :var:`stderr`.
 
-The :var:`uri` scheme (before "://") specifies the "connector" that will be used
-to establish the connection. The :var:`uri` path (after "://") is parsed by the
-connector. If :var:`uri` is NULL, the value of :envvar:`FLUX_URI` is used.  If
-:envvar:`FLUX_URI` is not set, a compiled-in default URI is used.
+When set, the :var:`uri` argument may be a valid native URI, e.g. as
+returned from the :man1:`flux-uri` command or found in the :var:`local-uri`
+and :var:`parent-uri` broker attributes, or a path-like string referencing
+a possible instance ancestor like "/" for the top-level or root instance,
+or ".." for the parent instance. See `ANCESTOR PATHS`_ below.
+
+If :var:`uri` is NULL, the value of :envvar:`FLUX_URI` is used.
+If :envvar:`FLUX_URI` is not set, a compiled-in default URI is used.
+
+When :var:`uri` contains a native URI, the scheme (before "://") specifies
+the "connector" that will be used to establish the connection. The :var:`uri`
+path (after "://") is parsed by the connector.
 
 *flags* is the logical "or" of zero or more of the following flags:
 
@@ -89,10 +97,24 @@ original handle.
 the Flux message broker.
 
 
+ANCESTOR PATHS
+==============
+
+As an alternative to a URI, the :func:`flux_open` and :func:`flux_open_ex`
+functions also take a path-like string indicating that a handle to an ancestor
+instance should be opened. This string follows the following rules:
+
+ - One or more ".." separated by "/" refer to a parent instance
+   (possibly multiple levels up the hierarchy)
+ - "." indicates the current instance
+ - A single slash ("/") indicates the root instance
+ - ".." at the root is not an error, it just returns a handle to the root
+   instance
+
 RETURN VALUE
 ============
 
-:func:`flux_open` and :func:`flux_clone` return a :type:`flux_t`` handle on
+:func:`flux_open` and :func:`flux_clone` return a :type:`flux_t` handle on
 success.  On error, NULL is returned, with :var:`errno` set.
 
 

src/bindings/python/flux/core/handle.py

@@ -32,6 +32,15 @@ class Flux(Wrapper):
     Example:
         >>> flux.Flux()
         <flux.core.Flux object at 0x...>
+
+    Args:
+        uri (str): A fully-qualified native path as returned by
+            :man1:`flux-uri`, or a path-like string that references any
+            ancestor in the current hierarchy, e.g. ``/`` refers to
+            the root instance, ``..`` refers to the parent, ``../..``
+            the parent's parent and so on. See :man3:`flux_open` for more
+            details.
+        flags (int): flags as described in :man3:`flux_open`.
     """
 
     #  Thread local storage to hold a reactor_running boolean, indicating

src/common/libflux/handle.c

@@ -171,9 +171,184 @@ static char *strtrim (char *s, const char *trim)
     return *s ? s : NULL;
 }
 
+/* Return local URI from either FLUX_URI environment variable, or
+ * if that is not set, the builtin config. Caller must free result.
+ */
+static char *get_local_uri (void)
+{
+    const char *uri;
+    char *result = NULL;
+
+    if ((uri = getenv ("FLUX_URI")))
+        return strdup (uri);
+    if (asprintf (&result,
+                  "local://%s/local",
+                  flux_conf_builtin_get ("rundir", FLUX_CONF_INSTALLED)) < 0)
+        result = NULL;
+    return result;
+}
+
+/* Return the current instance level as an integer using the
+ * instance-level broker attribute.
+ */
+static int get_instance_level (flux_t *h)
+{
+    long l;
+    char *endptr;
+    const char *level;
+
+    if (!(level = flux_attr_get (h, "instance-level")))
+        return -1;
+
+    errno = 0;
+    l = strtol (level, &endptr, 10);
+    if (errno != 0 || *endptr != '\0') {
+        errno = EINVAL;
+        return -1;
+    }
+    if (l < 0 || l > INT_MAX) {
+        errno = ERANGE;
+        return -1;
+    }
+    return (int) l;
+}
+
+/* Resolve the URI of an ancestor `n` levels up the hierarchy.
+ * If n is 0, then the current default uri (FLUX_URI or builtin) is returned.
+ * If n >= instance-level, then the URI of the root instance is returned.
+ */
+static char *resolve_ancestor_uri (flux_t *h, int n, flux_error_t *errp)
+{
+    int level;
+    int depth = 0;
+    const char *uri = NULL;
+    char *result = NULL;
+
+    if ((level = get_instance_level (h)) < 0) {
+        errprintf (errp,
+                   "Failed to get instance-level attribute: %s",
+                   strerror (errno));
+        return NULL;
+    }
+
+    /* If n is 0 (resolve current uri) or level is 0 (we're already
+     * in the root instance), return copy of local URI immediately:
+     */
+    if (n == 0 || level == 0) {
+        if (!(result = get_local_uri ())) {
+            errprintf (errp, "Failed to get local URI");
+            return NULL;
+        }
+        return result;
+    }
+
+    /* Resolve to depth 0 unless n is > 0 (but ensure depth not < 0)
+     */
+    if (n > 0 && (depth = level - n) < 0)
+        depth = 0;
+
+    /* Take a reference on h since it will be closed below
+     */
+    flux_incref (h);
+
+    while (!result) {
+        flux_t *parent_h;
+
+        if (!(uri = flux_attr_get (h, "parent-uri"))
+            || !(parent_h = flux_open_ex (uri, 0, errp)))
+            goto out;
+
+        if (--level == depth) {
+            if (!(result = strdup (uri))) {
+                errprintf (errp, "Out of memory");
+                goto out;
+            }
+        }
+        flux_close (h);
+        h = parent_h;
+    }
+out:
+    flux_close (h);
+    return result;
+}
+
+/* Count the number of parent elements ".." separated by one or more "/"
+ * in `path`. It is an error if anything but ".." or "." appears in each
+ * element ("." is ignored and indicates "current instance").
+ *
+ * `path` should have already been checked for a leading slash (an error).
+ */
+static int count_parents (const char *path, flux_error_t *errp)
+{
+    char *copy;
+    char *str;
+    char *s;
+    char *sp = NULL;
+    int n = 0;
+    int rc = -1;
+
+    if (!(copy = strdup (path))) {
+        errprintf (errp, "Out of memory");
+        return -1;
+    }
+    str = copy;
+    while ((s = strtok_r (str, "/", &sp))) {
+        if (streq (s, ".."))
+            n++;
+        else if (!streq (s, ".")) {
+            errprintf (errp, "%s: invalid URI path element '%s'", path, s);
+            errno = EINVAL;
+            goto out;
+        }
+        str = NULL;
+    }
+    rc = n;
+out:
+    ERRNO_SAFE_WRAP (free, copy);
+    return rc;
+}
+
+/* Resolve a path-like string where one or more ".." separated by "/" specify
+ * to resolve parent instance URIs (possibly multiple levels up),  "."
+ * indicates the current instance, and a single slash ("/") specifies the
+ * root instance URI.
+ */
+static char *resolve_path_uri (const char *path, flux_error_t *errp)
+{
+    char *uri = NULL;
+    int nparents;
+    flux_t *h;
+
+    /* Always start from current enclosing instance
+     */
+    if (!(h = flux_open_ex (NULL, 0, errp)))
+        return NULL;
+
+    if (path[0] == '/') {
+        /* Leading slash only allowed if it is the only character in the
+        * uri path string:
+        */
+        if (!streq (path, "/")) {
+            errprintf (errp, "%s: invalid URI", path);
+            errno = EINVAL;
+            goto out;
+        }
+        /* nparents < 0 means resolve to root
+         */
+        nparents = -1;
+    }
+    else if ((nparents = count_parents (path, errp)) < 0)
+        goto out;
+
+    uri = resolve_ancestor_uri (h, nparents, errp);
+out:
+    flux_close (h);
+    return uri;
+}
+
 flux_t *flux_open_ex (const char *uri, int flags, flux_error_t *errp)
 {
-    char *default_uri = NULL;
+    char *tmp = NULL;
     char *path = NULL;
     char *scheme = NULL;
     void *dso = NULL;
@@ -197,15 +372,20 @@ flux_t *flux_open_ex (const char *uri, int flags, flux_error_t *errp)
 
     /* Try to get URI from (in descending precedence):
      *   argument > environment > builtin
+     *
+     * If supplied argument starts with "." or "/", then treat it as
+     * a path-like argument which may reference an ancestor (as processed
+     * by resolve_path_uri())
      */
-    if (!uri)
-        uri = getenv ("FLUX_URI");
     if (!uri) {
-        if (asprintf (&default_uri, "local://%s/local",
-                      flux_conf_builtin_get ("rundir",
-                                             FLUX_CONF_INSTALLED)) < 0)
+        if (!(tmp = get_local_uri ()))
+            goto error;
+        uri = tmp;
+    }
+    else if (uri[0] == '.' || uri[0] == '/') {
+        if (!(tmp = resolve_path_uri (uri, errp)))
             goto error;
-        uri = default_uri;
+        uri = tmp;
     }
     if (!(scheme = strdup (uri)))
         goto error;
@@ -244,13 +424,13 @@ flux_t *flux_open_ex (const char *uri, int flags, flux_error_t *errp)
             goto error_handle;
     }
     free (scheme);
-    free (default_uri);
+    free (tmp);
     return h;
 error_handle:
     flux_handle_destroy (h);
 error:
     ERRNO_SAFE_WRAP (free, scheme);
-    ERRNO_SAFE_WRAP (free, default_uri);
+    ERRNO_SAFE_WRAP (free, tmp);
     /*
      *  Fill errp->text with strerror only when a more specific error has not
      *   already been set.

t/t3100-flux-in-flux.t

@@ -54,7 +54,7 @@ test_expect_success "flux --root works in subinstance" '
 	id=$(flux batch -n1 --wrap \
 		flux run flux start ${ARGS} \
 		flux --root getattr instance-level) &&
-	flux job wait-event -vt 30 $id clean &&
+	flux job wait-event -vt 60 $id clean &&
 	test_debug "cat flux-${id}.out" &&
 	test "$(cat flux-${id}.out)" -eq 0
 '
@@ -106,4 +106,47 @@ test_expect_success 'flux can launch multiple brokers per node (R lookup fallbac
 		--conf=resource.no-update-watch=true \
 		flux resource info
 '
+test_expect_success 'flux_open("/") works at top-level' '
+	flux python -c "import flux; print(flux.Flux(\"/\").attr_get(\"instance-level\"));"
+'
+test_expect_success 'flux_open(3) accepts path-like URIs: "/", "../.." etc' '
+	cat <<-EOF >flux_open.py &&
+	import unittest
+	import flux
+	import sys
+	from collections import namedtuple
+
+	Test = namedtuple("Test", ["arg", "level"])
+	tests = [
+	    Test(None, 3),
+	    Test(".", 3),
+	    Test("./", 3),
+	    Test("..", 2),
+	    Test("../", 2),
+	    Test("./..", 2),
+	    Test("../..", 1),
+	    Test("..//..", 1),
+	    Test("../../", 1),
+	    Test(".././..", 1),
+	    Test("../../..", 0),
+	    Test("../../../..", 0),
+	    Test("../../../../../../../", 0),
+	    Test("/", 0),
+	]
+
+	class TestFluxOpen(unittest.TestCase):
+	    def test_pathlike_uris(self):
+	        for test in tests:
+	            l = flux.Flux(test.arg).attr_get("instance-level")
+	            self.assertEqual(int(l), test.level)
+
+	    def test_pathlike_bad_arg(self):
+	        for arg in ("/.", "//", "../f", "../f/.."):
+	            with self.assertRaises(OSError, msg=f"arg={arg}"):
+	                h = flux.Flux(arg)
+	                print(h.attr_get("instance-level"))
+	unittest.main(testRunner=unittest.TextTestRunner(verbosity=2))
+	EOF
+	flux alloc -n1 flux alloc -n1 flux alloc -n1 flux python ./flux_open.py
+'
 test_done