[PATCH v3 0/3]

[PATCH v3 0/3]

[dje--- via]

Add support for ipv6 host forwarding

This patchset takes the original patch from Maxim,
https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
and updates it.

New option: -ipv6-hostfwd

New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove

These are the ipv6 equivalents of their ipv4 counterparts.

The libslirp part of the patch has been committed upstream,
and will require adding it to qemu.
https://gitlab.freedesktop.org/slirp/libslirp/-/commit/0624fbe7d39b5433d7084a5096d1effc1df74e39
[plus the subsequent merge commit]

Changes from v2:
- split out libslirp commit
- clarify spelling of ipv6 addresses in docs
- tighten parsing of ipv6 addresses

Change from v1:
- libslirp part is now upstream
- net/slirp.c changes split into two pieces (refactor, add ipv6)
- added docs

Doug Evans (3):
  slirp: Placeholder for libslirp ipv6 hostfwd support
  net/slirp.c: Refactor address parsing
  net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

 hmp-commands.hx     |  32 +++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 310 +++++++++++++++++++++++++++++++++++---------
 qapi/net.json       |   4 +
 slirp               |   2 +-
 5 files changed, 285 insertions(+), 65 deletions(-)

-- 
2.30.0.365.g02bc693789-goog

[PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[dje--- via]

This commit is intended to only contain the slirp submodule change
that adds ipv6 hostfwd support.
---
 slirp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp b/slirp
index 8f43a99191..358c0827d4 160000
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
+Subproject commit 358c0827d49778f016312bfb4167fe639900681f
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[Eric Blake]

On 2/3/21 5:35 PM, dje--- via wrote:
> This commit is intended to only contain the slirp submodule change
> that adds ipv6 hostfwd support.


Missing your signed-off-by, and as written, your authorship would be
based on the From: dje--- via <qemu-devel@nongnu.org> header (that looks
like our mailing list rewrote things due to SPF policies, but that it
botched your name in the process), rather than your Reply-to: Doug Evans
<dje@google.com> header.  To fix the latter, you can convince git to
include a From: line in the first line of the body that will override
whatever is in the headers even after mailing list rewrites.

> ---
>  slirp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp b/slirp
> index 8f43a99191..358c0827d4 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Thu, Feb 4, 2021 at 8:02 AM Eric Blake <eblake@redhat.com> wrote:

> On 2/3/21 5:35 PM, dje--- via wrote:
> > This commit is intended to only contain the slirp submodule change
> > that adds ipv6 hostfwd support.
>
>
> Missing your signed-off-by, and as written, your authorship would be
> based on the From: dje--- via <qemu-devel@nongnu.org> header (that looks
> like our mailing list rewrote things due to SPF policies, but that it
> botched your name in the process), rather than your Reply-to: Doug Evans
> <dje@google.com> header.  To fix the latter, you can convince git to
> include a From: line in the first line of the body that will override
> whatever is in the headers even after mailing list rewrites.
>
> > ---
> >  slirp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/slirp b/slirp
> > index 8f43a99191..358c0827d4 160000
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> > +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> >


Samuel, how do merges from the libslirp tree into the qemu tree work?
I wasn't expecting there was anything more I would do, but please correct
me if I'm wrong.

Therefore, what this patch (1/3) is is just a placeholder to solve the
problem of removing the "subproject comment" out of the other patches.
The signed-off-by line is intentionally missing.

[-- Attachment #2: Type: text/html, Size: 2292 bytes --]

Re: [PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[Marc-André Lureau]

Hi,

On Thu, Feb 4, 2021 at 3:38 AM dje--- via <qemu-devel@nongnu.org> wrote:
>
> This commit is intended to only contain the slirp submodule change
> that adds ipv6 hostfwd support.
> ---
>  slirp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

It's generally recommended to include the shortlog of some sort in the
commit message of what changed between the two commits, ex:
https://patchew.org/QEMU/20210125073427.3970606-1-marcandre.lureau@redhat.com/20210125073427.3970606-2-marcandre.lureau@redhat.com/

thanks

> diff --git a/slirp b/slirp
> index 8f43a99191..358c0827d4 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> --
> 2.30.0.365.g02bc693789-goog
>
>


-- 
Marc-André Lureau

[PATCH v3 2/3] net/slirp.c: Refactor address parsing

[dje--- via]

... in preparation for adding ipv6 host forwarding support.

Signed-off-by: Doug Evans <dje@google.com>
---
 net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 129 insertions(+), 71 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index be914c0be0..a21a313302 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -631,15 +631,83 @@ static SlirpState *slirp_lookup(Monitor *mon, const char *id)
     }
 }
 
-void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
+/*
+ * Parse a protocol name of the form "name<sep>".
+ * Valid protocols are "tcp" and "udp". An empty string means "tcp".
+ * Returns a pointer to the end of the parsed string on success, and stores
+ * the result in *is_udp.
+ * Otherwise returns NULL and stores the error message in *errmsg, which must
+ * be freed by the caller.
+ */
+static const char *parse_protocol(const char *str, int sep, int *is_udp,
+                                  char **errmsg)
+{
+    char buf[10];
+    const char *p = str;
+
+    if (get_str_sep(buf, sizeof(buf), &p, sep) < 0) {
+        *errmsg = g_strdup("Missing protcol name separator");
+        return NULL;
+    }
+
+    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
+        *is_udp = 0;
+    } else if (!strcmp(buf, "udp")) {
+        *is_udp = 1;
+    } else {
+        *errmsg = g_strdup("Bad protcol name");
+        return NULL;
+    }
+
+    return p;
+}
+
+/*
+ * Parse an ipv4 address/port of the form "addr<addr_sep>port<port_sep>".
+ * "kind" is either "host" or "guest" and is included in error messages.
+ * An empty address means INADDR_ANY.
+ * Returns a pointer to the end of the parsed string on success, and stores
+ * the results in *addr, *port.
+ * Otherwise returns NULL and stores the error message in *errmsg, which must
+ * be freed by the caller.
+ */
+static const char *parse_in4_addr_port(const char *str, const char *kind,
+                                       int addr_sep, int port_sep,
+                                       struct in_addr *addr, int *port,
+                                       char **errmsg)
 {
-    struct in_addr host_addr = { .s_addr = INADDR_ANY };
-    int host_port;
     char buf[256];
-    const char *src_str, *p;
+    const char *p = str;
+
+    if (get_str_sep(buf, sizeof(buf), &p, addr_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s address separator", kind);
+        return NULL;
+    }
+    if (buf[0] == '\0') {
+        addr->s_addr = INADDR_ANY;
+    } else if (!inet_aton(buf, addr)) {
+        *errmsg = g_strdup_printf("Bad %s address", kind);
+        return NULL;
+    }
+
+    if (get_str_sep(buf, sizeof(buf), &p, port_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s port separator", kind);
+        return NULL;
+    }
+    if (qemu_strtoi(buf, NULL, 10, port) < 0 ||
+        *port < 0 || *port > 65535) {
+        *errmsg = g_strdup_printf("Bad %s port", kind);
+        return NULL;
+    }
+
+    return p;
+}
+
+static void hmp_hostfwd_remove_worker(Monitor *mon, const QDict *qdict,
+                                      int family)
+{
+    const char *src_str;
     SlirpState *s;
-    int is_udp = 0;
-    int err;
     const char *arg1 = qdict_get_str(qdict, "arg1");
     const char *arg2 = qdict_get_try_str(qdict, "arg2");
 
@@ -654,38 +722,42 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    p = src_str;
-    if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        goto fail_syntax;
-    }
+    int host_port;
+    int is_udp;
+    char *errmsg = NULL;
+    int err;
 
-    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
-        is_udp = 0;
-    } else if (!strcmp(buf, "udp")) {
-        is_udp = 1;
-    } else {
-        goto fail_syntax;
-    }
+    g_assert(src_str != NULL);
+    const char *p = src_str;
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        goto fail_syntax;
-    }
-    if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
+    p = parse_protocol(p, ':', &is_udp, &errmsg);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (qemu_strtoi(p, NULL, 10, &host_port)) {
-        goto fail_syntax;
+    if (family == AF_INET) {
+        struct in_addr host_addr;
+        if (parse_in4_addr_port(p, "host", ':', '\0', &host_addr, &host_port,
+                                &errmsg) == NULL) {
+            goto fail_syntax;
+        }
+        err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
+    } else {
+        g_assert_not_reached();
     }
 
-    err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
-
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
                    err ? "not found" : "removed");
     return;
 
  fail_syntax:
-    monitor_printf(mon, "invalid format\n");
+    monitor_printf(mon, "Invalid format: %s\n", errmsg);
+    g_free(errmsg);
+}
+
+void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_remove_worker(mon, qdict, AF_INET);
 }
 
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
@@ -694,56 +766,29 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
     struct in_addr guest_addr = { .s_addr = 0 };
     int host_port, guest_port;
     const char *p;
-    char buf[256];
     int is_udp;
-    char *end;
-    const char *fail_reason = "Unknown reason";
+    char *errmsg = NULL;
 
+    g_assert(redir_str != NULL);
     p = redir_str;
-    if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "No : separators";
-        goto fail_syntax;
-    }
-    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
-        is_udp = 0;
-    } else if (!strcmp(buf, "udp")) {
-        is_udp = 1;
-    } else {
-        fail_reason = "Bad protocol name";
-        goto fail_syntax;
-    }
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "Missing : separator";
-        goto fail_syntax;
-    }
-    if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
-        fail_reason = "Bad host address";
+    p = parse_protocol(p, ':', &is_udp, &errmsg);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (get_str_sep(buf, sizeof(buf), &p, '-') < 0) {
-        fail_reason = "Bad host port separator";
-        goto fail_syntax;
-    }
-    host_port = strtol(buf, &end, 0);
-    if (*end != '\0' || host_port < 0 || host_port > 65535) {
-        fail_reason = "Bad host port";
+    p = parse_in4_addr_port(p, "host", ':', '-', &host_addr, &host_port,
+                            &errmsg);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "Missing guest address";
+    if (parse_in4_addr_port(p, "guest", ':', '\0', &guest_addr, &guest_port,
+                            &errmsg) == NULL) {
         goto fail_syntax;
     }
-    if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) {
-        fail_reason = "Bad guest address";
-        goto fail_syntax;
-    }
-
-    guest_port = strtol(p, &end, 0);
-    if (*end != '\0' || guest_port < 1 || guest_port > 65535) {
-        fail_reason = "Bad guest port";
+    if (guest_port == 0) {
+        errmsg = g_strdup("Bad guest port");
         goto fail_syntax;
     }
 
@@ -757,11 +802,12 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 
  fail_syntax:
     error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
-               fail_reason);
+               errmsg);
+    g_free(errmsg);
     return -1;
 }
 
-void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
+static void hmp_hostfwd_add_worker(Monitor *mon, const QDict *qdict, int family)
 {
     const char *redir_str;
     SlirpState *s;
@@ -775,13 +821,25 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
         s = slirp_lookup(mon, NULL);
         redir_str = arg1;
     }
-    if (s) {
-        Error *err = NULL;
-        if (slirp_hostfwd(s, redir_str, &err) < 0) {
-            error_report_err(err);
-        }
+    if (!s) {
+        return;
     }
 
+    Error *err = NULL;
+    int rc;
+    if (family == AF_INET) {
+        rc = slirp_hostfwd(s, redir_str, &err);
+    } else {
+        g_assert_not_reached();
+    }
+    if (rc < 0) {
+        error_report_err(err);
+    }
+}
+
+void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_add_worker(mon, qdict, AF_INET);
 }
 
 #ifndef _WIN32
-- 
2.30.0.365.g02bc693789-goog

[PATCH v3 3/3] net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

[dje--- via]

These are identical to their ipv4 counterparts, but for ipv6.

Signed-off-by: Doug Evans <dje@google.com>
---
 hmp-commands.hx     |  32 +++++++++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 128 +++++++++++++++++++++++++++++++++++++++++++-
 qapi/net.json       |   4 ++
 4 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..6a9ec0361d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1392,6 +1392,38 @@ SRST
   Remove host-to-guest TCP or UDP redirection.
 ERST
 
+#ifdef CONFIG_SLIRP
+    {
+        .name       = "ipv6_hostfwd_add",
+        .args_type  = "arg1:s,arg2:s?",
+        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport-[guestaddr6]:guestport",
+        .help       = "redirect TCP6 or UDP6 connections from host to guest (requires -net user)",
+        .cmd        = hmp_ipv6_hostfwd_add,
+    },
+#endif
+SRST
+``ipv6_hostfwd_add``
+  Redirect TCP6 or UDP6 connections from host to guest (requires -net user).
+  Note that IPV6 addresses must be wrapped in square brackets [].
+  E.g., write "[::1]" not "::1".
+ERST
+
+#ifdef CONFIG_SLIRP
+    {
+        .name       = "ipv6_hostfwd_remove",
+        .args_type  = "arg1:s,arg2:s?",
+        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport",
+        .help       = "remove host-to-guest TCP6 or UDP6 redirection",
+        .cmd        = hmp_ipv6_hostfwd_remove,
+    },
+#endif
+SRST
+``ipv6_hostfwd_remove``
+  Remove host-to-guest TCP6 or UDP6 redirection.
+  Note that IPV6 addresses must be wrapped in square brackets [].
+  E.g., write "[::1]" not "::1".
+ERST
+
     {
         .name       = "balloon",
         .args_type  = "value:M",
diff --git a/include/net/slirp.h b/include/net/slirp.h
index bad3e1e241..4796a5cd39 100644
--- a/include/net/slirp.h
+++ b/include/net/slirp.h
@@ -29,6 +29,8 @@
 
 void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
+void hmp_ipv6_hostfwd_add(Monitor *mon, const QDict *qdict);
+void hmp_ipv6_hostfwd_remove(Monitor *mon, const QDict *qdict);
 
 void hmp_info_usernet(Monitor *mon, const QDict *qdict);
 
diff --git a/net/slirp.c b/net/slirp.c
index a21a313302..2a59c20286 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -70,6 +70,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
 /* slirp network adapter */
 
 #define SLIRP_CFG_HOSTFWD 1
+#define SLIRP_CFG_IPV6_HOSTFWD 2
 
 struct slirp_config_str {
     struct slirp_config_str *next;
@@ -101,6 +102,8 @@ static QTAILQ_HEAD(, SlirpState) slirp_stacks =
     QTAILQ_HEAD_INITIALIZER(slirp_stacks);
 
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp);
+static int slirp_ipv6_hostfwd(SlirpState *s, const char *redir_str,
+                              Error **errp);
 static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp);
 
 #ifndef _WIN32
@@ -586,6 +589,10 @@ static int net_slirp_init(NetClientState *peer, const char *model,
             if (slirp_hostfwd(s, config->str, errp) < 0) {
                 goto error;
             }
+        } else if (config->flags & SLIRP_CFG_IPV6_HOSTFWD) {
+            if (slirp_ipv6_hostfwd(s, config->str, errp) < 0) {
+                goto error;
+            }
         } else {
             if (slirp_guestfwd(s, config->str, errp) < 0) {
                 goto error;
@@ -703,6 +710,58 @@ static const char *parse_in4_addr_port(const char *str, const char *kind,
     return p;
 }
 
+/*
+ * Parse an ipv6 address/port of the form "addr<addr_sep>port<port_sep>".
+ * "kind" is either "host" or "guest" and is included in error messages.
+ * An empty address means in6addr_any.
+ * Returns a pointer to the end of the parsed string on success, and stores
+ * the results in *addr, *port.
+ * Otherwise returns NULL and stores the error message in *errmsg, which must
+ * be freed by the caller.
+ */
+static const char *parse_in6_addr_port(const char *str, const char *kind,
+                                       int addr_sep, int port_sep,
+                                       struct in6_addr *addr, int *port,
+                                       char **errmsg)
+{
+    char buf[256];
+    const char *p = str;
+
+    if (*(p++) != '[') {
+        *errmsg = g_strdup_printf("IPv6 %s address must be enclosed"
+                                  " in square brackets", kind);
+        return NULL;
+    }
+    if (get_str_sep(buf, sizeof(buf), &p, ']') < 0) {
+        *errmsg = g_strdup_printf("IPv6 %s address must be enclosed"
+                                  " in square brackets", kind);
+        return NULL;
+    }
+    if (buf[0] == '\0') {
+        *addr = in6addr_any;
+    } else if (!inet_pton(AF_INET6, buf, addr)) {
+        *errmsg = g_strdup_printf("Bad %s address", kind);
+        return NULL;
+    }
+
+    if (*p++ != addr_sep) {
+        *errmsg = g_strdup_printf("Missing %s address separator after IPv6 address", kind);
+        return NULL;
+    }
+
+    if (get_str_sep(buf, sizeof(buf), &p, port_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s port separator", kind);
+        return NULL;
+    }
+    if (qemu_strtoi(buf, NULL, 10, port) < 0 ||
+        *port < 0 || *port > 65535) {
+        *errmsg = g_strdup_printf("Bad %s port", kind);
+        return NULL;
+    }
+
+    return p;
+}
+
 static void hmp_hostfwd_remove_worker(Monitor *mon, const QDict *qdict,
                                       int family)
 {
@@ -743,7 +802,12 @@ static void hmp_hostfwd_remove_worker(Monitor *mon, const QDict *qdict,
         }
         err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
     } else {
-        g_assert_not_reached();
+        struct in6_addr host_addr;
+        if (parse_in6_addr_port(p, "host", ':', '\0', &host_addr, &host_port,
+                                &errmsg) == NULL) {
+            goto fail_syntax;
+        }
+        err = slirp_remove_ipv6_hostfwd(s->slirp, is_udp, host_addr, host_port);
     }
 
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
@@ -760,6 +824,11 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
     hmp_hostfwd_remove_worker(mon, qdict, AF_INET);
 }
 
+void hmp_ipv6_hostfwd_remove(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_remove_worker(mon, qdict, AF_INET6);
+}
+
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 {
     struct in_addr host_addr = { .s_addr = INADDR_ANY };
@@ -807,6 +876,55 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
     return -1;
 }
 
+static int slirp_ipv6_hostfwd(SlirpState *s, const char *redir_str,
+                              Error **errp)
+{
+    struct in6_addr host_addr = in6addr_any;
+    struct in6_addr guest_addr;
+    int host_port, guest_port;
+    const char *p;
+    int is_udp;
+    char *errmsg = NULL;
+
+    memset(&guest_addr, 0, sizeof(guest_addr));
+    g_assert(redir_str != NULL);
+    p = redir_str;
+
+    p = parse_protocol(p, ':', &is_udp, &errmsg);
+    if (p == NULL) {
+        goto fail_syntax;
+    }
+
+    p = parse_in6_addr_port(p, "host", ':', '-', &host_addr, &host_port,
+                            &errmsg);
+    if (p == NULL) {
+        goto fail_syntax;
+    }
+
+    if (parse_in6_addr_port(p, "guest", ':', '\0', &guest_addr, &guest_port,
+                            &errmsg) == NULL) {
+        goto fail_syntax;
+    }
+    if (guest_port == 0) {
+        errmsg = g_strdup("Bad guest port");
+        goto fail_syntax;
+    }
+
+    if (slirp_add_ipv6_hostfwd(s->slirp, is_udp, host_addr, host_port,
+                               guest_addr, guest_port) < 0) {
+        error_setg(errp, "Could not set up host forwarding rule '%s'",
+                   redir_str);
+        return -1;
+    }
+    return 0;
+
+ fail_syntax:
+    error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
+               errmsg);
+    g_free(errmsg);
+    return -1;
+}
+
 static void hmp_hostfwd_add_worker(Monitor *mon, const QDict *qdict, int family)
 {
     const char *redir_str;
@@ -830,7 +948,7 @@ static void hmp_hostfwd_add_worker(Monitor *mon, const QDict *qdict, int family)
     if (family == AF_INET) {
         rc = slirp_hostfwd(s, redir_str, &err);
     } else {
-        g_assert_not_reached();
+        rc = slirp_ipv6_hostfwd(s, redir_str, &err);
     }
     if (rc < 0) {
         error_report_err(err);
@@ -842,6 +960,11 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
     hmp_hostfwd_add_worker(mon, qdict, AF_INET);
 }
 
+void hmp_ipv6_hostfwd_add(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_add_worker(mon, qdict, AF_INET6);
+}
+
 #ifndef _WIN32
 
 /* automatic user mode samba server configuration */
@@ -1148,6 +1271,7 @@ int net_init_slirp(const Netdev *netdev, const char *name,
     /* all optional fields are initialized to "all bits zero" */
 
     net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
+    net_init_slirp_configs(user->ipv6_hostfwd, SLIRP_CFG_IPV6_HOSTFWD);
     net_init_slirp_configs(user->guestfwd, 0);
 
     ret = net_slirp_init(peer, "user", name, user->q_restrict,
diff --git a/qapi/net.json b/qapi/net.json
index c31748c87f..443473107a 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -161,6 +161,9 @@
 # @hostfwd: redirect incoming TCP or UDP host connections to guest
 #           endpoints
 #
+# @ipv6-hostfwd: redirect incoming IPV6 TCP or UDP host connections to
+#                guest endpoints (since 6.0)
+#
 # @guestfwd: forward guest TCP connections
 #
 # @tftp-server-name: RFC2132 "TFTP server name" string (Since 3.1)
@@ -189,6 +192,7 @@
     '*smb':       'str',
     '*smbserver': 'str',
     '*hostfwd':   ['String'],
+    '*ipv6-hostfwd': ['String'],
     '*guestfwd':  ['String'],
     '*tftp-server-name': 'str' } }
 
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v3 0/3]

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210203233539.1990032-1-dje@google.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210203233539.1990032-1-dje@google.com
Subject: [PATCH v3 0/3]

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210203213729.1940893-1-dje@google.com -> patchew/20210203213729.1940893-1-dje@google.com
 * [new tag]         patchew/20210203233539.1990032-1-dje@google.com -> patchew/20210203233539.1990032-1-dje@google.com
Switched to a new branch 'test'
998dd12 net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands
e6a45e5 net/slirp.c: Refactor address parsing
6faf9a9 slirp: Placeholder for libslirp ipv6 hostfwd support

=== OUTPUT BEGIN ===
1/3 Checking commit 6faf9a93b1cf (slirp: Placeholder for libslirp ipv6 hostfwd support)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 2 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit e6a45e50689f (net/slirp.c: Refactor address parsing)
3/3 Checking commit 998dd12e51aa (net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands)
ERROR: line over 90 characters
#145: FILE: net/slirp.c:748:
+        *errmsg = g_strdup_printf("Missing %s address separator after IPv6 address", kind);

total: 1 errors, 0 warnings, 250 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210203233539.1990032-1-dje@google.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> Add support for ipv6 host forwarding
> 
> This patchset takes the original patch from Maxim,
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> and updates it.
> 
> New option: -ipv6-hostfwd
> 
> New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> 
> These are the ipv6 equivalents of their ipv4 counterparts.

Before I noticed this v3, I send a reply to your v2 sugesting
that we don't need to add any new commands/options. We can
use existing inet_parse() helper function to parse the address
info and transparently support IPv4/6 in the existing commands
and options. This matches normal practice elsewhere in QEMU
for IP dual stack.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > Add support for ipv6 host forwarding
> >
> > This patchset takes the original patch from Maxim,
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > and updates it.
> >
> > New option: -ipv6-hostfwd
> >
> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> >
> > These are the ipv6 equivalents of their ipv4 counterparts.
>
> Before I noticed this v3, I send a reply to your v2 sugesting
> that we don't need to add any new commands/options. We can
> use existing inet_parse() helper function to parse the address
> info and transparently support IPv4/6 in the existing commands
> and options. This matches normal practice elsewhere in QEMU
> for IP dual stack.
>

I'm all for this, fwiw.

[-- Attachment #2: Type: text/html, Size: 1458 bytes --]

Re: [PATCH v3 0/3]

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:

> On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
>> > Add support for ipv6 host forwarding
>> >
>> > This patchset takes the original patch from Maxim,
>> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
>> > and updates it.
>> >
>> > New option: -ipv6-hostfwd
>> >
>> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
>> >
>> > These are the ipv6 equivalents of their ipv4 counterparts.
>>
>> Before I noticed this v3, I send a reply to your v2 sugesting
>> that we don't need to add any new commands/options. We can
>> use existing inet_parse() helper function to parse the address
>> info and transparently support IPv4/6 in the existing commands
>> and options. This matches normal practice elsewhere in QEMU
>> for IP dual stack.
>>
>
> I'm all for this, fwiw.
>


I should say I'm all for not adding new commands/options.
Looking at inet_parse() it cannot be used as-is.
The question then becomes: Will refactoring it buy enough?

[-- Attachment #2: Type: text/html, Size: 2174 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> 
> > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> >> > Add support for ipv6 host forwarding
> >> >
> >> > This patchset takes the original patch from Maxim,
> >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> >> > and updates it.
> >> >
> >> > New option: -ipv6-hostfwd
> >> >
> >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> >> >
> >> > These are the ipv6 equivalents of their ipv4 counterparts.
> >>
> >> Before I noticed this v3, I send a reply to your v2 sugesting
> >> that we don't need to add any new commands/options. We can
> >> use existing inet_parse() helper function to parse the address
> >> info and transparently support IPv4/6 in the existing commands
> >> and options. This matches normal practice elsewhere in QEMU
> >> for IP dual stack.
> >>
> >
> > I'm all for this, fwiw.
> >
> 
> 
> I should say I'm all for not adding new commands/options.
> Looking at inet_parse() it cannot be used as-is.
> The question then becomes: Will refactoring it buy enough?

What's the problem your hitting with inet_parse ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> >
> > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com
> >
> > > wrote:
> > >
> > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > >> > Add support for ipv6 host forwarding
> > >> >
> > >> > This patchset takes the original patch from Maxim,
> > >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > >> > and updates it.
> > >> >
> > >> > New option: -ipv6-hostfwd
> > >> >
> > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > >> >
> > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > >>
> > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > >> that we don't need to add any new commands/options. We can
> > >> use existing inet_parse() helper function to parse the address
> > >> info and transparently support IPv4/6 in the existing commands
> > >> and options. This matches normal practice elsewhere in QEMU
> > >> for IP dual stack.
> > >>
> > >
> > > I'm all for this, fwiw.
> > >
> >
> >
> > I should say I'm all for not adding new commands/options.
> > Looking at inet_parse() it cannot be used as-is.
> > The question then becomes: Will refactoring it buy enough?
>
> What's the problem your hitting with inet_parse ?
>


First, this is the inet_parse() function we're talking about, right?

int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)

https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

[-- Attachment #2: Type: text/html, Size: 3171 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > >
> > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com
> > >
> > > > wrote:
> > > >
> > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > >> > Add support for ipv6 host forwarding
> > > >> >
> > > >> > This patchset takes the original patch from Maxim,
> > > >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > > >> > and updates it.
> > > >> >
> > > >> > New option: -ipv6-hostfwd
> > > >> >
> > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > >> >
> > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > >>
> > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > >> that we don't need to add any new commands/options. We can
> > > >> use existing inet_parse() helper function to parse the address
> > > >> info and transparently support IPv4/6 in the existing commands
> > > >> and options. This matches normal practice elsewhere in QEMU
> > > >> for IP dual stack.
> > > >>
> > > >
> > > > I'm all for this, fwiw.
> > > >
> > >
> > >
> > > I should say I'm all for not adding new commands/options.
> > > Looking at inet_parse() it cannot be used as-is.
> > > The question then becomes: Will refactoring it buy enough?
> >
> > What's the problem your hitting with inet_parse ?
> >
> 
> 
> First, this is the inet_parse() function we're talking about, right?
> 
> int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> 
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

Yes, that's right.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 2885 bytes --]

On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > > >
> > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> berrange@redhat.com
> > > >
> > > > > wrote:
> > > > >
> > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > >> > Add support for ipv6 host forwarding
> > > > >> >
> > > > >> > This patchset takes the original patch from Maxim,
> > > > >> >
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > > > >> > and updates it.
> > > > >> >
> > > > >> > New option: -ipv6-hostfwd
> > > > >> >
> > > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > > >> >
> > > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > > >>
> > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > >> that we don't need to add any new commands/options. We can
> > > > >> use existing inet_parse() helper function to parse the address
> > > > >> info and transparently support IPv4/6 in the existing commands
> > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > >> for IP dual stack.
> > > > >>
> > > > >
> > > > > I'm all for this, fwiw.
> > > > >
> > > >
> > > >
> > > > I should say I'm all for not adding new commands/options.
> > > > Looking at inet_parse() it cannot be used as-is.
> > > > The question then becomes: Will refactoring it buy enough?
> > >
> > > What's the problem your hitting with inet_parse ?
> > >
> >
> >
> > First, this is the inet_parse() function we're talking about, right?
> >
> > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> >
> >
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
>
> Yes, that's right.
>


Thanks, just wanted to be sure.

The syntax it supports is not the same as what's needed for host forwarding.
inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
hostfwd: address:port-address:port
If we wanted to have a utility that parsed "address:port" for v4+v6 then
we'd have to split the "address:port" part out of inet_parse.

Plus the way inet_parse() parses the address, which is fine for its
purposes, is with sscanf.
Alas the terminating character is not the same (',' vs '-').
IWBN to retain passing sscanf a constant format string so that the compiler
can catch various errors,
and if one keeps that then any kind of refactor loses some appeal.
[Though one could require all callers to accept either ',' or '-' as the
delimiter.]

[-- Attachment #2: Type: text/html, Size: 5012 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 02:40:17PM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > > > >
> > > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> > berrange@redhat.com
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > > >> > Add support for ipv6 host forwarding
> > > > > >> >
> > > > > >> > This patchset takes the original patch from Maxim,
> > > > > >> >
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > > > > >> > and updates it.
> > > > > >> >
> > > > > >> > New option: -ipv6-hostfwd
> > > > > >> >
> > > > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > > > >> >
> > > > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > > > >>
> > > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > > >> that we don't need to add any new commands/options. We can
> > > > > >> use existing inet_parse() helper function to parse the address
> > > > > >> info and transparently support IPv4/6 in the existing commands
> > > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > > >> for IP dual stack.
> > > > > >>
> > > > > >
> > > > > > I'm all for this, fwiw.
> > > > > >
> > > > >
> > > > >
> > > > > I should say I'm all for not adding new commands/options.
> > > > > Looking at inet_parse() it cannot be used as-is.
> > > > > The question then becomes: Will refactoring it buy enough?
> > > >
> > > > What's the problem your hitting with inet_parse ?
> > > >
> > >
> > >
> > > First, this is the inet_parse() function we're talking about, right?
> > >
> > > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > >
> > >
> > https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
> >
> > Yes, that's right.
> >
> 
> 
> Thanks, just wanted to be sure.
> 
> The syntax it supports is not the same as what's needed for host forwarding.
> inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
> hostfwd: address:port-address:port
> If we wanted to have a utility that parsed "address:port" for v4+v6 then
> we'd have to split the "address:port" part out of inet_parse.
> 
> Plus the way inet_parse() parses the address, which is fine for its
> purposes, is with sscanf.
> Alas the terminating character is not the same (',' vs '-').
> IWBN to retain passing sscanf a constant format string so that the compiler
> can catch various errors,
> and if one keeps that then any kind of refactor loses some appeal.
> [Though one could require all callers to accept either ',' or '-' as the
> delimiter.]

I think the key useful part to keep common impl for is the handling
of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
"address:port" part out into a new inet_addr_parse() helper that can be
called from inet_pase and from slirp.

inet_parse() can split on the first ",", and then call inet_addr_parse
on the first segment.

slirp can split on "-", and call inet_addr_parse with both segments.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Thu, Feb 11, 2021 at 1:12 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> [...]
>
> I think the key useful part to keep common impl for is the handling
> of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
> "address:port" part out into a new inet_addr_parse() helper that can be
> called from inet_pase and from slirp.
>
> inet_parse() can split on the first ",", and then call inet_addr_parse
> on the first segment.
>
> slirp can split on "-", and call inet_addr_parse with both segments.
>


v4 here:
https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06011.html

[-- Attachment #2: Type: text/html, Size: 1213 bytes --]

[PATCH v3 0/3]

[Abhash Jha]

Hello,

The first patch adds support for configuring the Sampling frequency
(inter-measurement period) of the sensor. The values should be provided
in milihertz. The default value for the inter-measurement period for
ALS is 10ms or 100000 mHz and for Range is 50ms or 20000 mHz.

The second patch adds support for interrupt based single shot reading.
We registered an irq_handler that fires everytime the data is ready.
And then we read the appropriate value in the `vl6180_measure` routine.

The third patch adds support for continuous mode in the sensor by using
buffers. We enable the sensor's continuous mode in the buffer_postenable
function depending on the `active_scan_mask`.
The continuous mode can be disabled by disabling the buffer.
Added a trigger to the device for the continuous mode. Also validating that
the device uses the internal trigger provided by us.

Changes in v2:
- Fixed `label followed by a declaration is a C23 extension [-Wc23-extensions]`
  by moving the guard(mutex)(&data->lock) above the switch statement.

- The above error was pointed out during testing by kernel-test-robot

Changes in v3:
- Fixed race condition related to `reinit_completion`
- Used `iio_for_each_active_channel` instead of manually accessing `masklength`
- Accepting sampling frequency values in milihertz instead of miliseconds.
- Minor code refactoring.

Thanks,
Abhash Jha

Abhash Jha (3):
  iio: light: vl6180: Add configurable inter-measurement period support
  iio: light: vl6180: Added Interrupt support for single shot access
  iio: light: vl6180: Add support for Continuous Mode

 drivers/iio/light/vl6180.c | 255 ++++++++++++++++++++++++++++++++++---
 1 file changed, 238 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v2 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

This v2 addresses an omission noted by Andrei Rybak[1]. I didn't
factor out the "name" into a variable in 2/3 for ease of reading
3/3. That's now done.

1. https://lore.kernel.org/git/23c4ce5f-7769-1d2c-3a97-ac9733f73a25@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  bundle cmd: stop leaking memory from parse_options_cmd_bundle()
  bundle.c: use a temporary variable for OIDs and names
  bundle: remove "ref_list" in favor of string-list.c API

 builtin/bundle.c | 91 ++++++++++++++++++++++++++++++++----------------
 bundle.c         | 74 ++++++++++++++++++++++-----------------
 bundle.h         | 20 +++++------
 transport.c      | 12 +++++--
 4 files changed, 122 insertions(+), 75 deletions(-)

Range-diff against v1:
1:  f4191088ac3 = 1:  932c0883ce0 bundle cmd: stop leaking memory from parse_options_cmd_bundle()
2:  f297fd0432a ! 2:  7e0d57951e5 bundle.c: use a temporary variable for OIDs and names
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    - 			add_pending_object(&revs, o, e->name);
    -@@ bundle.c: int verify_bundle(struct repository *r,
    +-			add_pending_object(&revs, o, e->name);
    ++			add_pending_object(&revs, o, name);
    + 			continue;
      		}
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      	if (revs.pending.nr != p->nr)
      		return ret;
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
    @@ bundle.c: int verify_bundle(struct repository *r,
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      
      	/* Clean up objects used, as they will be reused. */
    @@ bundle.c: int verify_bundle(struct repository *r,
     
      ## transport.c ##
     @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    + 
      	for (i = 0; i < data->header.references.nr; i++) {
      		struct ref_list_entry *e = data->header.references.list + i;
    - 		struct ref *ref = alloc_ref(e->name);
    +-		struct ref *ref = alloc_ref(e->name);
     -		oidcpy(&ref->old_oid, &e->oid);
    ++		const char *name = e->name;
    ++		struct ref *ref = alloc_ref(name);
     +		struct object_id *oid = &e->oid;
     +		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
3:  887313d3b02 ! 3:  9d9cb5aaf9e bundle: remove "ref_list" in favor of string-list.c API
    @@ bundle.c: int verify_bundle(struct repository *r,
      	repo_init_revisions(r, &revs, NULL);
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    --			add_pending_object(&revs, o, e->name);
    -+			add_pending_object(&revs, o, e->string);
    - 			continue;
    - 		}
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    - 	if (revs.pending.nr != p->nr)
    - 		return ret;
     @@ bundle.c: int verify_bundle(struct repository *r,
      			i--;
      
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		const struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
      		if (o->flags & SHOWN)
    - 			continue;
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    +@@ bundle.c: int verify_bundle(struct repository *r,
      
      	/* Clean up objects used, as they will be reused. */
      	for (i = 0; i < p->nr; i++) {
    @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport
      
      	for (i = 0; i < data->header.references.nr; i++) {
     -		struct ref_list_entry *e = data->header.references.list + i;
    --		struct ref *ref = alloc_ref(e->name);
    --		struct object_id *oid = &e->oid;
    +-		const char *name = e->name;
     +		struct string_list_item *e = data->header.references.items + i;
    -+		struct ref *ref = alloc_ref(e->string);
    -+		const struct object_id *oid = e->util;
    ++		const char *name = e->string;
    + 		struct ref *ref = alloc_ref(name);
    +-		struct object_id *oid = &e->oid;
    ++		struct object_id *oid = e->util;
      		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
      		result = ref;
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v3 0/3]

[Ævar Arnfjörð Bjarmason]

Refactor the bundle API to use the string_list API instead of its own
version of a similar API. See [1] for v2.

Addresses comments by Jeff King about us being too overzelous in
trying not to leak memory (the 'die_no_repo' is gone), and other
flow/style comments of his.

I also added a bundle_header_init() function for use in transport.c,
and noticed a redundant call to string_list_clear() there.

1. https://lore.kernel.org/git/cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  bundle cmd: stop leaking memory from parse_options_cmd_bundle()
  bundle.c: use a temporary variable for OIDs and names
  bundle: remove "ref_list" in favor of string-list.c API

 builtin/bundle.c | 74 ++++++++++++++++++++++++++++++------------------
 bundle.c         | 65 ++++++++++++++++++++++++++----------------
 bundle.h         | 21 +++++++-------
 transport.c      | 10 +++++--
 4 files changed, 105 insertions(+), 65 deletions(-)

Range-diff against v2:
1:  6a8b20a7cf3 < -:  ----------- upload-pack: run is_repository_shallow() before setup_revisions()
2:  d88b2c04102 < -:  ----------- revision.h: unify "disable_stdin" and "read_from_stdin"
3:  d433d7b24a3 < -:  ----------- pack-objects.c: do stdin parsing via revision.c's API
4:  e59a06c3148 < -:  ----------- pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
5:  f4191088ac3 ! 1:  3d0d7a8e8b5 bundle cmd: stop leaking memory from parse_options_cmd_bundle()
    @@ Commit message
         about those fixes if valgrind runs cleanly at the end without any
         leaks whatsoever.
     
    +    An earlier version of this change went out of its way to not leak
    +    memory on the die() codepaths here, but that was deemed too verbose to
    +    worry about in a built-in that's dying anyway. The only reason we'd
    +    need that is to appease a mode like SANITIZE=leak within the scope of
    +    an entire test file.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/bundle.c ##
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	struct strvec pack_opts;
      	int version = -1;
     -
    -+	int die_no_repo = 0;
     +	int ret;
      	struct option options[] = {
      		OPT_SET_INT('q', "quiet", &progress,
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	argc = parse_options_cmd_bundle(argc, argv, prefix,
      			builtin_bundle_create_usage, options, &bundle_file);
     @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
    - 	if (progress && all_progress_implied)
    - 		strvec_push(&pack_opts, "--all-progress-implied");
      
    --	if (!startup_info->have_repository)
    -+	if (!startup_info->have_repository) {
    -+		die_no_repo = 1;
    -+		goto cleanup;
    -+	}
    -+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
    -+cleanup:
    -+	free(bundle_file);
    -+	if (die_no_repo)
    + 	if (!startup_info->have_repository)
      		die(_("Need a repository to create a bundle."));
     -	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
    ++	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
    ++	free(bundle_file);
     +	return ret;
      }
      
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	struct bundle_header header;
      	int bundle_fd = -1;
     -
    -+	int die_no_repo = 0;
     +	int ret;
      	struct option options[] = {
      		OPT_END()
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	memset(&header, 0, sizeof(header));
     -	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
     -		return 1;
    --	if (!startup_info->have_repository)
    --		die(_("Need a repository to unbundle."));
    --	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
     +	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
     +		ret = 1;
     +		goto cleanup;
     +	}
    -+	if (!startup_info->have_repository) {
    -+		die_no_repo = 1;
    -+		goto cleanup;
    -+	}
    + 	if (!startup_info->have_repository)
    + 		die(_("Need a repository to unbundle."));
    +-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
     +	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
      		list_bundle_refs(&header, argc, argv);
     +cleanup:
    -+	if (die_no_repo)
    -+		die(_("Need a repository to unbundle."));
     +	free(bundle_file);
     +	return ret;
      }
6:  f297fd0432a ! 2:  e47646d3a98 bundle.c: use a temporary variable for OIDs and names
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    - 			add_pending_object(&revs, o, e->name);
    -@@ bundle.c: int verify_bundle(struct repository *r,
    +-			add_pending_object(&revs, o, e->name);
    ++			add_pending_object(&revs, o, name);
    + 			continue;
      		}
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      	if (revs.pending.nr != p->nr)
      		return ret;
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
    @@ bundle.c: int verify_bundle(struct repository *r,
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      
      	/* Clean up objects used, as they will be reused. */
    @@ bundle.c: int verify_bundle(struct repository *r,
     
      ## transport.c ##
     @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    + 
      	for (i = 0; i < data->header.references.nr; i++) {
      		struct ref_list_entry *e = data->header.references.list + i;
    - 		struct ref *ref = alloc_ref(e->name);
    +-		struct ref *ref = alloc_ref(e->name);
     -		oidcpy(&ref->old_oid, &e->oid);
    ++		const char *name = e->name;
    ++		struct ref *ref = alloc_ref(name);
     +		struct object_id *oid = &e->oid;
     +		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
7:  887313d3b02 ! 3:  f1066ee1b9a bundle: remove "ref_list" in favor of string-list.c API
    @@ builtin/bundle.c: static int cmd_bundle_list_heads(int argc, const char **argv,
     -	struct bundle_header header;
     +	struct bundle_header header = BUNDLE_HEADER_INIT;
      	int bundle_fd = -1;
    - 	int die_no_repo = 0;
      	int ret;
    + 	struct option options[] = {
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
      			builtin_bundle_unbundle_usage, options, &bundle_file);
      	/* bundle internals use argv[1] as further parameters */
    @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, co
      		ret = 1;
      		goto cleanup;
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
    - 	}
    + 		die(_("Need a repository to unbundle."));
      	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
      		list_bundle_refs(&header, argc, argv);
     +	bundle_header_release(&header);
      cleanup:
    - 	if (die_no_repo)
    - 		die(_("Need a repository to unbundle."));
    + 	free(bundle_file);
    + 	return ret;
     
      ## bundle.c ##
     @@ bundle.c: static struct {
    @@ bundle.c: static struct {
      
     -static void add_to_ref_list(const struct object_id *oid, const char *name,
     -		struct ref_list *list)
    --{
    ++void bundle_header_init(struct bundle_header *header)
    + {
     -	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
     -	oidcpy(&list->list[list->nr].oid, oid);
     -	list->list[list->nr].name = xstrdup(name);
     -	list->nr++;
    --}
    --
    - static int parse_capability(struct bundle_header *header, const char *capability)
    - {
    - 	const char *arg;
    -@@ bundle.c: static int parse_bundle_header(int fd, struct bundle_header *header,
    - 	/* The bundle header ends with an empty line */
    - 	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
    - 	       buf.len && buf.buf[0] != '\n') {
    --		struct object_id oid;
    -+		struct object_id *oid;
    - 		int is_prereq = 0;
    - 		const char *p;
    ++	memset(header, 0, sizeof(*header));
    ++	string_list_init(&header->prerequisites, 1);
    ++	string_list_init(&header->references, 1);
    ++}
    ++
    ++void bundle_header_release(struct bundle_header *header)
    ++{
    ++	string_list_clear(&header->prerequisites, 1);
    ++	string_list_clear(&header->references, 1);
    + }
      
    + static int parse_capability(struct bundle_header *header, const char *capability)
     @@ bundle.c: static int parse_bundle_header(int fd, struct bundle_header *header,
    - 		 * Prerequisites have object name that is optionally
    - 		 * followed by SP and subject line.
    - 		 */
    --		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
    -+		oid = xmalloc(sizeof(struct object_id));
    -+		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
    - 		    (*p && !isspace(*p)) ||
    - 		    (!is_prereq && !*p)) {
    - 			if (report_path)
    - 				error(_("unrecognized header: %s%s (%d)"),
    - 				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
      			status = -1;
    -+			free(oid);
      			break;
      		} else {
    --			if (is_prereq)
    ++			struct object_id *dup = oiddup(&oid);
    + 			if (is_prereq)
     -				add_to_ref_list(&oid, "", &header->prerequisites);
    --			else
    ++				string_list_append(&header->prerequisites, "")->util = dup;
    + 			else
     -				add_to_ref_list(&oid, p + 1, &header->references);
    -+			const char *string = is_prereq ? "" : p + 1;
    -+			struct string_list *list = is_prereq
    -+				? &header->prerequisites
    -+				: &header->references;
    -+			string_list_append(list, string)->util = oid;
    ++				string_list_append(&header->references, p + 1)->util = dup;
      		}
      	}
      
    @@ bundle.c: int verify_bundle(struct repository *r,
      	repo_init_revisions(r, &revs, NULL);
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    --			add_pending_object(&revs, o, e->name);
    -+			add_pending_object(&revs, o, e->string);
    - 			continue;
    - 		}
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    - 	if (revs.pending.nr != p->nr)
    - 		return ret;
     @@ bundle.c: int verify_bundle(struct repository *r,
      			i--;
      
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		const struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
      		if (o->flags & SHOWN)
    - 			continue;
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    +@@ bundle.c: int verify_bundle(struct repository *r,
      
      	/* Clean up objects used, as they will be reused. */
      	for (i = 0; i < p->nr; i++) {
    @@ bundle.c: int verify_bundle(struct repository *r,
      
      		r = &header->references;
      		printf_ln(Q_("The bundle contains this ref:",
    -@@ bundle.c: int unbundle(struct repository *r, struct bundle_header *header,
    - 		return error(_("index-pack died"));
    - 	return 0;
    - }
    -+
    -+void bundle_header_release(struct bundle_header *header)
    -+{
    -+	string_list_clear(&header->prerequisites, 1);
    -+	string_list_clear(&header->references, 1);
    -+}
     
      ## bundle.h ##
     @@
    @@ bundle.h
     +	.prerequisites = STRING_LIST_INIT_DUP, \
     +	.references = STRING_LIST_INIT_DUP, \
     +}
    ++void bundle_header_init(struct bundle_header *header);
    ++void bundle_header_release(struct bundle_header *header);
     +
      int is_bundle(const char *path, int quiet);
      int read_bundle_header(const char *path, struct bundle_header *header);
      int create_bundle(struct repository *r, const char *path,
    -@@ bundle.h: int unbundle(struct repository *r, struct bundle_header *header,
    - 	     int bundle_fd, int flags);
    - int list_bundle_refs(struct bundle_header *header,
    - 		int argc, const char **argv);
    -+void bundle_header_release(struct bundle_header *header);
    - 
    - #endif
     
      ## transport.c ##
     @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport
      
      	for (i = 0; i < data->header.references.nr; i++) {
     -		struct ref_list_entry *e = data->header.references.list + i;
    --		struct ref *ref = alloc_ref(e->name);
    --		struct object_id *oid = &e->oid;
    +-		const char *name = e->name;
     +		struct string_list_item *e = data->header.references.items + i;
    -+		struct ref *ref = alloc_ref(e->string);
    -+		const struct object_id *oid = e->util;
    ++		const char *name = e->string;
    + 		struct ref *ref = alloc_ref(name);
    +-		struct object_id *oid = &e->oid;
    ++		struct object_id *oid = e->util;
      		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
      		result = ref;
    - 	}
    -+	string_list_clear(&data->header.references, 1);
    - 	return result;
    - }
    - 
     @@ transport.c: static int close_bundle(struct transport *transport)
      	struct bundle_transport_data *data = transport->data;
      	if (data->fd > 0)
    @@ transport.c: struct transport *transport_get(struct remote *remote, const char *
      		die(_("git-over-rsync is no longer supported"));
      	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
      		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
    -+		string_list_init(&data->header.prerequisites, 1);
    -+		string_list_init(&data->header.references, 1);
    ++		bundle_header_init(&data->header);
      		transport_check_allowed("file");
      		ret->data = data;
      		ret->vtable = &bundle_vtable;
-- 
2.32.0.613.g8e17abc2eb

Re: [PATCH v3 0/3]

[Jeff King]

On Wed, Jun 30, 2021 at 04:06:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Refactor the bundle API to use the string_list API instead of its own
> version of a similar API. See [1] for v2.
> 
> Addresses comments by Jeff King about us being too overzelous in
> trying not to leak memory (the 'die_no_repo' is gone), and other
> flow/style comments of his.
> 
> I also added a bundle_header_init() function for use in transport.c,
> and noticed a redundant call to string_list_clear() there.

Thanks, all three look good to me.

As an aside, having to have a separate bundle_header_init() and
BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
date with each other), but quite common in our code base. I wonder if
writing:

  void bundle_header_init(struct bundle_header *header)
  {
	struct bundle_header blank = BUNDLE_HEADER_INIT;
	memcpy(header, &blank, sizeof(*header));
  }

would let a smart enough compiler just init "header" in place without
the extra copy (the performance of a single bundle_header almost
certainly doesn't matter, but it might for other types).

Just musing. ;)

-Peff

Re: [PATCH v3 0/3]

[Jeff King]

On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:

> As an aside, having to have a separate bundle_header_init() and
> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
> date with each other), but quite common in our code base. I wonder if
> writing:
> 
>   void bundle_header_init(struct bundle_header *header)
>   {
> 	struct bundle_header blank = BUNDLE_HEADER_INIT;
> 	memcpy(header, &blank, sizeof(*header));
>   }
> 
> would let a smart enough compiler just init "header" in place without
> the extra copy (the performance of a single bundle_header almost
> certainly doesn't matter, but it might for other types).
> 
> Just musing. ;)

For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9

With "gcc -O2" the memcpy goes away and we init "header" directly.

If we want to start using this technique widely, I don't think it should
be part of your series, though. This probably applies to quite a few
data structures, so it would make more sense to have a series which
converts several.

-Peff

Re: [PATCH v3 0/3]

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Jeff King wrote:

> On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
>
>> As an aside, having to have a separate bundle_header_init() and
>> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
>> date with each other), but quite common in our code base. I wonder if
>> writing:
>> 
>>   void bundle_header_init(struct bundle_header *header)
>>   {
>> 	struct bundle_header blank = BUNDLE_HEADER_INIT;
>> 	memcpy(header, &blank, sizeof(*header));
>>   }
>> 
>> would let a smart enough compiler just init "header" in place without
>> the extra copy (the performance of a single bundle_header almost
>> certainly doesn't matter, but it might for other types).
>> 
>> Just musing. ;)
>
> For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
>
> With "gcc -O2" the memcpy goes away and we init "header" directly.
>
> If we want to start using this technique widely, I don't think it should
> be part of your series, though. This probably applies to quite a few
> data structures, so it would make more sense to have a series which
> converts several.

That's cool, yeah that would make quite a lot of code better. Thanks!

Re: [PATCH v3 0/3]

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jun 30 2021, Jeff King wrote:
>
>> On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
>>
>>> As an aside, having to have a separate bundle_header_init() and
>>> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
>>> date with each other), but quite common in our code base. I wonder if
>>> writing:
>>> 
>>>   void bundle_header_init(struct bundle_header *header)
>>>   {
>>> 	struct bundle_header blank = BUNDLE_HEADER_INIT;
>>> 	memcpy(header, &blank, sizeof(*header));
>>>   }
>>> 
>>> would let a smart enough compiler just init "header" in place without
>>> the extra copy (the performance of a single bundle_header almost
>>> certainly doesn't matter, but it might for other types).
>>> 
>>> Just musing. ;)
>>
>> For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
>>
>> With "gcc -O2" the memcpy goes away and we init "header" directly.
>>
>> If we want to start using this technique widely, I don't think it should
>> be part of your series, though. This probably applies to quite a few
>> data structures, so it would make more sense to have a series which
>> converts several.
>
> That's cool, yeah that would make quite a lot of code better. Thanks!

Just for future reference: I submitted a small series to make use of
this suggested idiom:
https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/

[PATCH v3 0/3]

[Yordan Karadzhov (VMware)]

The patch-set implements new commandline options for KernelShark that
can be used to pre-select the CPU and Task plots to be shown, before
opening the GUI. The idea was suggested by Julia Lawall.

Changes is v2:
 - Fixing a bug in the parsing of the string representing the list of
   IDs, given to the command line options.

 - New Patch 3/3 that limits the number of CPU plots shown by default
   when KernelShark starts. 

Changes is v3:
 - Switching to using comma instead of space for separating the
   Ids (Pid or CPU) of the plots. This way we don't need to worry
   about quoting the arguments.

 - Adding to patch [2/3] logic to handle the case when the user uses
   the "--cpu" or "--pid" options multiple times.

Yordan Karadzhov (VMware) (3):
  kernel-shark: Add methods for selecting the plots to be shown
  kernel-shark: Add command line options for selecting plots to be shown
  kernel-shark: Set a maximum number of plots to be shown by default

 kernel-shark/src/KsGLWidget.cpp   | 13 +++++++++--
 kernel-shark/src/KsMainWindow.cpp | 39 +++++++++++++++++++++++++++++++
 kernel-shark/src/KsMainWindow.hpp |  4 ++++
 kernel-shark/src/KsUtils.cpp      | 24 +++++++++++++++++++
 kernel-shark/src/KsUtils.hpp      |  2 ++
 kernel-shark/src/kernelshark.cpp  | 34 ++++++++++++++++++++++++---
 6 files changed, 111 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH v2 0/2] refactoring branch colorization to ref-filter

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

Finally found some time to follow up on this :)

I decided to take the approach suggested by Peff for simplicity. I have
another version which uses a hash map to store *all* of the information
returned by get_worktrees, information which can then be accessed in
the fashion %(workree:path) or %(worktree:is_detached), but it seemed
like a lot of code to add and there was no use case to justify the
addition of a hash map at this time. If there's interest though, I can
make a separate patch after this one to introduce those changes. They
build directly off of the changes introduced here.

I've split this work into two commits since the items are logically
separate.

CI results: https://travis-ci.org/nbelakovski/git/builds/453723727

Nickolai Belakovski (2):
  ref-filter: add worktree atom
  branch: Mark and colorize a branch differently if it is checked out in
    a linked worktree

 builtin/branch.c               | 22 +++++++++++++---------
 color.h                        | 18 ++++++++++++++++++
 ref-filter.c                   | 31 +++++++++++++++++++++++++++++++
 t/t3200-branch.sh              |  8 ++++----
 t/t3203-branch-output.sh       | 21 +++++++++++++++++++++
 t/t6302-for-each-ref-filter.sh | 15 +++++++++++++++
 t/test-lib-functions.sh        |  6 ++++++
 7 files changed, 108 insertions(+), 13 deletions(-)

-- 
2.14.2

[PATCH v3 0/3]

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

Finally got around to submitting latest changes.

I think this addresses all the feedback

The atom now returns the worktree path instead of '+'

I stuck to cyan for the coloring, since it seemed most popular

I added one more change to display the worktree path in cyan for git branch -vvv
Not sure if it's in the best place, but it seemed like it would be nice to add
the path in the same color so that there's some visibility as to why a particular
branch is colored in cyan. If it proves to be controversial, I wouldn't want it to
hold up this series, we can skip it and I can move discussion to a separate thread
(or just forget it, as the case may be)

Travis CI results: https://travis-ci.org/nbelakovski/git/builds/468569102

Nickolai Belakovski (3):
  ref-filter: add worktreepath atom
  branch: Mark and color a branch differently if it is checked out in a
    linked worktree
  branch: Add an extra verbose output displaying worktree path for refs
    checked out in a linked worktree

 Documentation/git-for-each-ref.txt |  4 +++
 builtin/branch.c                   | 16 ++++++---
 ref-filter.c                       | 70 ++++++++++++++++++++++++++++++++++++++
 t/t3200-branch.sh                  |  8 ++---
 t/t3203-branch-output.sh           | 21 ++++++++++++
 t/t6302-for-each-ref-filter.sh     | 15 ++++++++
 6 files changed, 126 insertions(+), 8 deletions(-)

-- 
2.14.2

Re: [PATCH v3 0/3]

[Jeff King]

On Sun, Dec 16, 2018 at 01:57:56PM -0800, nbelakovski@gmail.com wrote:

> Finally got around to submitting latest changes.
> 
> I think this addresses all the feedback
> 
> The atom now returns the worktree path instead of '+'

Thanks, I think patch 1 is definitely going in the right direction.
There are a few issues I found in its implementation, but they should be
easy to fix (and won't affect the other patches).

I don't really have a strong opinion on the behavior of the other
patches.

-Peff

[PATCH v3 0/3]

[Kefeng Wang]

Make dw8250_set_termios() as the default set_termios callback for 8250 dw uart, correct me
if I am wrong.

Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that it is not 16500
compatible. Meanwhile, set dw8250_serial_out32 to keep consistent between serial_out
and serial_in in ACPI.

Change since v2:
- Add a new patch to use new var dev in probe
- Use built-in device properties to set device parameters for existing device probed by acpi,
  suggested by Andy Shevchenko 


Change since v1:
- Use acpi_match_device() instead of acpi_dev_found(), limit the check to the device
  being probed and not a global search for whole DSDT (pointed by graeme.gregory@linaro.org)

Kefeng Wang (3):
  serial: 8250_dw: make dw8250_set_termios as default set_termios
    callback
  serial: 8250_dw: Use new dev variable in probe
  serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

 drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 30 deletions(-)

-- 
1.7.12.4

Re: [PATCH v3 0/3]

[Andy Shevchenko]

On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> Make dw8250_set_termios() as the default set_termios callback for 8250
> dw uart, correct me
> if I am wrong.
> 
> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
> it is not 16500
> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
> between serial_out
> and serial_in in ACPI.

I will review it perhaps late this week.
Though have a quick question, did you test it? I hope you have done.

> 
> Change since v2:
> - Add a new patch to use new var dev in probe
> - Use built-in device properties to set device parameters for existing
> device probed by acpi,
>   suggested by Andy Shevchenko 
> 
> 
> Change since v1:
> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
> to the device
>   being probed and not a global search for whole DSDT (pointed by grae
> me.gregory@linaro.org)
> 
> Kefeng Wang (3):
>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>     callback
>   serial: 8250_dw: Use new dev variable in probe
>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
> 
>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
> -----------
>  1 file changed, 41 insertions(+), 30 deletions(-)
> 

-- 

Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/3]

[Kefeng Wang]

On 2016/8/10 0:01, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>> Make dw8250_set_termios() as the default set_termios callback for 8250
>> dw uart, correct me
>> if I am wrong.
>>
>> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
>> it is not 16500
>> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
>> between serial_out
>> and serial_in in ACPI.
> 
> I will review it perhaps late this week.

Thanks.

> Though have a quick question, did you test it? I hope you have done.

Yes, I have already tested the patchset on D02 board.

Kefeng

> 
>>
>> Change since v2:
>> - Add a new patch to use new var dev in probe
>> - Use built-in device properties to set device parameters for existing
>> device probed by acpi,
>>   suggested by Andy Shevchenko 
>>
>>
>> Change since v1:
>> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
>> to the device
>>   being probed and not a global search for whole DSDT (pointed by grae
>> me.gregory@linaro.org)
>>
>> Kefeng Wang (3):
>>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>>     callback
>>   serial: 8250_dw: Use new dev variable in probe
>>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
>>
>>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
>> -----------
>>  1 file changed, 41 insertions(+), 30 deletions(-)
>>
> 

Re: [PATCH v3 0/3]

[Kefeng Wang]

On 2016/8/10 13:36, Kefeng Wang wrote:
> 
> 
> On 2016/8/10 0:01, Andy Shevchenko wrote:
>> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>>> Make dw8250_set_termios() as the default set_termios callback for 8250
>>> dw uart, correct me
>>> if I am wrong.
>>>
>>> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
>>> it is not 16500
>>> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
>>> between serial_out
>>> and serial_in in ACPI.
>>
>> I will review it perhaps late this week.
> 

Hi Andy, kindly ping...


> Thanks.
> 
>> Though have a quick question, did you test it? I hope you have done.
> 
> Yes, I have already tested the patchset on D02 board.
> 
> Kefeng
> 
>>
>>>
>>> Change since v2:
>>> - Add a new patch to use new var dev in probe
>>> - Use built-in device properties to set device parameters for existing
>>> device probed by acpi,
>>>   suggested by Andy Shevchenko 
>>>
>>>
>>> Change since v1:
>>> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
>>> to the device
>>>   being probed and not a global search for whole DSDT (pointed by grae
>>> me.gregory@linaro.org)
>>>
>>> Kefeng Wang (3):
>>>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>>>     callback
>>>   serial: 8250_dw: Use new dev variable in probe
>>>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
>>>
>>>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
>>> -----------
>>>  1 file changed, 41 insertions(+), 30 deletions(-)
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v3 0/3]

[Andy Shevchenko]

On Fri, 2016-08-19 at 14:26 +0800, Kefeng Wang wrote:
> 
> Make dw8250_set_termios() as the default set_termios callback
> > > > for 8250
> > > > dw uart, correct me
> > > > if I am wrong.
> > > > 
> > > > Then add ACPI support for uart on Hisilicon Hip05 soc, be
> > > > careful that
> > > > it is not 16500
> > > > compatible. Meanwhile, set dw8250_serial_out32 to keep
> > > > consistent
> > > > between serial_out
> > > > and serial_in in ACPI.
> > > 
> > > I will review it perhaps late this week.
> > 
> 
> Hi Andy, kindly ping...

Sorry for being a bit late, but here we are.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 0/3]

[Jacob Pan]

Minor syntax change based on Joe Perches comments.
https://lkml.org/lkml/2012/11/13/27
Missed the actual changes in v2.

We have done some experiment with idle injection on Intel platforms.
The idea is to use the increasingly power efficient package level
C-states for power capping and passive thermal control.

Documentation is included in the patch to explain the theory of
operation, performance implication, calibration, scalability, and user
interface. Please refer to the following file for more details.

Documentation/thermal/intel_powerclamp.txt

Arjan van de Ven created the original idea and driver, I have been
refining driver in hope that they can be to be useful beyond a proof
of concept.


Jacob Pan (3):
  tick: export nohz tick idle symbols for module use
  x86/nmi: export local_touch_nmi() symbol for modules
  PM: Introduce Intel PowerClamp Driver

 Documentation/thermal/intel_powerclamp.txt |  307 +++++++++++
 arch/x86/kernel/nmi.c                      |    1 +
 drivers/thermal/Kconfig                    |   10 +
 drivers/thermal/Makefile                   |    1 +
 drivers/thermal/intel_powerclamp.c         |  766 ++++++++++++++++++++++++++++
 kernel/time/tick-sched.c                   |    2 +
 6 files changed, 1087 insertions(+)
 create mode 100644 Documentation/thermal/intel_powerclamp.txt
 create mode 100644 drivers/thermal/intel_powerclamp.c

-- 
1.7.9.5

Re: [PATCH v2 1/3] Move the user-facing test library to test-lib-functions.sh

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> This just moves all the user-facing functions to a separate file and
> sources that instead.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  t/test-lib-functions.sh |  835 +++++++++++++++++++++++++++++++++++++++++++++++
>  t/test-lib.sh           |  552 +-------------------------------
>  2 files changed, 840 insertions(+), 547 deletions(-)
>  create mode 100644 t/test-lib-functions.sh

I would have expected from the log description that the number of deleted
lines would be about the same as the number of added lines, and the
difference would primarily come from the addition of "include" aka "dot"
". ./test-lib-functions.sh" that becomes necessary in t/test-lib.sh, some
boilerplate material at the beginning of the new file e.g. "#!/bin/sh",
and copying (not moving) the same Copyright block to the new file.

But 835-552 = 283 feels way way more than that.  What else is going on?

[PATCH v3 0/3]

[Thomas Rast]

Junio C Hamano wrote:
> > ---
> >  t/test-lib-functions.sh |  835 +++++++++++++++++++++++++++++++++++++++++++++++
> >  t/test-lib.sh           |  552 +-------------------------------
> >  2 files changed, 840 insertions(+), 547 deletions(-)
> >  create mode 100644 t/test-lib-functions.sh
> 
> I would have expected from the log description that the number of deleted
> lines would be about the same as the number of added lines, and the
> difference would primarily come from the addition of "include" aka "dot"
> ". ./test-lib-functions.sh" that becomes necessary in t/test-lib.sh, some
> boilerplate material at the beginning of the new file e.g. "#!/bin/sh",
> and copying (not moving) the same Copyright block to the new file.

There were actually more mistakes lurking :-( so I am resending the
whole series.  I also put in the copyright that you asked for.  I
verified the results by looking at the diff between a reverse git-show
for test-lib.sh and a forward git-show for test-lib-functions.sh,
which looks as follows:

  --- /dev/fd/63	2012-02-17 10:55:32.994197654 +0100
  +++ /dev/fd/62	2012-02-17 10:55:32.994197654 +0100
  @@ -9,17 +9,29 @@
       
       Signed-off-by: Thomas Rast <trast@student.ethz.ch>
   
  -diff --git b/t/test-lib.sh a/t/test-lib.sh
  -index 1da3f40..e28d5fd 100644
  ---- b/t/test-lib.sh
  -+++ a/t/test-lib.sh
  -@@ -223,9 +223,248 @@ die () {
  - GIT_EXIT_OK=
  - trap 'die' EXIT
  - 
  --# The user-facing functions are loaded from a separate file so that
  --# test_perf subshells can have them too
  --. "${TEST_DIRECTORY:-.}"/test-lib-functions.sh
  +diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
  +new file mode 100644
  +index 0000000..7b3b4be
  +--- /dev/null
  ++++ b/t/test-lib-functions.sh
  +@@ -0,0 +1,565 @@
  ++#!/bin/sh
  ++#
  ++# Copyright (c) 2005 Junio C Hamano
  ++#
  ++# This program is free software: you can redistribute it and/or modify
  ++# it under the terms of the GNU General Public License as published by
  ++# the Free Software Foundation, either version 2 of the License, or
  ++# (at your option) any later version.
  ++#
  ++# This program is distributed in the hope that it will be useful,
  ++# but WITHOUT ANY WARRANTY; without even the implied warranty of
  ++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  ++# GNU General Public License for more details.
  ++#
  ++# You should have received a copy of the GNU General Public License
  ++# along with this program.  If not, see http://www.gnu.org/licenses/ .
  ++
   +# The semantics of the editor variables are that of invoking
   +# sh -c "$EDITOR \"$@\"" files ...
   +#
  @@ -192,7 +204,6 @@
   +	git config "$@"
   +}
   +
  -+
   +test_config_global () {
   +	test_when_finished "test_unconfig --global '$1'" &&
   +	git config --global "$@"
  @@ -262,13 +273,7 @@
   +	esac
   +	return 1
   +}
  - 
  - # You are not expected to call test_ok_ and test_failure_ directly, use
  - # the text_expect_* functions instead.
  -@@ -313,6 +552,313 @@ test_skip () {
  - 	esac
  - }
  - 
  ++
   +test_expect_failure () {
   +	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
   +	test "$#" = 2 ||
  @@ -575,7 +580,3 @@
   +		mv .git/hooks .git/hooks-disabled
   +	) || exit
   +}
  -+
  - test_done () {
  - 	GIT_EXIT_OK=t
  - 



Thomas Rast (3):
  Move the user-facing test library to test-lib-functions.sh
  Introduce a performance testing framework
  Add a performance test for git-grep

 Makefile                        |   22 +-
 t/Makefile                      |   43 ++-
 t/perf/.gitignore               |    2 +
 t/perf/Makefile                 |   15 +
 t/perf/README                   |  146 ++++++++++
 t/perf/aggregate.perl           |  166 +++++++++++
 t/perf/min_time.perl            |   21 ++
 t/perf/p0000-perf-lib-sanity.sh |   41 +++
 t/perf/p0001-rev-list.sh        |   17 ++
 t/perf/p7810-grep.sh            |   23 ++
 t/perf/perf-lib.sh              |  198 ++++++++++++++
 t/perf/run                      |   82 ++++++
 t/test-lib-functions.sh         |  565 ++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                   |  574 ++-------------------------------------
 14 files changed, 1363 insertions(+), 552 deletions(-)
 create mode 100644 t/perf/.gitignore
 create mode 100644 t/perf/Makefile
 create mode 100644 t/perf/README
 create mode 100755 t/perf/aggregate.perl
 create mode 100755 t/perf/min_time.perl
 create mode 100755 t/perf/p0000-perf-lib-sanity.sh
 create mode 100755 t/perf/p0001-rev-list.sh
 create mode 100755 t/perf/p7810-grep.sh
 create mode 100644 t/perf/perf-lib.sh
 create mode 100755 t/perf/run
 create mode 100644 t/test-lib-functions.sh

-- 
1.7.9.1.365.ge223f

Re: [PATCH v3 0/3]

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> There were actually more mistakes lurking :-( so I am resending the
> whole series.

Ok, will requeue.  The diff you attached to this cover letter looked at
least halfway sane, compared to the previous round ;-), though it is not
exactly clear what goes to lib-test-functions and what goes to lib-test
(for example, you moved test_expect_success to 'test-functions', but it
calls test_ok_ that is in 'test-lib', and test_ok_ is directly used by
test_perf in the new 't/perf/perf-lib.sh'), making it harder for people to
decide where to put their additions to the test infrastructure from now
on.  There needs a bit of description in the first patch to guide them.

I seem to be getting intermittent test failures, and every time the
failing tests are different, when these three are queued to 'pu'. I didn't
look for what goes wrong and how.

Thanks.

Re: [PATCH v3 0/3]

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
> ...
> I seem to be getting intermittent test failures, and every time the
> failing tests are different, when these three are queued to 'pu'. I didn't
> look for what goes wrong and how.

False alarm. I suspect that it is jb/required-filter topic that is causing
intermittent failures from convert.c depending on the timing of how fast
filter subprocess dies vs how fast we consume its result or something.

Repeatedly running t0021 like this:

    $ cd t
    $ while sh t0021-conversion.sh ; do :; done

under load seems to make it fail every once in a while.

test_must_fail: died by signal: git add test.fc

Are we dying on SIGPIPE or something?

Re: [PATCH v3 0/3]

[Jeff King]

On Fri, Feb 17, 2012 at 03:28:51PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Thomas Rast <trast@student.ethz.ch> writes:
> > ...
> > I seem to be getting intermittent test failures, and every time the
> > failing tests are different, when these three are queued to 'pu'. I didn't
> > look for what goes wrong and how.
> 
> False alarm. I suspect that it is jb/required-filter topic that is causing
> intermittent failures from convert.c depending on the timing of how fast
> filter subprocess dies vs how fast we consume its result or something.
> 
> Repeatedly running t0021 like this:
> 
>     $ cd t
>     $ while sh t0021-conversion.sh ; do :; done
> 
> under load seems to make it fail every once in a while.
> 
> test_must_fail: died by signal: git add test.fc
> 
> Are we dying on SIGPIPE or something?

I would be unsurprised if that is the case. Joey Hess mentioned similar
issues with hooks a month or two ago. And I have been seeing
intermittent failures of t5541 under load that I traced back to SIGPIPE.
I've been meaning to dig further and come up with a good solution.
Here's some previous discussion:

  http://article.gmane.org/gmane.comp.version-control.git/186291

I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
the log family.

-Peff

Re: [PATCH v3 0/3]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
> the log family.

Hmmmm, now you confused me...  What is special about the log family?  Do
you mean "when we use pager"?  But then we are writing into the pager,
which the user can make it exit, which in turn causes us to write into the
pipe, so I would expect that we would want to ignore SIGPIPE --- ah, then
we explicitly catch error in xwrite() and say die() which we do not want.

So you want to let SIGPIPE silently kill us when the pager is in use; is
that it?

Re: [PATCH v3 0/3]

[Jeff King]

On Fri, Feb 17, 2012 at 11:27:26PM -0800, Junio C Hamano wrote:

> > I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
> > the log family.
> 
> Hmmmm, now you confused me...  What is special about the log family?  Do
> you mean "when we use pager"?  But then we are writing into the pager,
> which the user can make it exit, which in turn causes us to write into the
> pipe, so I would expect that we would want to ignore SIGPIPE --- ah, then
> we explicitly catch error in xwrite() and say die() which we do not want.

Sort of. I mentioned the log family because those are the commands that
tend to generate large amounts of input, and which are likely to hit
SIGPIPE. But let me elaborate on my thinking a bit.

There are basically three types of writing callsites in git:

  1. Careful calls to write() or fwrite().

     These are the majority of calls. In these, we check the return
     value of write() and die, return the error code, or whatever is
     appropriate for the callsite. SIGPIPE kills the program before the
     careful code gets the chance to make a decision about what to do.
     At best, this is a nuisance; even if the program is going to die(),
     it's likely that the custom code could produce a useful error
     message. At worst it causes bugs. For example, we may write to a
     subprocess that only needs part of our input to make a decision
     (e.g., some hooks and credential helpers). With SIGPIPE, we end up
     dying even though no error has occurred. Worse, these are often
     annoying heisenbugs that depend on the timing of write() and
     close() between the two processes.

  2. Non-careful calls of small, fixed-size output.

     Things like "git remote" will write some output to stdout via
     printf and not bother checking the return code. As the main purpose
     of the program is to create output, it's OK to be killed by
     SIGPIPE if it happens due to a write to stdout. But respecting
     SIGPIPE doesn't buy as all that much. It might save us from making
     a few write() calls that will go to nowhere, but most of the work
     has already been done by the time we are outputting.

     And it has a cost to the other careful write calls in the same
     program, because respecting SIGPIPE is a per-process thing. So for
     something like "git remote show foo", while we would like SIGPIPE
     to kick in for output to stdout, we would not want it for a pipe we
     opened to talk to ls-remote.

  3. Non-careful calls of large, streaming data.

     Commands like "git log" will non-carefully output to stdout, as
     well, but they will generate tons of data, consuming possibly
     minutes of CPU time (e.g., "git log -p"). If whoever is reading the
     output stops doing so, we really want to kill the program to avoid
     wasting CPU time.  In this instance, SIGPIPE is a big win.

     It still has the downside that careful calls in the same program
     will be subject to using SIGPIPE. For "log" and friends, this is
     probably OK with the current code, as we don't make a lot of pipes.
     But that is somewhat of an implementation detail. E.g., "git log
     -p" with external diff or textconv writes to a tempfile, and then
     runs a subprocess with the tempfile as input. But we could just as
     easily have used pipes, and may choose to do so in the future. You
     may even be able to trigger a convert_to_git filter in the current
     code, which does use pipes.

So basically, I find SIGPIPE to be a simplistic too-heavy hammer that
ends up affecting all writes to pipes, when in reality we are only
interested in affecting ones to some "main" output (usually stdout).
That works OK for small Unix-y programs like "head", but is overly
simplistic for something as big as git.

In an ideal world, we could set a per-descriptor version of SIGPIPE, and
just turn it on for stdout (or somehow find out which descriptor caused
the SIGPIPE after the fact). But that's not possible.

So our next best thing would be to actually check the results of
these non-careful writes. Unfortunately, this means either:

  a. wrapping every printf with a function that will appropriately die
     on error. This makes the code more cumbersome.

  or

  b. occasionally checking ferror(stdout) while doing long streams
     (e.g., checking it after each commit is written in git log, and
     aborting if we saw a write error). This is less cumbersome, but it
     does mean that errno may or may not still be accurate by the time
     we notice the error. So it's hard to reliably differentiate EPIPE
     from other errors. It would be nice, for example, to have git log
     exit silently on EPIPE, but properly print an error for something
     like ENOSPC. But perhaps that isn't a big deal, as I believe right
     now that we would silently ignore something like ENOSPC.

Less robust than that is to just ignore SIGPIPE in most git programs
(which don't benefit from it, and where it is only a liability), but
then manually enable it for the few that care (the log family, and
perhaps diff. Maybe things like "git tag -l", though that output tends
to be pretty small). But I think it would work OK in practice, because
those commands don't tend to make pipes other than the "main" output.
And it's quite easy to implement.

> So you want to let SIGPIPE silently kill us when the pager is in use; is
> that it?

That's an OK heuristic, as the pager being in use is a good sign that we
will generate long, streaming output. It has two downsides, though. One
is that it suffers from the too-heavy hammer of the preceding paragraph.
The other is that it only catches when _we_ create the pipe. You would
also want to catch something like:

  git rev-list | head

and stop the traversal. So I think it is less about whether a pager is
in use and more about whether we are creating long output that would
benefit from being cut off early. The pager is an indicator of that, but
it's not a perfect one; I think we'd do better to mark those spots
manually (i.e., by re-enabling SIGPIPE in commands that we deem
appropriate).

-Peff