[Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via Qemu-devel]

This is v6 of a proposed patch set for fixing file ID collisions with 9pfs.

v5->v6:

  * Rebased to https://github.com/gkurz/qemu/commits/9p-next
    (SHA1 177fd3b6a8).

  * Replaced previous boolean option 'remap_inodes' by tertiary option
    'multidevs=remap|forbid|warn', where 'warn' is the new/old default
    behaviour for not breaking existing installations:
    https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg07098.html

  * Dropped incomplete fix in v9fs_do_readdir() which aimed to prevent
    exposing info outside export root with '..' entry. Postponed this
    fix for now for the reasons described:
    https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01862.html

Christian Schoenebeck (4):
  9p: Treat multiple devices on one export as an error
  9p: Added virtfs option 'multidevs=remap|forbid|warn'
  9p: stat_to_qid: implement slow path
  9p: Use variable length suffixes for inode remapping

 fsdev/file-op-9p.h      |   5 +
 fsdev/qemu-fsdev-opts.c |   7 +-
 fsdev/qemu-fsdev.c      |  11 ++
 hw/9pfs/9p.c            | 488 +++++++++++++++++++++++++++++++++++++++++++++---
 hw/9pfs/9p.h            |  51 +++++
 qemu-options.hx         |  33 +++-
 vl.c                    |   6 +-
 7 files changed, 565 insertions(+), 36 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v6 1/4] 9p: Treat multiple devices on one export as an error

[Christian Schoenebeck via Qemu-devel]

The QID path should uniquely identify a file. However, the
inode of a file is currently used as the QID path, which
on its own only uniquely identifies files within a device.
Here we track the device hosting the 9pfs share, in order
to prevent security issues with QID path collisions from
other devices.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
[CS: - Assign dev_id to export root's device already in
       v9fs_device_realize_common(), not postponed in
       stat_to_qid().
     - error_report_once() if more than one device was
       shared by export.
     - Return -ENODEV instead of -ENOSYS in stat_to_qid().
     - Fixed typo in log comment. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++------------
 hw/9pfs/9p.h |  1 +
 2 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 586a6dccba..8cc65c2c67 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -572,10 +572,18 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_SOCKET)
 
 /* This is the algorithm from ufs in spfs */
-static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
+static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 {
     size_t size;
 
+    if (pdu->s->dev_id != stbuf->st_dev) {
+        error_report_once(
+            "9p: Multiple devices detected in same VirtFS export. "
+            "You must use a separate export for each device."
+        );
+        return -ENODEV;
+    }
+
     memset(&qidp->path, 0, sizeof(qidp->path));
     size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
     memcpy(&qidp->path, &stbuf->st_ino, size);
@@ -587,6 +595,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
     if (S_ISLNK(stbuf->st_mode)) {
         qidp->type |= P9_QID_TYPE_SYMLINK;
     }
+
+    return 0;
 }
 
 static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
@@ -599,7 +609,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
     if (err < 0) {
         return err;
     }
-    stat_to_qid(&stbuf, qidp);
+    err = stat_to_qid(pdu, &stbuf, qidp);
+    if (err < 0) {
+        return err;
+    }
     return 0;
 }
 
@@ -830,7 +843,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
 
     memset(v9stat, 0, sizeof(*v9stat));
 
-    stat_to_qid(stbuf, &v9stat->qid);
+    err = stat_to_qid(pdu, stbuf, &v9stat->qid);
+    if (err < 0) {
+        return err;
+    }
     v9stat->mode = stat_to_v9mode(stbuf);
     v9stat->atime = stbuf->st_atime;
     v9stat->mtime = stbuf->st_mtime;
@@ -891,7 +907,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
 #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
 
 
-static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
+static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
                                 V9fsStatDotl *v9lstat)
 {
     memset(v9lstat, 0, sizeof(*v9lstat));
@@ -913,7 +929,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
     /* Currently we only support BASIC fields in stat */
     v9lstat->st_result_mask = P9_STATS_BASIC;
 
-    stat_to_qid(stbuf, &v9lstat->qid);
+    return stat_to_qid(pdu, stbuf, &v9lstat->qid);
 }
 
 static void print_sg(struct iovec *sg, int cnt)
@@ -1115,7 +1131,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
     uint64_t request_mask;
     V9fsStatDotl v9stat_dotl;
     V9fsPDU *pdu = opaque;
-    V9fsState *s = pdu->s;
 
     retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
     if (retval < 0) {
@@ -1136,7 +1151,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
     if (retval < 0) {
         goto out;
     }
-    stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
+    retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
+    if (retval < 0) {
+        goto out;
+    }
 
     /*  fill st_gen if requested and supported by underlying fs */
     if (request_mask & P9_STATS_GEN) {
@@ -1381,7 +1399,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
             if (err < 0) {
                 goto out;
             }
-            stat_to_qid(&stbuf, &qid);
+            err = stat_to_qid(pdu, &stbuf, &qid);
+            if (err < 0) {
+                goto out;
+            }
             v9fs_path_copy(&dpath, &path);
         }
         memcpy(&qids[name_idx], &qid, sizeof(qid));
@@ -1483,7 +1504,10 @@ static void coroutine_fn v9fs_open(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     if (S_ISDIR(stbuf.st_mode)) {
         err = v9fs_co_opendir(pdu, fidp);
         if (err < 0) {
@@ -1593,7 +1617,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
         fidp->flags |= FID_NON_RECLAIMABLE;
     }
     iounit =  get_iounit(pdu, &fidp->path);
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     if (err < 0) {
         goto out;
@@ -2327,7 +2354,10 @@ static void coroutine_fn v9fs_create(void *opaque)
         }
     }
     iounit = get_iounit(pdu, &fidp->path);
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     if (err < 0) {
         goto out;
@@ -2384,7 +2414,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err =  pdu_marshal(pdu, offset, "Q", &qid);
     if (err < 0) {
         goto out;
@@ -3064,7 +3097,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Q", &qid);
     if (err < 0) {
         goto out;
@@ -3222,7 +3258,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Q", &qid);
     if (err < 0) {
         goto out;
@@ -3633,6 +3672,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
         goto out;
     }
 
+    s->dev_id = stat.st_dev;
+
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 8883761b2c..5e316178d5 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -256,6 +256,7 @@ struct V9fsState
     Error *migration_blocker;
     V9fsConf fsconf;
     V9fsQID root_qid;
+    dev_t dev_id;
 };
 
 /* 9p2000.L open flags */
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 1/4] 9p: Treat multiple devices on one export as an error

[Greg Kurz]

On Thu, 22 Aug 2019 21:28:19 +0200
Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> wrote:

> The QID path should uniquely identify a file. However, the
> inode of a file is currently used as the QID path, which
> on its own only uniquely identifies files within a device.
> Here we track the device hosting the 9pfs share, in order
> to prevent security issues with QID path collisions from
> other devices.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> [CS: - Assign dev_id to export root's device already in
>        v9fs_device_realize_common(), not postponed in
>        stat_to_qid().
>      - error_report_once() if more than one device was
>        shared by export.
>      - Return -ENODEV instead of -ENOSYS in stat_to_qid().
>      - Fixed typo in log comment. ]
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++------------
>  hw/9pfs/9p.h |  1 +
>  2 files changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 586a6dccba..8cc65c2c67 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -572,10 +572,18 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>                                  P9_STAT_MODE_SOCKET)
>  
>  /* This is the algorithm from ufs in spfs */
> -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>  {
>      size_t size;
>  
> +    if (pdu->s->dev_id != stbuf->st_dev) {
> +        error_report_once(
> +            "9p: Multiple devices detected in same VirtFS export. "
> +            "You must use a separate export for each device."
> +        );
> +        return -ENODEV;

As explained in the v5 review, we don't necessarily want to break existing
cross-device setups that just happen to work. Moreover, the next patch
re-allows them since remap_inodes=ignore is the default. I would thus
only do:

        warn_report_once(
                    "9p: Multiple devices detected in same VirtFS export, "
                    "which might lead to file ID collisions and severe "
                    "misbehaviours on guest! You should use a separate "
                    "export for each device shared from host."
        );

So I've just changed that and applied to 9p-next since it is
a valuable improvement. Note that I've kept the signature change
of stat_to_qid() for simplicity even if it isn't needed before
the next patch.

> +    }
> +
>      memset(&qidp->path, 0, sizeof(qidp->path));
>      size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
>      memcpy(&qidp->path, &stbuf->st_ino, size);
> @@ -587,6 +595,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
>      if (S_ISLNK(stbuf->st_mode)) {
>          qidp->type |= P9_QID_TYPE_SYMLINK;
>      }
> +
> +    return 0;
>  }
>  
>  static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> @@ -599,7 +609,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
>      if (err < 0) {
>          return err;
>      }
> -    stat_to_qid(&stbuf, qidp);
> +    err = stat_to_qid(pdu, &stbuf, qidp);
> +    if (err < 0) {
> +        return err;
> +    }
>      return 0;
>  }
>  
> @@ -830,7 +843,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>  
>      memset(v9stat, 0, sizeof(*v9stat));
>  
> -    stat_to_qid(stbuf, &v9stat->qid);
> +    err = stat_to_qid(pdu, stbuf, &v9stat->qid);
> +    if (err < 0) {
> +        return err;
> +    }
>      v9stat->mode = stat_to_v9mode(stbuf);
>      v9stat->atime = stbuf->st_atime;
>      v9stat->mtime = stbuf->st_mtime;
> @@ -891,7 +907,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>  #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
>  
>  
> -static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
> +static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>                                  V9fsStatDotl *v9lstat)
>  {
>      memset(v9lstat, 0, sizeof(*v9lstat));
> @@ -913,7 +929,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
>      /* Currently we only support BASIC fields in stat */
>      v9lstat->st_result_mask = P9_STATS_BASIC;
>  
> -    stat_to_qid(stbuf, &v9lstat->qid);
> +    return stat_to_qid(pdu, stbuf, &v9lstat->qid);
>  }
>  
>  static void print_sg(struct iovec *sg, int cnt)
> @@ -1115,7 +1131,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>      uint64_t request_mask;
>      V9fsStatDotl v9stat_dotl;
>      V9fsPDU *pdu = opaque;
> -    V9fsState *s = pdu->s;
>  
>      retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
>      if (retval < 0) {
> @@ -1136,7 +1151,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>      if (retval < 0) {
>          goto out;
>      }
> -    stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
> +    retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
> +    if (retval < 0) {
> +        goto out;
> +    }
>  
>      /*  fill st_gen if requested and supported by underlying fs */
>      if (request_mask & P9_STATS_GEN) {
> @@ -1381,7 +1399,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
>              if (err < 0) {
>                  goto out;
>              }
> -            stat_to_qid(&stbuf, &qid);
> +            err = stat_to_qid(pdu, &stbuf, &qid);
> +            if (err < 0) {
> +                goto out;
> +            }
>              v9fs_path_copy(&dpath, &path);
>          }
>          memcpy(&qids[name_idx], &qid, sizeof(qid));
> @@ -1483,7 +1504,10 @@ static void coroutine_fn v9fs_open(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      if (S_ISDIR(stbuf.st_mode)) {
>          err = v9fs_co_opendir(pdu, fidp);
>          if (err < 0) {
> @@ -1593,7 +1617,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
>          fidp->flags |= FID_NON_RECLAIMABLE;
>      }
>      iounit =  get_iounit(pdu, &fidp->path);
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
>      if (err < 0) {
>          goto out;
> @@ -2327,7 +2354,10 @@ static void coroutine_fn v9fs_create(void *opaque)
>          }
>      }
>      iounit = get_iounit(pdu, &fidp->path);
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
>      if (err < 0) {
>          goto out;
> @@ -2384,7 +2414,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err =  pdu_marshal(pdu, offset, "Q", &qid);
>      if (err < 0) {
>          goto out;
> @@ -3064,7 +3097,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Q", &qid);
>      if (err < 0) {
>          goto out;
> @@ -3222,7 +3258,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Q", &qid);
>      if (err < 0) {
>          goto out;
> @@ -3633,6 +3672,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>          goto out;
>      }
>  
> +    s->dev_id = stat.st_dev;
> +
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
>  
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 8883761b2c..5e316178d5 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -256,6 +256,7 @@ struct V9fsState
>      Error *migration_blocker;
>      V9fsConf fsconf;
>      V9fsQID root_qid;
> +    dev_t dev_id;
>  };
>  
>  /* 9p2000.L open flags */

Re: [Qemu-devel] [PATCH v6 1/4] 9p: Treat multiple devices on one export as an error

[Christian Schoenebeck via Qemu-devel]

On Donnerstag, 29. August 2019 18:27:30 CEST Greg Kurz wrote:
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 586a6dccba..8cc65c2c67 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -572,10 +572,18 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> > 
> >                                  P9_STAT_MODE_SOCKET)
> >  
> >  /* This is the algorithm from ufs in spfs */
> > 
> > -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> > +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID
> > *qidp)> 
> >  {
> >  
> >      size_t size;
> > 
> > +    if (pdu->s->dev_id != stbuf->st_dev) {
> > +        error_report_once(
> > +            "9p: Multiple devices detected in same VirtFS export. "
> > +            "You must use a separate export for each device."
> > +        );
> > +        return -ENODEV;
> 
> As explained in the v5 review, we don't necessarily want to break existing
> cross-device setups that just happen to work. Moreover, the next patch
> re-allows them since remap_inodes=ignore is the default. I would thus
> only do:
> 
>         warn_report_once(
>                     "9p: Multiple devices detected in same VirtFS export, "
>                     "which might lead to file ID collisions and severe "
>                     "misbehaviours on guest! You should use a separate "
>                     "export for each device shared from host."
>         );
> 
> So I've just changed that and applied to 9p-next since it is
> a valuable improvement. Note that I've kept the signature change
> of stat_to_qid() for simplicity even if it isn't needed before
> the next patch.

I'm fine with your changes.

Dropping "return -ENODEV" in this patch was beyond my default level for 
details since this is really just a very detailed sort of git history 
tweaking. :) Like you already pointed out, not aborting (as default behaviour) 
would have been addressed with the subsequent patch anyway.

[Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Christian Schoenebeck via Qemu-devel]

'warn' (default): Only log an error message (once) on host if more than one
device is shared by same export, except of that just ignore this config
error though. This is the default behaviour for not breaking existing
installations implying that they really know what they are doing.

'forbid': Like 'warn', but except of just logging an error this
also denies access of guest to additional devices.

'remap': Allows to share more than one device per export by remapping
inodes from host to guest appropriately. To support multiple devices on the
9p share, and avoid qid path collisions we take the device id as input to
generate a unique QID path. The lowest 48 bits of the path will be set
equal to the file inode, and the top bits will be uniquely assigned based
on the top 16 bits of the inode and the device id.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
[CS: - Rebased to https://github.com/gkurz/qemu/commits/9p-next
       (SHA1 177fd3b6a8).
     - Updated hash calls to new xxhash API.
     - Added virtfs option 'multidevs', original patch simply did the inode
       remapping without being asked.
     - Updated docs for new option 'multidevs'.
     - Capture root_ino in v9fs_device_realize_common() as well, not just
       the device id.
     - Fixed v9fs_do_readdir() not having remapped inodes.
     - Log error message when running out of prefixes in
       qid_path_prefixmap().
     - Fixed definition of QPATH_INO_MASK.
     - Dropped unnecessary parantheses in qpp_lookup_func().
     - Dropped unnecessary g_malloc0() result checks. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 fsdev/file-op-9p.h      |   5 ++
 fsdev/qemu-fsdev-opts.c |   7 +-
 fsdev/qemu-fsdev.c      |  11 +++
 hw/9pfs/9p.c            | 182 ++++++++++++++++++++++++++++++++++++++++++------
 hw/9pfs/9p.h            |  13 ++++
 qemu-options.hx         |  33 +++++++--
 vl.c                    |   6 +-
 7 files changed, 229 insertions(+), 28 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index c757c8099f..f2f7772c86 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -59,6 +59,11 @@ typedef struct ExtendedOps {
 #define V9FS_RDONLY                 0x00000040
 #define V9FS_PROXY_SOCK_FD          0x00000080
 #define V9FS_PROXY_SOCK_NAME        0x00000100
+/*
+ * multidevs option (either one of the two applies exclusively)
+ */
+#define V9FS_REMAP_INODES           0x00000200
+#define V9FS_FORBID_MULTIDEVS       0x00000400
 
 #define V9FS_SEC_MASK               0x0000003C
 
diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
index 7c31ffffaf..07a18c6e48 100644
--- a/fsdev/qemu-fsdev-opts.c
+++ b/fsdev/qemu-fsdev-opts.c
@@ -31,7 +31,9 @@ static QemuOptsList qemu_fsdev_opts = {
         }, {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
-
+        }, {
+            .name = "multidevs",
+            .type = QEMU_OPT_STRING,
         }, {
             .name = "socket",
             .type = QEMU_OPT_STRING,
@@ -76,6 +78,9 @@ static QemuOptsList qemu_virtfs_opts = {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
         }, {
+            .name = "multidevs",
+            .type = QEMU_OPT_STRING,
+        }, {
             .name = "socket",
             .type = QEMU_OPT_STRING,
         }, {
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 077a8c4e2b..ed03d559a9 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -58,6 +58,7 @@ static FsDriverTable FsDrivers[] = {
             "writeout",
             "fmode",
             "dmode",
+            "multidevs",
             "throttling.bps-total",
             "throttling.bps-read",
             "throttling.bps-write",
@@ -121,6 +122,7 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
     const char *fsdev_id = qemu_opts_id(opts);
     const char *fsdriver = qemu_opt_get(opts, "fsdriver");
     const char *writeout = qemu_opt_get(opts, "writeout");
+    const char *multidevs = qemu_opt_get(opts, "multidevs");
     bool ro = qemu_opt_get_bool(opts, "readonly", 0);
 
     if (!fsdev_id) {
@@ -161,6 +163,15 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
     } else {
         fsle->fse.export_flags &= ~V9FS_RDONLY;
     }
+    if (multidevs) {
+        if (!strcmp(multidevs, "remap")) {
+            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
+            fsle->fse.export_flags |= V9FS_REMAP_INODES;
+        } else if (!strcmp(multidevs, "forbid")) {
+            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
+            fsle->fse.export_flags |= V9FS_FORBID_MULTIDEVS;
+        }
+    }
 
     if (fsle->fse.ops->parse_opts) {
         if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8cc65c2c67..c96ea51116 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -25,6 +25,7 @@
 #include "trace.h"
 #include "migration/blocker.h"
 #include "sysemu/qtest.h"
+#include "qemu/xxhash.h"
 
 int open_fd_hw;
 int total_open_fd;
@@ -571,22 +572,109 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_NAMED_PIPE |   \
                                 P9_STAT_MODE_SOCKET)
 
-/* This is the algorithm from ufs in spfs */
+
+/* creative abuse of tb_hash_func7, which is based on xxhash */
+static uint32_t qpp_hash(QppEntry e)
+{
+    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
+}
+
+static bool qpp_lookup_func(const void *obj, const void *userp)
+{
+    const QppEntry *e1 = obj, *e2 = userp;
+    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
+}
+
+static void qpp_table_remove(void *p, uint32_t h, void *up)
+{
+    g_free(p);
+}
+
+static void qpp_table_destroy(struct qht *ht)
+{
+    qht_iter(ht, qpp_table_remove, NULL);
+    qht_destroy(ht);
+}
+
+/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
+ * to a unique QID path (64 bits). To avoid having to map and keep track
+ * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
+ * the device id to the 16 highest bits of the QID path. The 48 lowest bits
+ * of the QID path equal to the lowest bits of the inode number.
+ *
+ * This takes advantage of the fact that inode number are usually not
+ * random but allocated sequentially, so we have fewer items to keep
+ * track of.
+ */
+static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
+                                uint64_t *path)
+{
+    QppEntry lookup = {
+        .dev = stbuf->st_dev,
+        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
+    }, *val;
+    uint32_t hash = qpp_hash(lookup);
+
+    val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
+
+    if (!val) {
+        if (pdu->s->qp_prefix_next == 0) {
+            /* we ran out of prefixes */
+            error_report_once(
+                "9p: No more prefixes available for remapping inodes from "
+                "host to guest."
+            );
+            return -ENFILE;
+        }
+
+        val = g_malloc0(sizeof(QppEntry));
+        *val = lookup;
+
+        /* new unique inode prefix and device combo */
+        val->qp_prefix = pdu->s->qp_prefix_next++;
+        qht_insert(&pdu->s->qpp_table, val, hash, NULL);
+    }
+
+    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+    return 0;
+}
+
 static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 {
+    int err;
     size_t size;
 
-    if (pdu->s->dev_id != stbuf->st_dev) {
-        error_report_once(
-            "9p: Multiple devices detected in same VirtFS export. "
-            "You must use a separate export for each device."
-        );
-        return -ENODEV;
+    if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
+        /* map inode+device to qid path (fast path) */
+        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+        if (err) {
+            return err;
+        }
+    } else {
+        if (pdu->s->dev_id != stbuf->st_dev) {
+            if (pdu->s->ctx.export_flags & V9FS_FORBID_MULTIDEVS) {
+                error_report_once(
+                    "9p: Multiple devices detected in same VirtFS export. "
+                    "Access of guest to additional devices is (partly) "
+                    "denied due to virtfs option 'multidevs=forbid' being "
+                    "effective."
+                );
+                return -ENODEV;
+            } else {
+                error_report_once(
+                    "9p: Multiple devices detected in same VirtFS export, "
+                    "which might lead to file ID collisions and severe "
+                    "misbehaviours on guest! You should either use a "
+                    "separate export for each device shared from host or "
+                    "use virtfs option 'multidevs=remap'!"
+                );
+            }
+        }
+        memset(&qidp->path, 0, sizeof(qidp->path));
+        size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
+        memcpy(&qidp->path, &stbuf->st_ino, size);
     }
 
-    memset(&qidp->path, 0, sizeof(qidp->path));
-    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
-    memcpy(&qidp->path, &stbuf->st_ino, size);
     qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
     qidp->type = 0;
     if (S_ISDIR(stbuf->st_mode)) {
@@ -616,6 +704,30 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
     return 0;
 }
 
+static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
+                                      struct dirent *dent, V9fsQID *qidp)
+{
+    struct stat stbuf;
+    V9fsPath path;
+    int err;
+
+    v9fs_path_init(&path);
+
+    err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
+    if (err < 0) {
+        goto out;
+    }
+    err = v9fs_co_lstat(pdu, &path, &stbuf);
+    if (err < 0) {
+        goto out;
+    }
+    err = stat_to_qid(pdu, &stbuf, qidp);
+
+out:
+    v9fs_path_free(&path);
+    return err;
+}
+
 V9fsPDU *pdu_alloc(V9fsState *s)
 {
     V9fsPDU *pdu = NULL;
@@ -1964,16 +2076,39 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
             v9fs_string_free(&name);
             return count;
         }
-        /*
-         * Fill up just the path field of qid because the client uses
-         * only that. To fill the entire qid structure we will have
-         * to stat each dirent found, which is expensive
-         */
-        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
-        memcpy(&qid.path, &dent->d_ino, size);
-        /* Fill the other fields with dummy values */
-        qid.type = 0;
-        qid.version = 0;
+
+        if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
+            /*
+             * dirent_to_qid() implies expensive stat call for each entry,
+             * we must do that here though since inode remapping requires
+             * the device id, which in turn might be different for
+             * different entries; we cannot make any assumption to avoid
+             * that here.
+             */
+            err = dirent_to_qid(pdu, fidp, dent, &qid);
+            if (err < 0) {
+                v9fs_readdir_unlock(&fidp->fs.dir);
+                v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
+                v9fs_string_free(&name);
+                return err;
+            }
+        } else {
+            /*
+             * Fill up just the path field of qid because the client uses
+             * only that. To fill the entire qid structure we will have
+             * to stat each dirent found, which is expensive. For the
+             * latter reason we don't call dirent_to_qid() here. Only drawback
+             * is that no multi-device export detection of stat_to_qid()
+             * would be done and provided as error to the user here. But
+             * user would get that error anyway when accessing those
+             * files/dirs through other ways.
+             */
+            size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
+            memcpy(&qid.path, &dent->d_ino, size);
+            /* Fill the other fields with dummy values */
+            qid.type = 0;
+            qid.version = 0;
+        }
 
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
@@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
         goto out;
     }
 
+    s->root_ino = stat.st_ino;
     s->dev_id = stat.st_dev;
 
+    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
+    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
+    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
 
@@ -3687,6 +3827,7 @@ out:
         }
         g_free(s->tag);
         g_free(s->ctx.fs_root);
+        qpp_table_destroy(&s->qpp_table);
         v9fs_path_free(&path);
     }
     return rc;
@@ -3699,6 +3840,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
     }
     fsdev_throttle_cleanup(s->ctx.fst);
     g_free(s->tag);
+    qpp_table_destroy(&s->qpp_table);
     g_free(s->ctx.fs_root);
 }
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 5e316178d5..a283b0193e 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -8,6 +8,7 @@
 #include "fsdev/9p-iov-marshal.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
+#include "qemu/qht.h"
 
 enum {
     P9_TLERROR = 6,
@@ -235,6 +236,15 @@ struct V9fsFidState
     V9fsFidState *rclm_lst;
 };
 
+#define QPATH_INO_MASK        ((1ULL << 48) - 1)
+
+/* QID path prefix entry, see stat_to_qid */
+typedef struct {
+    dev_t dev;
+    uint16_t ino_prefix;
+    uint16_t qp_prefix;
+} QppEntry;
+
 struct V9fsState
 {
     QLIST_HEAD(, V9fsPDU) free_list;
@@ -256,7 +266,10 @@ struct V9fsState
     Error *migration_blocker;
     V9fsConf fsconf;
     V9fsQID root_qid;
+    ino_t root_ino;
     dev_t dev_id;
+    struct qht qpp_table;
+    uint16_t qp_prefix_next;
 };
 
 /* 9p2000.L open flags */
diff --git a/qemu-options.hx b/qemu-options.hx
index 9621e934c0..603e5e8e15 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1335,17 +1335,17 @@ ETEXI
 
 DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
     "-virtfs local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-file|passthrough|none\n"
-    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
-    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]\n"
-    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly]\n"
+    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode][,multidevs=remap|forbid|warn]\n"
+    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly][,multidevs=remap|forbid|warn]\n"
+    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly][,multidevs=remap|forbid|warn]\n"
     "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
     QEMU_ARCH_ALL)
 
 STEXI
 
-@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}]
-@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
-@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
+@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}][,multidevs=@var{multidevs}]
+@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
+@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
 @itemx -virtfs synth,mount_tag=@var{mount_tag}
 @findex -virtfs
 
@@ -1399,6 +1399,27 @@ Specifies the default mode for newly created directories on the host. Works
 only with security models "mapped-xattr" and "mapped-file".
 @item mount_tag=@var{mount_tag}
 Specifies the tag name to be used by the guest to mount this export point.
+@item multidevs=@var{multidevs}
+Specifies how to deal with multiple devices being shared with a 9p export.
+Supported behaviours are either "remap", "forbid" or "warn". The latter is
+the default behaviour on which virtfs 9p expects only one device to be
+shared with the same export, and if more than one device is shared and
+accessed via the same 9p export then only a warning message is logged
+(once) by qemu on host side. In order to avoid file ID collisions on guest
+you should either create a separate virtfs export for each device to be
+shared with guests (recommended way) or you might use "remap" instead which
+allows you to share multiple devices with only one export instead, which is
+achieved by remapping the original inode numbers from host to guest in a
+way that would prevent such collisions. Remapping inodes in such use cases
+is required because the original device IDs from host are never passed and
+exposed on guest. Instead all files of an export shared with virtfs always
+share the same device id on guest. So two files with identical inode
+numbers but from actually different devices on host would otherwise cause a
+file ID collision and hence potential misbehaviours on guest. "forbid" on
+the other hand assumes like "warn" that only one device is shared by the
+same export, however it will not only log a warning message but also
+deny access to additional devices on guest. Note though that "forbid" does
+currently not block all possible file access operations.
 @end table
 ETEXI
 
diff --git a/vl.c b/vl.c
index b426b32134..9cb29b483d 100644
--- a/vl.c
+++ b/vl.c
@@ -3320,7 +3320,7 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_virtfs: {
                 QemuOpts *fsdev;
                 QemuOpts *device;
-                const char *writeout, *sock_fd, *socket, *path, *security_model;
+                const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;
 
                 olist = qemu_find_opts("virtfs");
                 if (!olist) {
@@ -3380,6 +3380,10 @@ int main(int argc, char **argv, char **envp)
                 qemu_opt_set_bool(fsdev, "readonly",
                                   qemu_opt_get_bool(opts, "readonly", 0),
                                   &error_abort);
+                multidevs = qemu_opt_get(opts, "multidevs");
+                if (multidevs) {
+                    qemu_opt_set(fsdev, "multidevs", multidevs, &error_abort);
+                }
                 device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                           &error_abort);
                 qemu_opt_set(device, "driver", "virtio-9p-pci", &error_abort);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Greg Kurz]

On Thu, 22 Aug 2019 21:33:37 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> 'warn' (default): Only log an error message (once) on host if more than one
> device is shared by same export, except of that just ignore this config
> error though. This is the default behaviour for not breaking existing
> installations implying that they really know what they are doing.
> 
> 'forbid': Like 'warn', but except of just logging an error this
> also denies access of guest to additional devices.
> 
> 'remap': Allows to share more than one device per export by remapping
> inodes from host to guest appropriately. To support multiple devices on the
> 9p share, and avoid qid path collisions we take the device id as input to
> generate a unique QID path. The lowest 48 bits of the path will be set
> equal to the file inode, and the top bits will be uniquely assigned based
> on the top 16 bits of the inode and the device id.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> [CS: - Rebased to https://github.com/gkurz/qemu/commits/9p-next
>        (SHA1 177fd3b6a8).
>      - Updated hash calls to new xxhash API.
>      - Added virtfs option 'multidevs', original patch simply did the inode
>        remapping without being asked.
>      - Updated docs for new option 'multidevs'.
>      - Capture root_ino in v9fs_device_realize_common() as well, not just
>        the device id.
>      - Fixed v9fs_do_readdir() not having remapped inodes.
>      - Log error message when running out of prefixes in
>        qid_path_prefixmap().
>      - Fixed definition of QPATH_INO_MASK.
>      - Dropped unnecessary parantheses in qpp_lookup_func().
>      - Dropped unnecessary g_malloc0() result checks. ]
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Looks good but needs some more polishing. See below.

>  fsdev/file-op-9p.h      |   5 ++
>  fsdev/qemu-fsdev-opts.c |   7 +-
>  fsdev/qemu-fsdev.c      |  11 +++
>  hw/9pfs/9p.c            | 182 ++++++++++++++++++++++++++++++++++++++++++------
>  hw/9pfs/9p.h            |  13 ++++
>  qemu-options.hx         |  33 +++++++--
>  vl.c                    |   6 +-
>  7 files changed, 229 insertions(+), 28 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index c757c8099f..f2f7772c86 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -59,6 +59,11 @@ typedef struct ExtendedOps {
>  #define V9FS_RDONLY                 0x00000040
>  #define V9FS_PROXY_SOCK_FD          0x00000080
>  #define V9FS_PROXY_SOCK_NAME        0x00000100
> +/*
> + * multidevs option (either one of the two applies exclusively)
> + */
> +#define V9FS_REMAP_INODES           0x00000200
> +#define V9FS_FORBID_MULTIDEVS       0x00000400
>  
>  #define V9FS_SEC_MASK               0x0000003C
>  
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> index 7c31ffffaf..07a18c6e48 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -31,7 +31,9 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "readonly",
>              .type = QEMU_OPT_BOOL,
> -
> +        }, {
> +            .name = "multidevs",
> +            .type = QEMU_OPT_STRING,
>          }, {
>              .name = "socket",
>              .type = QEMU_OPT_STRING,
> @@ -76,6 +78,9 @@ static QemuOptsList qemu_virtfs_opts = {
>              .name = "readonly",
>              .type = QEMU_OPT_BOOL,
>          }, {
> +            .name = "multidevs",
> +            .type = QEMU_OPT_STRING,
> +        }, {
>              .name = "socket",
>              .type = QEMU_OPT_STRING,
>          }, {
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index 077a8c4e2b..ed03d559a9 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -58,6 +58,7 @@ static FsDriverTable FsDrivers[] = {
>              "writeout",
>              "fmode",
>              "dmode",
> +            "multidevs",

So we only allow this for the "local" backend. Any reason not to
add this to "proxy" as well ?

I didn't do it for the "throttling" options because it is a
feature I didn't care to support much, but "multidevs" is more
a fix than a fancy feature.

>              "throttling.bps-total",
>              "throttling.bps-read",
>              "throttling.bps-write",
> @@ -121,6 +122,7 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
>      const char *fsdev_id = qemu_opts_id(opts);
>      const char *fsdriver = qemu_opt_get(opts, "fsdriver");
>      const char *writeout = qemu_opt_get(opts, "writeout");
> +    const char *multidevs = qemu_opt_get(opts, "multidevs");
>      bool ro = qemu_opt_get_bool(opts, "readonly", 0);
>  
>      if (!fsdev_id) {
> @@ -161,6 +163,15 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
>      } else {
>          fsle->fse.export_flags &= ~V9FS_RDONLY;
>      }
> +    if (multidevs) {
> +        if (!strcmp(multidevs, "remap")) {
> +            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
> +            fsle->fse.export_flags |= V9FS_REMAP_INODES;
> +        } else if (!strcmp(multidevs, "forbid")) {
> +            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
> +            fsle->fse.export_flags |= V9FS_FORBID_MULTIDEVS;
> +        }

And...

        } else if (!strcmp(multidevs, "warn")) {
            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
        } else {
            error_setg(errp, "invalid multidevs property '%s'", multidevs);
            return -1;
        }

... because we're a bit pedantic for command line options :)

> +    }
>  
>      if (fsle->fse.ops->parse_opts) {
>          if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8cc65c2c67..c96ea51116 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -25,6 +25,7 @@
>  #include "trace.h"
>  #include "migration/blocker.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/xxhash.h"
>  
>  int open_fd_hw;
>  int total_open_fd;
> @@ -571,22 +572,109 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>                                  P9_STAT_MODE_NAMED_PIPE |   \
>                                  P9_STAT_MODE_SOCKET)
>  
> -/* This is the algorithm from ufs in spfs */
> +
> +/* creative abuse of tb_hash_func7, which is based on xxhash */
> +static uint32_t qpp_hash(QppEntry e)
> +{
> +    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
> +}
> +
> +static bool qpp_lookup_func(const void *obj, const void *userp)
> +{
> +    const QppEntry *e1 = obj, *e2 = userp;
> +    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> +}
> +
> +static void qpp_table_remove(void *p, uint32_t h, void *up)
> +{
> +    g_free(p);
> +}
> +
> +static void qpp_table_destroy(struct qht *ht)
> +{
> +    qht_iter(ht, qpp_table_remove, NULL);
> +    qht_destroy(ht);
> +}

Ok to have a function for this instead of open-coding but I'd
like to see qpp_table_init() for consistency.

> +
> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> + * to a unique QID path (64 bits). To avoid having to map and keep track
> + * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> + * the device id to the 16 highest bits of the QID path. The 48 lowest bits
> + * of the QID path equal to the lowest bits of the inode number.
> + *
> + * This takes advantage of the fact that inode number are usually not
> + * random but allocated sequentially, so we have fewer items to keep
> + * track of.
> + */
> +static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
> +                                uint64_t *path)
> +{
> +    QppEntry lookup = {
> +        .dev = stbuf->st_dev,
> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
> +    }, *val;
> +    uint32_t hash = qpp_hash(lookup);
> +
> +    val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
> +
> +    if (!val) {
> +        if (pdu->s->qp_prefix_next == 0) {
> +            /* we ran out of prefixes */
> +            error_report_once(
> +                "9p: No more prefixes available for remapping inodes from "
> +                "host to guest."
> +            );
> +            return -ENFILE;
> +        }
> +
> +        val = g_malloc0(sizeof(QppEntry));
> +        *val = lookup;
> +
> +        /* new unique inode prefix and device combo */
> +        val->qp_prefix = pdu->s->qp_prefix_next++;
> +        qht_insert(&pdu->s->qpp_table, val, hash, NULL);
> +    }
> +
> +    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
> +    return 0;
> +}
> +
>  static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>  {
> +    int err;
>      size_t size;
>  
> -    if (pdu->s->dev_id != stbuf->st_dev) {
> -        error_report_once(
> -            "9p: Multiple devices detected in same VirtFS export. "
> -            "You must use a separate export for each device."
> -        );
> -        return -ENODEV;
> +    if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> +        /* map inode+device to qid path (fast path) */
> +        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> +        if (err) {
> +            return err;
> +        }
> +    } else {
> +        if (pdu->s->dev_id != stbuf->st_dev) {
> +            if (pdu->s->ctx.export_flags & V9FS_FORBID_MULTIDEVS) {
> +                error_report_once(
> +                    "9p: Multiple devices detected in same VirtFS export. "
> +                    "Access of guest to additional devices is (partly) "
> +                    "denied due to virtfs option 'multidevs=forbid' being "
> +                    "effective."
> +                );
> +                return -ENODEV;
> +            } else {
> +                error_report_once(

Please use warn_report_once().

> +                    "9p: Multiple devices detected in same VirtFS export, "
> +                    "which might lead to file ID collisions and severe "
> +                    "misbehaviours on guest! You should either use a "
> +                    "separate export for each device shared from host or "
> +                    "use virtfs option 'multidevs=remap'!"
> +                );
> +            }
> +        }
> +        memset(&qidp->path, 0, sizeof(qidp->path));
> +        size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> +        memcpy(&qidp->path, &stbuf->st_ino, size);
>      }
>  
> -    memset(&qidp->path, 0, sizeof(qidp->path));
> -    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> -    memcpy(&qidp->path, &stbuf->st_ino, size);
>      qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
>      qidp->type = 0;
>      if (S_ISDIR(stbuf->st_mode)) {
> @@ -616,6 +704,30 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
>      return 0;
>  }
>  
> +static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> +                                      struct dirent *dent, V9fsQID *qidp)
> +{
> +    struct stat stbuf;
> +    V9fsPath path;
> +    int err;
> +
> +    v9fs_path_init(&path);
> +
> +    err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
> +    if (err < 0) {
> +        goto out;
> +    }
> +    err = v9fs_co_lstat(pdu, &path, &stbuf);
> +    if (err < 0) {
> +        goto out;
> +    }
> +    err = stat_to_qid(pdu, &stbuf, qidp);
> +
> +out:
> +    v9fs_path_free(&path);
> +    return err;
> +}
> +
>  V9fsPDU *pdu_alloc(V9fsState *s)
>  {
>      V9fsPDU *pdu = NULL;
> @@ -1964,16 +2076,39 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>              v9fs_string_free(&name);
>              return count;
>          }
> -        /*
> -         * Fill up just the path field of qid because the client uses
> -         * only that. To fill the entire qid structure we will have
> -         * to stat each dirent found, which is expensive
> -         */
> -        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> -        memcpy(&qid.path, &dent->d_ino, size);
> -        /* Fill the other fields with dummy values */
> -        qid.type = 0;
> -        qid.version = 0;
> +
> +        if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> +            /*
> +             * dirent_to_qid() implies expensive stat call for each entry,
> +             * we must do that here though since inode remapping requires
> +             * the device id, which in turn might be different for
> +             * different entries; we cannot make any assumption to avoid
> +             * that here.
> +             */
> +            err = dirent_to_qid(pdu, fidp, dent, &qid);
> +            if (err < 0) {
> +                v9fs_readdir_unlock(&fidp->fs.dir);
> +                v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> +                v9fs_string_free(&name);
> +                return err;
> +            }
> +        } else {
> +            /*
> +             * Fill up just the path field of qid because the client uses
> +             * only that. To fill the entire qid structure we will have
> +             * to stat each dirent found, which is expensive. For the
> +             * latter reason we don't call dirent_to_qid() here. Only drawback
> +             * is that no multi-device export detection of stat_to_qid()
> +             * would be done and provided as error to the user here. But
> +             * user would get that error anyway when accessing those
> +             * files/dirs through other ways.
> +             */
> +            size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> +            memcpy(&qid.path, &dent->d_ino, size);
> +            /* Fill the other fields with dummy values */
> +            qid.type = 0;
> +            qid.version = 0;
> +        }
>  
>          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>          goto out;
>      }
>  
> +    s->root_ino = stat.st_ino;
>      s->dev_id = stat.st_dev;
>  
> +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> +    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
> +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> +
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
>  
> @@ -3687,6 +3827,7 @@ out:
>          }
>          g_free(s->tag);
>          g_free(s->ctx.fs_root);
> +        qpp_table_destroy(&s->qpp_table);

This causes QEMU to crash if we get there before qht_init() was called.
This should be guarded by a s->qpp_table.map != NULL check.

I've just posted a patch that simplifies error handling in this
function, you in Cc. The patch is also in 9p-next. Please rebase
on top of it.

>          v9fs_path_free(&path);
>      }
>      return rc;
> @@ -3699,6 +3840,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>      }
>      fsdev_throttle_cleanup(s->ctx.fst);
>      g_free(s->tag);
> +    qpp_table_destroy(&s->qpp_table);
>      g_free(s->ctx.fs_root);
>  }
>  
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 5e316178d5..a283b0193e 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -8,6 +8,7 @@
>  #include "fsdev/9p-iov-marshal.h"
>  #include "qemu/thread.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/qht.h"
>  
>  enum {
>      P9_TLERROR = 6,
> @@ -235,6 +236,15 @@ struct V9fsFidState
>      V9fsFidState *rclm_lst;
>  };
>  
> +#define QPATH_INO_MASK        ((1ULL << 48) - 1)
> +
> +/* QID path prefix entry, see stat_to_qid */
> +typedef struct {
> +    dev_t dev;
> +    uint16_t ino_prefix;
> +    uint16_t qp_prefix;
> +} QppEntry;
> +
>  struct V9fsState
>  {
>      QLIST_HEAD(, V9fsPDU) free_list;
> @@ -256,7 +266,10 @@ struct V9fsState
>      Error *migration_blocker;
>      V9fsConf fsconf;
>      V9fsQID root_qid;
> +    ino_t root_ino;
>      dev_t dev_id;
> +    struct qht qpp_table;
> +    uint16_t qp_prefix_next;
>  };
>  
>  /* 9p2000.L open flags */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9621e934c0..603e5e8e15 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1335,17 +1335,17 @@ ETEXI
>  
>  DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
>      "-virtfs local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-file|passthrough|none\n"
> -    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
> -    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]\n"
> -    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly]\n"
> +    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode][,multidevs=remap|forbid|warn]\n"
> +    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly][,multidevs=remap|forbid|warn]\n"
> +    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly][,multidevs=remap|forbid|warn]\n"

Either enable support in "proxy" or don't update the "proxy" documentation :)

>      "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
>      QEMU_ARCH_ALL)
>  
>  STEXI
>  
> -@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}]
> -@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
> -@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
> +@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}][,multidevs=@var{multidevs}]
> +@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
> +@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]

Ditto.

>  @itemx -virtfs synth,mount_tag=@var{mount_tag}
>  @findex -virtfs
>  
> @@ -1399,6 +1399,27 @@ Specifies the default mode for newly created directories on the host. Works
>  only with security models "mapped-xattr" and "mapped-file".
>  @item mount_tag=@var{mount_tag}
>  Specifies the tag name to be used by the guest to mount this export point.
> +@item multidevs=@var{multidevs}
> +Specifies how to deal with multiple devices being shared with a 9p export.
> +Supported behaviours are either "remap", "forbid" or "warn". The latter is
> +the default behaviour on which virtfs 9p expects only one device to be
> +shared with the same export, and if more than one device is shared and
> +accessed via the same 9p export then only a warning message is logged
> +(once) by qemu on host side. In order to avoid file ID collisions on guest
> +you should either create a separate virtfs export for each device to be
> +shared with guests (recommended way) or you might use "remap" instead which
> +allows you to share multiple devices with only one export instead, which is
> +achieved by remapping the original inode numbers from host to guest in a
> +way that would prevent such collisions. Remapping inodes in such use cases
> +is required because the original device IDs from host are never passed and
> +exposed on guest. Instead all files of an export shared with virtfs always
> +share the same device id on guest. So two files with identical inode
> +numbers but from actually different devices on host would otherwise cause a
> +file ID collision and hence potential misbehaviours on guest. "forbid" on
> +the other hand assumes like "warn" that only one device is shared by the
> +same export, however it will not only log a warning message but also
> +deny access to additional devices on guest. Note though that "forbid" does
> +currently not block all possible file access operations.

Maybe provide a list of such operations that won't be blocked ?

>  @end table
>  ETEXI
>  
> diff --git a/vl.c b/vl.c
> index b426b32134..9cb29b483d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3320,7 +3320,7 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_virtfs: {
>                  QemuOpts *fsdev;
>                  QemuOpts *device;
> -                const char *writeout, *sock_fd, *socket, *path, *security_model;
> +                const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;
>  
>                  olist = qemu_find_opts("virtfs");
>                  if (!olist) {
> @@ -3380,6 +3380,10 @@ int main(int argc, char **argv, char **envp)
>                  qemu_opt_set_bool(fsdev, "readonly",
>                                    qemu_opt_get_bool(opts, "readonly", 0),
>                                    &error_abort);
> +                multidevs = qemu_opt_get(opts, "multidevs");
> +                if (multidevs) {
> +                    qemu_opt_set(fsdev, "multidevs", multidevs, &error_abort);
> +                }
>                  device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
>                                            &error_abort);
>                  qemu_opt_set(device, "driver", "virtio-9p-pci", &error_abort);

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Christian Schoenebeck via Qemu-devel]

On Donnerstag, 29. August 2019 18:55:28 CEST Greg Kurz wrote:
> > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> > index 7c31ffffaf..07a18c6e48 100644
> > --- a/fsdev/qemu-fsdev-opts.c
> > +++ b/fsdev/qemu-fsdev-opts.c
> > @@ -31,7 +31,9 @@ static QemuOptsList qemu_fsdev_opts = {
> > 
> >          }, {
> >          
> >              .name = "readonly",
> >              .type = QEMU_OPT_BOOL,
> > 
> > -
> > +        }, {
> > +            .name = "multidevs",
> > +            .type = QEMU_OPT_STRING,
> > 
> >          }, {
> >          
> >              .name = "socket",
> >              .type = QEMU_OPT_STRING,
> > 
> > @@ -76,6 +78,9 @@ static QemuOptsList qemu_virtfs_opts = {
> > 
> >              .name = "readonly",
> >              .type = QEMU_OPT_BOOL,
> >          
> >          }, {
> > 
> > +            .name = "multidevs",
> > +            .type = QEMU_OPT_STRING,
> > +        }, {
> > 
> >              .name = "socket",
> >              .type = QEMU_OPT_STRING,
> >          
> >          }, {
> > 
> > diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> > index 077a8c4e2b..ed03d559a9 100644
> > --- a/fsdev/qemu-fsdev.c
> > +++ b/fsdev/qemu-fsdev.c
> > @@ -58,6 +58,7 @@ static FsDriverTable FsDrivers[] = {
> > 
> >              "writeout",
> >              "fmode",
> >              "dmode",
> > 
> > +            "multidevs",
> 
> So we only allow this for the "local" backend. Any reason not to
> add this to "proxy" as well ?
> 
> I didn't do it for the "throttling" options because it is a
> feature I didn't care to support much, but "multidevs" is more
> a fix than a fancy feature.

Well, to be honest I haven't cared about the proxy backend at all. Like I 
mentioned before, I am a bit sceptical that the proxy feature is actually used 
by people in real life at all (at least in its current implementation). So 
personally I don't see much sense in investing time for adding & testing new 
things on this backend ATM.

> > +    if (multidevs) {
> > +        if (!strcmp(multidevs, "remap")) {
> > +            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
> > +            fsle->fse.export_flags |= V9FS_REMAP_INODES;
> > +        } else if (!strcmp(multidevs, "forbid")) {
> > +            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
> > +            fsle->fse.export_flags |= V9FS_FORBID_MULTIDEVS;
> > +        }
> 
> And...
> 
>         } else if (!strcmp(multidevs, "warn")) {
>             fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
>             fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
>         } else {
>             error_setg(errp, "invalid multidevs property '%s'", multidevs);
>             return -1;
>         }
> 
> ... because we're a bit pedantic for command line options :)

And I thought I adapted to relaxed handling of CLI arguments. See existing 
option "writeout".  :)

> 
> > +    }
> > 
> >      if (fsle->fse.ops->parse_opts) {
> >      
> >          if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 8cc65c2c67..c96ea51116 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include "trace.h"
> >  #include "migration/blocker.h"
> >  #include "sysemu/qtest.h"
> > 
> > +#include "qemu/xxhash.h"
> > 
> >  int open_fd_hw;
> >  int total_open_fd;
> > 
> > @@ -571,22 +572,109 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> > 
> >                                  P9_STAT_MODE_NAMED_PIPE |   \
> >                                  P9_STAT_MODE_SOCKET)
> > 
> > -/* This is the algorithm from ufs in spfs */
> > +
> > +/* creative abuse of tb_hash_func7, which is based on xxhash */
> > +static uint32_t qpp_hash(QppEntry e)
> > +{
> > +    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
> > +}
> > +
> > +static bool qpp_lookup_func(const void *obj, const void *userp)
> > +{
> > +    const QppEntry *e1 = obj, *e2 = userp;
> > +    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> > +}
> > +
> > +static void qpp_table_remove(void *p, uint32_t h, void *up)
> > +{
> > +    g_free(p);
> > +}
> > +
> > +static void qpp_table_destroy(struct qht *ht)
> > +{
> > +    qht_iter(ht, qpp_table_remove, NULL);
> > +    qht_destroy(ht);
> > +}
> 
> Ok to have a function for this instead of open-coding but I'd
> like to see qpp_table_init() for consistency.

Well, these are just qht_init() one-liners, but if you really want to have 
dedicated, local init functions for them, okay.

> >  static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID
> >  *qidp) {
> > 
> > +    int err;
> > 
> >      size_t size;
> > 
> > -    if (pdu->s->dev_id != stbuf->st_dev) {
> > -        error_report_once(
> > -            "9p: Multiple devices detected in same VirtFS export. "
> > -            "You must use a separate export for each device."
> > -        );
> > -        return -ENODEV;
> > +    if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> > +        /* map inode+device to qid path (fast path) */
> > +        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> > +        if (err) {
> > +            return err;
> > +        }
> > +    } else {
> > +        if (pdu->s->dev_id != stbuf->st_dev) {
> > +            if (pdu->s->ctx.export_flags & V9FS_FORBID_MULTIDEVS) {
> > +                error_report_once(
> > +                    "9p: Multiple devices detected in same VirtFS export.
> > " +                    "Access of guest to additional devices is (partly)
> > " +                    "denied due to virtfs option 'multidevs=forbid'
> > being " +                    "effective."
> > +                );
> > +                return -ENODEV;
> > +            } else {
> > +                error_report_once(
> 
> Please use warn_report_once().

Sure!

> > +                    "9p: Multiple devices detected in same VirtFS export,
> > " +                    "which might lead to file ID collisions and severe
> > " +                    "misbehaviours on guest! You should either use a "
> > +                    "separate export for each device shared from host or
> > " +                    "use virtfs option 'multidevs=remap'!"
> > +                );
> > +            }
> > +        }
> > +        memset(&qidp->path, 0, sizeof(qidp->path));
> > +        size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> > +        memcpy(&qidp->path, &stbuf->st_ino, size);
> > 
> >      }
> > 
> > -    memset(&qidp->path, 0, sizeof(qidp->path));
> > -    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> > -    memcpy(&qidp->path, &stbuf->st_ino, size);
> > 
> >      qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
> >      qidp->type = 0;
> >      if (S_ISDIR(stbuf->st_mode)) {
> > 
> > @@ -616,6 +704,30 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu,
> > V9fsFidState *fidp,> 
> >      return 0;
> >  
> >  }
> > 
> > +static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> > +                                      struct dirent *dent, V9fsQID *qidp)
> > +{
> > +    struct stat stbuf;
> > +    V9fsPath path;
> > +    int err;
> > +
> > +    v9fs_path_init(&path);
> > +
> > +    err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
> > +    if (err < 0) {
> > +        goto out;
> > +    }
> > +    err = v9fs_co_lstat(pdu, &path, &stbuf);
> > +    if (err < 0) {
> > +        goto out;
> > +    }
> > +    err = stat_to_qid(pdu, &stbuf, qidp);
> > +
> > +out:
> > +    v9fs_path_free(&path);
> > +    return err;
> > +}
> > +
> > 
> >  V9fsPDU *pdu_alloc(V9fsState *s)
> >  {
> >  
> >      V9fsPDU *pdu = NULL;
> > 
> > @@ -1964,16 +2076,39 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > *pdu, V9fsFidState *fidp,> 
> >              v9fs_string_free(&name);
> >              return count;
> >          
> >          }
> > 
> > -        /*
> > -         * Fill up just the path field of qid because the client uses
> > -         * only that. To fill the entire qid structure we will have
> > -         * to stat each dirent found, which is expensive
> > -         */
> > -        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> > -        memcpy(&qid.path, &dent->d_ino, size);
> > -        /* Fill the other fields with dummy values */
> > -        qid.type = 0;
> > -        qid.version = 0;
> > +
> > +        if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> > +            /*
> > +             * dirent_to_qid() implies expensive stat call for each
> > entry,
> > +             * we must do that here though since inode remapping requires
> > +             * the device id, which in turn might be different for
> > +             * different entries; we cannot make any assumption to avoid
> > +             * that here.
> > +             */
> > +            err = dirent_to_qid(pdu, fidp, dent, &qid);
> > +            if (err < 0) {
> > +                v9fs_readdir_unlock(&fidp->fs.dir);
> > +                v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> > +                v9fs_string_free(&name);
> > +                return err;
> > +            }
> > +        } else {
> > +            /*
> > +             * Fill up just the path field of qid because the client uses
> > +             * only that. To fill the entire qid structure we will have
> > +             * to stat each dirent found, which is expensive. For the
> > +             * latter reason we don't call dirent_to_qid() here. Only
> > drawback +             * is that no multi-device export detection of
> > stat_to_qid() +             * would be done and provided as error to the
> > user here. But +             * user would get that error anyway when
> > accessing those +             * files/dirs through other ways.
> > +             */
> > +            size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> > +            memcpy(&qid.path, &dent->d_ino, size);
> > +            /* Fill the other fields with dummy values */
> > +            qid.type = 0;
> > +            qid.version = 0;
> > +        }
> > 
> >          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > 
> > @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const
> > V9fsTransport *t,> 
> >          goto out;
> >      
> >      }
> > 
> > +    s->root_ino = stat.st_ino;
> > 
> >      s->dev_id = stat.st_dev;
> > 
> > +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> > +    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
> > +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> > +
> > 
> >      s->ctx.fst = &fse->fst;
> >      fsdev_throttle_init(s->ctx.fst);
> > 
> > @@ -3687,6 +3827,7 @@ out:
> >          }
> >          g_free(s->tag);
> >          g_free(s->ctx.fs_root);
> > 
> > +        qpp_table_destroy(&s->qpp_table);
> 
> This causes QEMU to crash if we get there before qht_init() was called.
> This should be guarded by a s->qpp_table.map != NULL check.

Touché.

I'll add that to qp_table_destroy() for simplicity.

> I've just posted a patch that simplifies error handling in this
> function, you in Cc. The patch is also in 9p-next. Please rebase
> on top of it.

Ok.
 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9621e934c0..603e5e8e15 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1335,17 +1335,17 @@ ETEXI
> > 
> >  DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
> >  
> >      "-virtfs
> >      local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-fil
> >      e|passthrough|none\n"> 
> > -    "       
> > [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n" -
> >    "-virtfs
> > proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]
> > \n" -    "-virtfs
> > proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonl
> > y]\n" +    "       
> > [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode][,mul
> > tidevs=remap|forbid|warn]\n" +    "-virtfs
> > proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]
> > [,multidevs=remap|forbid|warn]\n" +    "-virtfs
> > proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonl
> > y][,multidevs=remap|forbid|warn]\n"
> Either enable support in "proxy" or don't update the "proxy" documentation
> :)

Then, if you don't mind, I'll just drop it from the proxy documentation, for 
the reasons I described above.

> >      "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
> >      QEMU_ARCH_ALL)
> >  
> >  STEXI
> > 
> > -@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag}
> > ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly]
> > [,fmode=@var{fmode}][,dmode=@var{dmode}] -@itemx -virtfs
> > proxy,socket=@var{socket},mount_tag=@var{mount_tag}
> > [,writeout=@var{writeout}][,readonly] -@itemx -virtfs
> > proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag}
> > [,writeout=@var{writeout}][,readonly] +@item -virtfs
> > local,path=@var{path},mount_tag=@var{mount_tag}
> > ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly]
> > [,fmode=@var{fmode}][,dmode=@var{dmode}][,multidevs=@var{multidevs}]
> > +@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag}
> > [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}] +@itemx
> > -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag}
> > [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
> Ditto.

Ack.

> >  @itemx -virtfs synth,mount_tag=@var{mount_tag}
> >  @findex -virtfs
> > 
> > @@ -1399,6 +1399,27 @@ Specifies the default mode for newly created
> > directories on the host. Works> 
> >  only with security models "mapped-xattr" and "mapped-file".
> >  @item mount_tag=@var{mount_tag}
> >  Specifies the tag name to be used by the guest to mount this export
> >  point.
> > 
> > +@item multidevs=@var{multidevs}
> > +Specifies how to deal with multiple devices being shared with a 9p
> > export.
> > +Supported behaviours are either "remap", "forbid" or "warn". The latter
> > is
> > +the default behaviour on which virtfs 9p expects only one device to be
> > +shared with the same export, and if more than one device is shared and
> > +accessed via the same 9p export then only a warning message is logged
> > +(once) by qemu on host side. In order to avoid file ID collisions on
> > guest
> > +you should either create a separate virtfs export for each device to be
> > +shared with guests (recommended way) or you might use "remap" instead
> > which +allows you to share multiple devices with only one export instead,
> > which is +achieved by remapping the original inode numbers from host to
> > guest in a +way that would prevent such collisions. Remapping inodes in
> > such use cases +is required because the original device IDs from host are
> > never passed and +exposed on guest. Instead all files of an export shared
> > with virtfs always +share the same device id on guest. So two files with
> > identical inode +numbers but from actually different devices on host
> > would otherwise cause a +file ID collision and hence potential
> > misbehaviours on guest. "forbid" on +the other hand assumes like "warn"
> > that only one device is shared by the +same export, however it will not
> > only log a warning message but also +deny access to additional devices on
> > guest. Note though that "forbid" does +currently not block all possible
> > file access operations.
> 
> Maybe provide a list of such operations that won't be blocked ?

I feel punished for being so verbose. Okaaay. :)

> 
> >  @end table
> >  ETEXI
> > 
> > diff --git a/vl.c b/vl.c
> > index b426b32134..9cb29b483d 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3320,7 +3320,7 @@ int main(int argc, char **argv, char **envp)
> > 
> >              case QEMU_OPTION_virtfs: {
> >              
> >                  QemuOpts *fsdev;
> >                  QemuOpts *device;
> > 
> > -                const char *writeout, *sock_fd, *socket, *path,
> > *security_model; +                const char *writeout, *sock_fd,
> > *socket, *path, *security_model, *multidevs;> 
> >                  olist = qemu_find_opts("virtfs");
> >                  if (!olist) {
> > 
> > @@ -3380,6 +3380,10 @@ int main(int argc, char **argv, char **envp)
> > 
> >                  qemu_opt_set_bool(fsdev, "readonly",
> >                  
> >                                    qemu_opt_get_bool(opts, "readonly", 0),
> >                                    &error_abort);
> > 
> > +                multidevs = qemu_opt_get(opts, "multidevs");
> > +                if (multidevs) {
> > +                    qemu_opt_set(fsdev, "multidevs", multidevs,
> > &error_abort); +                }
> > 
> >                  device = qemu_opts_create(qemu_find_opts("device"), NULL,
> >                  0,
> >                  
> >                                            &error_abort);
> >                  
> >                  qemu_opt_set(device, "driver", "virtio-9p-pci",
> >                  &error_abort);

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Greg Kurz]

On Sun, 01 Sep 2019 20:40:34 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 29. August 2019 18:55:28 CEST Greg Kurz wrote:
> > > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> > > index 7c31ffffaf..07a18c6e48 100644
> > > --- a/fsdev/qemu-fsdev-opts.c
> > > +++ b/fsdev/qemu-fsdev-opts.c
> > > @@ -31,7 +31,9 @@ static QemuOptsList qemu_fsdev_opts = {
> > > 
> > >          }, {
> > >          
> > >              .name = "readonly",
> > >              .type = QEMU_OPT_BOOL,
> > > 
> > > -
> > > +        }, {
> > > +            .name = "multidevs",
> > > +            .type = QEMU_OPT_STRING,
> > > 
> > >          }, {
> > >          
> > >              .name = "socket",
> > >              .type = QEMU_OPT_STRING,
> > > 
> > > @@ -76,6 +78,9 @@ static QemuOptsList qemu_virtfs_opts = {
> > > 
> > >              .name = "readonly",
> > >              .type = QEMU_OPT_BOOL,
> > >          
> > >          }, {
> > > 
> > > +            .name = "multidevs",
> > > +            .type = QEMU_OPT_STRING,
> > > +        }, {
> > > 
> > >              .name = "socket",
> > >              .type = QEMU_OPT_STRING,
> > >          
> > >          }, {
> > > 
> > > diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> > > index 077a8c4e2b..ed03d559a9 100644
> > > --- a/fsdev/qemu-fsdev.c
> > > +++ b/fsdev/qemu-fsdev.c
> > > @@ -58,6 +58,7 @@ static FsDriverTable FsDrivers[] = {
> > > 
> > >              "writeout",
> > >              "fmode",
> > >              "dmode",
> > > 
> > > +            "multidevs",
> > 
> > So we only allow this for the "local" backend. Any reason not to
> > add this to "proxy" as well ?
> > 
> > I didn't do it for the "throttling" options because it is a
> > feature I didn't care to support much, but "multidevs" is more
> > a fix than a fancy feature.
> 
> Well, to be honest I haven't cared about the proxy backend at all. Like I 
> mentioned before, I am a bit sceptical that the proxy feature is actually used 
> by people in real life at all (at least in its current implementation). So 
> personally I don't see much sense in investing time for adding & testing new 
> things on this backend ATM.
> 

It's fine by me, as my ultimate plan is to deprecate proxy at some point
after virtio-fs gets merged :)

So in this case, you just need to not document the option as being
available for proxy.

> > > +    if (multidevs) {
> > > +        if (!strcmp(multidevs, "remap")) {
> > > +            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
> > > +            fsle->fse.export_flags |= V9FS_REMAP_INODES;
> > > +        } else if (!strcmp(multidevs, "forbid")) {
> > > +            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
> > > +            fsle->fse.export_flags |= V9FS_FORBID_MULTIDEVS;
> > > +        }
> > 
> > And...
> > 
> >         } else if (!strcmp(multidevs, "warn")) {
> >             fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
> >             fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
> >         } else {
> >             error_setg(errp, "invalid multidevs property '%s'", multidevs);
> >             return -1;
> >         }
> > 
> > ... because we're a bit pedantic for command line options :)
> 
> And I thought I adapted to relaxed handling of CLI arguments. See existing 
> option "writeout".  :)
> 

Yeah... it's a mess. I don't think people really cared at the time, but
I personally prefer to have stricter checking. Feel free to send fixes :)

> > 
> > > +    }
> > > 
> > >      if (fsle->fse.ops->parse_opts) {
> > >      
> > >          if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 8cc65c2c67..c96ea51116 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -25,6 +25,7 @@
> > > 
> > >  #include "trace.h"
> > >  #include "migration/blocker.h"
> > >  #include "sysemu/qtest.h"
> > > 
> > > +#include "qemu/xxhash.h"
> > > 
> > >  int open_fd_hw;
> > >  int total_open_fd;
> > > 
> > > @@ -571,22 +572,109 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> > > 
> > >                                  P9_STAT_MODE_NAMED_PIPE |   \
> > >                                  P9_STAT_MODE_SOCKET)
> > > 
> > > -/* This is the algorithm from ufs in spfs */
> > > +
> > > +/* creative abuse of tb_hash_func7, which is based on xxhash */
> > > +static uint32_t qpp_hash(QppEntry e)
> > > +{
> > > +    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
> > > +}
> > > +
> > > +static bool qpp_lookup_func(const void *obj, const void *userp)
> > > +{
> > > +    const QppEntry *e1 = obj, *e2 = userp;
> > > +    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> > > +}
> > > +
> > > +static void qpp_table_remove(void *p, uint32_t h, void *up)
> > > +{
> > > +    g_free(p);
> > > +}
> > > +
> > > +static void qpp_table_destroy(struct qht *ht)
> > > +{
> > > +    qht_iter(ht, qpp_table_remove, NULL);
> > > +    qht_destroy(ht);
> > > +}
> > 
> > Ok to have a function for this instead of open-coding but I'd
> > like to see qpp_table_init() for consistency.
> 
> Well, these are just qht_init() one-liners, but if you really want to have 
> dedicated, local init functions for them, okay.
> 

Yeah, even if it's a one-liner, I prefer consistency. Alternatively, with
an idempotent v9fs_device_unrealize_common() like in [1], you'd have
only one user for qpp_table_destroy() and you can open-code it. This
would address my consistency concern even better :)

[1] https://github.com/gkurz/qemu/commit/7fc4c49e910df2e155b36bf0a05de9209bd92da9

> > >  static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID
> > >  *qidp) {
> > > 
> > > +    int err;
> > > 
> > >      size_t size;
> > > 
> > > -    if (pdu->s->dev_id != stbuf->st_dev) {
> > > -        error_report_once(
> > > -            "9p: Multiple devices detected in same VirtFS export. "
> > > -            "You must use a separate export for each device."
> > > -        );
> > > -        return -ENODEV;
> > > +    if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> > > +        /* map inode+device to qid path (fast path) */
> > > +        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> > > +        if (err) {
> > > +            return err;
> > > +        }
> > > +    } else {
> > > +        if (pdu->s->dev_id != stbuf->st_dev) {
> > > +            if (pdu->s->ctx.export_flags & V9FS_FORBID_MULTIDEVS) {
> > > +                error_report_once(
> > > +                    "9p: Multiple devices detected in same VirtFS export.
> > > " +                    "Access of guest to additional devices is (partly)
> > > " +                    "denied due to virtfs option 'multidevs=forbid'
> > > being " +                    "effective."
> > > +                );
> > > +                return -ENODEV;
> > > +            } else {
> > > +                error_report_once(
> > 
> > Please use warn_report_once().
> 
> Sure!
> 
> > > +                    "9p: Multiple devices detected in same VirtFS export,
> > > " +                    "which might lead to file ID collisions and severe
> > > " +                    "misbehaviours on guest! You should either use a "
> > > +                    "separate export for each device shared from host or
> > > " +                    "use virtfs option 'multidevs=remap'!"
> > > +                );
> > > +            }
> > > +        }
> > > +        memset(&qidp->path, 0, sizeof(qidp->path));
> > > +        size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> > > +        memcpy(&qidp->path, &stbuf->st_ino, size);
> > > 
> > >      }
> > > 
> > > -    memset(&qidp->path, 0, sizeof(qidp->path));
> > > -    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> > > -    memcpy(&qidp->path, &stbuf->st_ino, size);
> > > 
> > >      qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
> > >      qidp->type = 0;
> > >      if (S_ISDIR(stbuf->st_mode)) {
> > > 
> > > @@ -616,6 +704,30 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu,
> > > V9fsFidState *fidp,> 
> > >      return 0;
> > >  
> > >  }
> > > 
> > > +static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> > > +                                      struct dirent *dent, V9fsQID *qidp)
> > > +{
> > > +    struct stat stbuf;
> > > +    V9fsPath path;
> > > +    int err;
> > > +
> > > +    v9fs_path_init(&path);
> > > +
> > > +    err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
> > > +    if (err < 0) {
> > > +        goto out;
> > > +    }
> > > +    err = v9fs_co_lstat(pdu, &path, &stbuf);
> > > +    if (err < 0) {
> > > +        goto out;
> > > +    }
> > > +    err = stat_to_qid(pdu, &stbuf, qidp);
> > > +
> > > +out:
> > > +    v9fs_path_free(&path);
> > > +    return err;
> > > +}
> > > +
> > > 
> > >  V9fsPDU *pdu_alloc(V9fsState *s)
> > >  {
> > >  
> > >      V9fsPDU *pdu = NULL;
> > > 
> > > @@ -1964,16 +2076,39 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > > *pdu, V9fsFidState *fidp,> 
> > >              v9fs_string_free(&name);
> > >              return count;
> > >          
> > >          }
> > > 
> > > -        /*
> > > -         * Fill up just the path field of qid because the client uses
> > > -         * only that. To fill the entire qid structure we will have
> > > -         * to stat each dirent found, which is expensive
> > > -         */
> > > -        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> > > -        memcpy(&qid.path, &dent->d_ino, size);
> > > -        /* Fill the other fields with dummy values */
> > > -        qid.type = 0;
> > > -        qid.version = 0;
> > > +
> > > +        if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> > > +            /*
> > > +             * dirent_to_qid() implies expensive stat call for each
> > > entry,
> > > +             * we must do that here though since inode remapping requires
> > > +             * the device id, which in turn might be different for
> > > +             * different entries; we cannot make any assumption to avoid
> > > +             * that here.
> > > +             */
> > > +            err = dirent_to_qid(pdu, fidp, dent, &qid);
> > > +            if (err < 0) {
> > > +                v9fs_readdir_unlock(&fidp->fs.dir);
> > > +                v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> > > +                v9fs_string_free(&name);
> > > +                return err;
> > > +            }
> > > +        } else {
> > > +            /*
> > > +             * Fill up just the path field of qid because the client uses
> > > +             * only that. To fill the entire qid structure we will have
> > > +             * to stat each dirent found, which is expensive. For the
> > > +             * latter reason we don't call dirent_to_qid() here. Only
> > > drawback +             * is that no multi-device export detection of
> > > stat_to_qid() +             * would be done and provided as error to the
> > > user here. But +             * user would get that error anyway when
> > > accessing those +             * files/dirs through other ways.
> > > +             */
> > > +            size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> > > +            memcpy(&qid.path, &dent->d_ino, size);
> > > +            /* Fill the other fields with dummy values */
> > > +            qid.type = 0;
> > > +            qid.version = 0;
> > > +        }
> > > 
> > >          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> > >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > > 
> > > @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const
> > > V9fsTransport *t,> 
> > >          goto out;
> > >      
> > >      }
> > > 
> > > +    s->root_ino = stat.st_ino;
> > > 
> > >      s->dev_id = stat.st_dev;
> > > 
> > > +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> > > +    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
> > > +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> > > +
> > > 
> > >      s->ctx.fst = &fse->fst;
> > >      fsdev_throttle_init(s->ctx.fst);
> > > 
> > > @@ -3687,6 +3827,7 @@ out:
> > >          }
> > >          g_free(s->tag);
> > >          g_free(s->ctx.fs_root);
> > > 
> > > +        qpp_table_destroy(&s->qpp_table);
> > 
> > This causes QEMU to crash if we get there before qht_init() was called.
> > This should be guarded by a s->qpp_table.map != NULL check.
> 
> Touché.
> 
> I'll add that to qp_table_destroy() for simplicity.
> 
> > I've just posted a patch that simplifies error handling in this
> > function, you in Cc. The patch is also in 9p-next. Please rebase
> > on top of it.
> 
> Ok.
>  
> > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > index 9621e934c0..603e5e8e15 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -1335,17 +1335,17 @@ ETEXI
> > > 
> > >  DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
> > >  
> > >      "-virtfs
> > >      local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-fil
> > >      e|passthrough|none\n"> 
> > > -    "       
> > > [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n" -
> > >    "-virtfs
> > > proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]
> > > \n" -    "-virtfs
> > > proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonl
> > > y]\n" +    "       
> > > [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode][,mul
> > > tidevs=remap|forbid|warn]\n" +    "-virtfs
> > > proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]
> > > [,multidevs=remap|forbid|warn]\n" +    "-virtfs
> > > proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonl
> > > y][,multidevs=remap|forbid|warn]\n"
> > Either enable support in "proxy" or don't update the "proxy" documentation
> > :)
> 
> Then, if you don't mind, I'll just drop it from the proxy documentation, for 
> the reasons I described above.
> 

Agreed.

> > >      "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
> > >      QEMU_ARCH_ALL)
> > >  
> > >  STEXI
> > > 
> > > -@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag}
> > > ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly]
> > > [,fmode=@var{fmode}][,dmode=@var{dmode}] -@itemx -virtfs
> > > proxy,socket=@var{socket},mount_tag=@var{mount_tag}
> > > [,writeout=@var{writeout}][,readonly] -@itemx -virtfs
> > > proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag}
> > > [,writeout=@var{writeout}][,readonly] +@item -virtfs
> > > local,path=@var{path},mount_tag=@var{mount_tag}
> > > ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly]
> > > [,fmode=@var{fmode}][,dmode=@var{dmode}][,multidevs=@var{multidevs}]
> > > +@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag}
> > > [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}] +@itemx
> > > -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag}
> > > [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
> > Ditto.
> 
> Ack.
> 
> > >  @itemx -virtfs synth,mount_tag=@var{mount_tag}
> > >  @findex -virtfs
> > > 
> > > @@ -1399,6 +1399,27 @@ Specifies the default mode for newly created
> > > directories on the host. Works> 
> > >  only with security models "mapped-xattr" and "mapped-file".
> > >  @item mount_tag=@var{mount_tag}
> > >  Specifies the tag name to be used by the guest to mount this export
> > >  point.
> > > 
> > > +@item multidevs=@var{multidevs}
> > > +Specifies how to deal with multiple devices being shared with a 9p
> > > export.
> > > +Supported behaviours are either "remap", "forbid" or "warn". The latter
> > > is
> > > +the default behaviour on which virtfs 9p expects only one device to be
> > > +shared with the same export, and if more than one device is shared and
> > > +accessed via the same 9p export then only a warning message is logged
> > > +(once) by qemu on host side. In order to avoid file ID collisions on
> > > guest
> > > +you should either create a separate virtfs export for each device to be
> > > +shared with guests (recommended way) or you might use "remap" instead
> > > which +allows you to share multiple devices with only one export instead,
> > > which is +achieved by remapping the original inode numbers from host to
> > > guest in a +way that would prevent such collisions. Remapping inodes in
> > > such use cases +is required because the original device IDs from host are
> > > never passed and +exposed on guest. Instead all files of an export shared
> > > with virtfs always +share the same device id on guest. So two files with
> > > identical inode +numbers but from actually different devices on host
> > > would otherwise cause a +file ID collision and hence potential
> > > misbehaviours on guest. "forbid" on +the other hand assumes like "warn"
> > > that only one device is shared by the +same export, however it will not
> > > only log a warning message but also +deny access to additional devices on
> > > guest. Note though that "forbid" does +currently not block all possible
> > > file access operations.
> > 
> > Maybe provide a list of such operations that won't be blocked ?
> 
> I feel punished for being so verbose. Okaaay. :)
> 

:)

> > 
> > >  @end table
> > >  ETEXI
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index b426b32134..9cb29b483d 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -3320,7 +3320,7 @@ int main(int argc, char **argv, char **envp)
> > > 
> > >              case QEMU_OPTION_virtfs: {
> > >              
> > >                  QemuOpts *fsdev;
> > >                  QemuOpts *device;
> > > 
> > > -                const char *writeout, *sock_fd, *socket, *path,
> > > *security_model; +                const char *writeout, *sock_fd,
> > > *socket, *path, *security_model, *multidevs;> 
> > >                  olist = qemu_find_opts("virtfs");
> > >                  if (!olist) {
> > > 
> > > @@ -3380,6 +3380,10 @@ int main(int argc, char **argv, char **envp)
> > > 
> > >                  qemu_opt_set_bool(fsdev, "readonly",
> > >                  
> > >                                    qemu_opt_get_bool(opts, "readonly", 0),
> > >                                    &error_abort);
> > > 
> > > +                multidevs = qemu_opt_get(opts, "multidevs");
> > > +                if (multidevs) {
> > > +                    qemu_opt_set(fsdev, "multidevs", multidevs,
> > > &error_abort); +                }
> > > 
> > >                  device = qemu_opts_create(qemu_find_opts("device"), NULL,
> > >                  0,
> > >                  
> > >                                            &error_abort);
> > >                  
> > >                  qemu_opt_set(device, "driver", "virtio-9p-pci",
> > >                  &error_abort);
> 

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Christian Schoenebeck via Qemu-devel]

On Montag, 2. September 2019 12:16:26 CEST Greg Kurz wrote:
> > > > @@ -571,22 +572,109 @@ static void coroutine_fn virtfs_reset(V9fsPDU
> > > > *pdu)
> > > > 
> > > >                                  P9_STAT_MODE_NAMED_PIPE |   \
> > > >                                  P9_STAT_MODE_SOCKET)
> > > > 
> > > > -/* This is the algorithm from ufs in spfs */
> > > > +
> > > > +/* creative abuse of tb_hash_func7, which is based on xxhash */
> > > > +static uint32_t qpp_hash(QppEntry e)
> > > > +{
> > > > +    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
> > > > +}
> > > > +
> > > > +static bool qpp_lookup_func(const void *obj, const void *userp)
> > > > +{
> > > > +    const QppEntry *e1 = obj, *e2 = userp;
> > > > +    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> > > > +}
> > > > +
> > > > +static void qpp_table_remove(void *p, uint32_t h, void *up)
> > > > +{
> > > > +    g_free(p);
> > > > +}
> > > > +
> > > > +static void qpp_table_destroy(struct qht *ht)
> > > > +{
> > > > +    qht_iter(ht, qpp_table_remove, NULL);
> > > > +    qht_destroy(ht);
> > > > +}
> > > 
> > > Ok to have a function for this instead of open-coding but I'd
> > > like to see qpp_table_init() for consistency.
> > 
> > Well, these are just qht_init() one-liners, but if you really want to have
> > dedicated, local init functions for them, okay.
> 
> Yeah, even if it's a one-liner, I prefer consistency. Alternatively, with
> an idempotent v9fs_device_unrealize_common() like in [1], you'd have
> only one user for qpp_table_destroy() and you can open-code it. This
> would address my consistency concern even better :)
> 
> [1]
> https://github.com/gkurz/qemu/commit/7fc4c49e910df2e155b36bf0a05de9209bd92d

I'll rather add qpp_table_init() then, because grouping the two calls 
qht_iter() and qht_destroy() together to a dedicated function 
qpp_table_destroy() still makes sense semantically IMO.

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Greg Kurz]

On Thu, 22 Aug 2019 21:33:37 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> 'warn' (default): Only log an error message (once) on host if more than one
> device is shared by same export, except of that just ignore this config
> error though. This is the default behaviour for not breaking existing
> installations implying that they really know what they are doing.
> 
> 'forbid': Like 'warn', but except of just logging an error this
> also denies access of guest to additional devices.
> 
> 'remap': Allows to share more than one device per export by remapping
> inodes from host to guest appropriately. To support multiple devices on the
> 9p share, and avoid qid path collisions we take the device id as input to
> generate a unique QID path. The lowest 48 bits of the path will be set
> equal to the file inode, and the top bits will be uniquely assigned based
> on the top 16 bits of the inode and the device id.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> [CS: - Rebased to https://github.com/gkurz/qemu/commits/9p-next
>        (SHA1 177fd3b6a8).
>      - Updated hash calls to new xxhash API.
>      - Added virtfs option 'multidevs', original patch simply did the inode
>        remapping without being asked.
>      - Updated docs for new option 'multidevs'.
>      - Capture root_ino in v9fs_device_realize_common() as well, not just
>        the device id.
>      - Fixed v9fs_do_readdir() not having remapped inodes.
>      - Log error message when running out of prefixes in
>        qid_path_prefixmap().
>      - Fixed definition of QPATH_INO_MASK.
>      - Dropped unnecessary parantheses in qpp_lookup_func().
>      - Dropped unnecessary g_malloc0() result checks. ]
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Some more comments below.

>  fsdev/file-op-9p.h      |   5 ++
>  fsdev/qemu-fsdev-opts.c |   7 +-
>  fsdev/qemu-fsdev.c      |  11 +++
>  hw/9pfs/9p.c            | 182 ++++++++++++++++++++++++++++++++++++++++++------
>  hw/9pfs/9p.h            |  13 ++++
>  qemu-options.hx         |  33 +++++++--
>  vl.c                    |   6 +-
>  7 files changed, 229 insertions(+), 28 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index c757c8099f..f2f7772c86 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -59,6 +59,11 @@ typedef struct ExtendedOps {
>  #define V9FS_RDONLY                 0x00000040
>  #define V9FS_PROXY_SOCK_FD          0x00000080
>  #define V9FS_PROXY_SOCK_NAME        0x00000100
> +/*
> + * multidevs option (either one of the two applies exclusively)
> + */
> +#define V9FS_REMAP_INODES           0x00000200
> +#define V9FS_FORBID_MULTIDEVS       0x00000400
>  
>  #define V9FS_SEC_MASK               0x0000003C
>  
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> index 7c31ffffaf..07a18c6e48 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -31,7 +31,9 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "readonly",
>              .type = QEMU_OPT_BOOL,
> -
> +        }, {
> +            .name = "multidevs",
> +            .type = QEMU_OPT_STRING,
>          }, {
>              .name = "socket",
>              .type = QEMU_OPT_STRING,
> @@ -76,6 +78,9 @@ static QemuOptsList qemu_virtfs_opts = {
>              .name = "readonly",
>              .type = QEMU_OPT_BOOL,
>          }, {
> +            .name = "multidevs",
> +            .type = QEMU_OPT_STRING,
> +        }, {
>              .name = "socket",
>              .type = QEMU_OPT_STRING,
>          }, {
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index 077a8c4e2b..ed03d559a9 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -58,6 +58,7 @@ static FsDriverTable FsDrivers[] = {
>              "writeout",
>              "fmode",
>              "dmode",
> +            "multidevs",
>              "throttling.bps-total",
>              "throttling.bps-read",
>              "throttling.bps-write",
> @@ -121,6 +122,7 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
>      const char *fsdev_id = qemu_opts_id(opts);
>      const char *fsdriver = qemu_opt_get(opts, "fsdriver");
>      const char *writeout = qemu_opt_get(opts, "writeout");
> +    const char *multidevs = qemu_opt_get(opts, "multidevs");
>      bool ro = qemu_opt_get_bool(opts, "readonly", 0);
>  
>      if (!fsdev_id) {
> @@ -161,6 +163,15 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
>      } else {
>          fsle->fse.export_flags &= ~V9FS_RDONLY;
>      }
> +    if (multidevs) {
> +        if (!strcmp(multidevs, "remap")) {
> +            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
> +            fsle->fse.export_flags |= V9FS_REMAP_INODES;
> +        } else if (!strcmp(multidevs, "forbid")) {
> +            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
> +            fsle->fse.export_flags |= V9FS_FORBID_MULTIDEVS;
> +        }
> +    }
>  
>      if (fsle->fse.ops->parse_opts) {
>          if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8cc65c2c67..c96ea51116 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -25,6 +25,7 @@
>  #include "trace.h"
>  #include "migration/blocker.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/xxhash.h"
>  
>  int open_fd_hw;
>  int total_open_fd;
> @@ -571,22 +572,109 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>                                  P9_STAT_MODE_NAMED_PIPE |   \
>                                  P9_STAT_MODE_SOCKET)
>  
> -/* This is the algorithm from ufs in spfs */
> +
> +/* creative abuse of tb_hash_func7, which is based on xxhash */
> +static uint32_t qpp_hash(QppEntry e)
> +{
> +    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
> +}
> +
> +static bool qpp_lookup_func(const void *obj, const void *userp)
> +{
> +    const QppEntry *e1 = obj, *e2 = userp;
> +    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> +}
> +
> +static void qpp_table_remove(void *p, uint32_t h, void *up)
> +{
> +    g_free(p);
> +}
> +
> +static void qpp_table_destroy(struct qht *ht)
> +{
> +    qht_iter(ht, qpp_table_remove, NULL);
> +    qht_destroy(ht);
> +}
> +
> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> + * to a unique QID path (64 bits). To avoid having to map and keep track
> + * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> + * the device id to the 16 highest bits of the QID path. The 48 lowest bits
> + * of the QID path equal to the lowest bits of the inode number.
> + *
> + * This takes advantage of the fact that inode number are usually not
> + * random but allocated sequentially, so we have fewer items to keep
> + * track of.
> + */
> +static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
> +                                uint64_t *path)
> +{
> +    QppEntry lookup = {
> +        .dev = stbuf->st_dev,
> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
> +    }, *val;
> +    uint32_t hash = qpp_hash(lookup);
> +
> +    val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
> +
> +    if (!val) {
> +        if (pdu->s->qp_prefix_next == 0) {
> +            /* we ran out of prefixes */
> +            error_report_once(
> +                "9p: No more prefixes available for remapping inodes from "
> +                "host to guest."
> +            );
> +            return -ENFILE;
> +        }
> +
> +        val = g_malloc0(sizeof(QppEntry));
> +        *val = lookup;
> +
> +        /* new unique inode prefix and device combo */
> +        val->qp_prefix = pdu->s->qp_prefix_next++;
> +        qht_insert(&pdu->s->qpp_table, val, hash, NULL);
> +    }
> +
> +    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
> +    return 0;
> +}
> +
>  static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>  {
> +    int err;
>      size_t size;
>  
> -    if (pdu->s->dev_id != stbuf->st_dev) {
> -        error_report_once(
> -            "9p: Multiple devices detected in same VirtFS export. "
> -            "You must use a separate export for each device."
> -        );
> -        return -ENODEV;
> +    if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> +        /* map inode+device to qid path (fast path) */
> +        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> +        if (err) {
> +            return err;
> +        }
> +    } else {
> +        if (pdu->s->dev_id != stbuf->st_dev) {
> +            if (pdu->s->ctx.export_flags & V9FS_FORBID_MULTIDEVS) {
> +                error_report_once(
> +                    "9p: Multiple devices detected in same VirtFS export. "
> +                    "Access of guest to additional devices is (partly) "
> +                    "denied due to virtfs option 'multidevs=forbid' being "
> +                    "effective."
> +                );
> +                return -ENODEV;
> +            } else {
> +                error_report_once(
> +                    "9p: Multiple devices detected in same VirtFS export, "
> +                    "which might lead to file ID collisions and severe "
> +                    "misbehaviours on guest! You should either use a "
> +                    "separate export for each device shared from host or "
> +                    "use virtfs option 'multidevs=remap'!"
> +                );
> +            }
> +        }
> +        memset(&qidp->path, 0, sizeof(qidp->path));
> +        size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> +        memcpy(&qidp->path, &stbuf->st_ino, size);
>      }
>  
> -    memset(&qidp->path, 0, sizeof(qidp->path));
> -    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> -    memcpy(&qidp->path, &stbuf->st_ino, size);
>      qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
>      qidp->type = 0;
>      if (S_ISDIR(stbuf->st_mode)) {
> @@ -616,6 +704,30 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
>      return 0;
>  }
>  
> +static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> +                                      struct dirent *dent, V9fsQID *qidp)
> +{
> +    struct stat stbuf;
> +    V9fsPath path;
> +    int err;
> +
> +    v9fs_path_init(&path);
> +
> +    err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
> +    if (err < 0) {
> +        goto out;
> +    }
> +    err = v9fs_co_lstat(pdu, &path, &stbuf);
> +    if (err < 0) {
> +        goto out;
> +    }
> +    err = stat_to_qid(pdu, &stbuf, qidp);
> +
> +out:
> +    v9fs_path_free(&path);
> +    return err;
> +}
> +
>  V9fsPDU *pdu_alloc(V9fsState *s)
>  {
>      V9fsPDU *pdu = NULL;
> @@ -1964,16 +2076,39 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>              v9fs_string_free(&name);
>              return count;
>          }
> -        /*
> -         * Fill up just the path field of qid because the client uses
> -         * only that. To fill the entire qid structure we will have
> -         * to stat each dirent found, which is expensive
> -         */
> -        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> -        memcpy(&qid.path, &dent->d_ino, size);
> -        /* Fill the other fields with dummy values */
> -        qid.type = 0;
> -        qid.version = 0;
> +
> +        if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> +            /*
> +             * dirent_to_qid() implies expensive stat call for each entry,
> +             * we must do that here though since inode remapping requires
> +             * the device id, which in turn might be different for
> +             * different entries; we cannot make any assumption to avoid
> +             * that here.
> +             */
> +            err = dirent_to_qid(pdu, fidp, dent, &qid);
> +            if (err < 0) {
> +                v9fs_readdir_unlock(&fidp->fs.dir);
> +                v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> +                v9fs_string_free(&name);
> +                return err;
> +            }
> +        } else {
> +            /*
> +             * Fill up just the path field of qid because the client uses
> +             * only that. To fill the entire qid structure we will have
> +             * to stat each dirent found, which is expensive. For the
> +             * latter reason we don't call dirent_to_qid() here. Only drawback
> +             * is that no multi-device export detection of stat_to_qid()
> +             * would be done and provided as error to the user here. But
> +             * user would get that error anyway when accessing those
> +             * files/dirs through other ways.
> +             */
> +            size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> +            memcpy(&qid.path, &dent->d_ino, size);
> +            /* Fill the other fields with dummy values */
> +            qid.type = 0;
> +            qid.version = 0;
> +        }
>  
>          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>          goto out;
>      }
>  
> +    s->root_ino = stat.st_ino;

This isn't used anywhere. It looks like a leftover of the readdir fix
in v5.

>      s->dev_id = stat.st_dev;
>  
> +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> +    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
> +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> +
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
>  
> @@ -3687,6 +3827,7 @@ out:
>          }
>          g_free(s->tag);
>          g_free(s->ctx.fs_root);
> +        qpp_table_destroy(&s->qpp_table);
>          v9fs_path_free(&path);
>      }
>      return rc;
> @@ -3699,6 +3840,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>      }
>      fsdev_throttle_cleanup(s->ctx.fst);
>      g_free(s->tag);
> +    qpp_table_destroy(&s->qpp_table);
>      g_free(s->ctx.fs_root);
>  }
>  
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 5e316178d5..a283b0193e 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -8,6 +8,7 @@
>  #include "fsdev/9p-iov-marshal.h"
>  #include "qemu/thread.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/qht.h"
>  
>  enum {
>      P9_TLERROR = 6,
> @@ -235,6 +236,15 @@ struct V9fsFidState
>      V9fsFidState *rclm_lst;
>  };
>  
> +#define QPATH_INO_MASK        ((1ULL << 48) - 1)
> +
> +/* QID path prefix entry, see stat_to_qid */
> +typedef struct {
> +    dev_t dev;
> +    uint16_t ino_prefix;
> +    uint16_t qp_prefix;
> +} QppEntry;
> +
>  struct V9fsState
>  {
>      QLIST_HEAD(, V9fsPDU) free_list;
> @@ -256,7 +266,10 @@ struct V9fsState
>      Error *migration_blocker;
>      V9fsConf fsconf;
>      V9fsQID root_qid;
> +    ino_t root_ino;

Thinking again, I'm wondering if root_ino and dev_id could be used
instead of root_qid in v9fs_walk()... Would you have a look at that
if you decide to fix the readdir issue ?

>      dev_t dev_id;
> +    struct qht qpp_table;
> +    uint16_t qp_prefix_next;
>  };
>  
>  /* 9p2000.L open flags */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9621e934c0..603e5e8e15 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1335,17 +1335,17 @@ ETEXI
>  
>  DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
>      "-virtfs local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-file|passthrough|none\n"
> -    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
> -    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]\n"
> -    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly]\n"
> +    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode][,multidevs=remap|forbid|warn]\n"
> +    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly][,multidevs=remap|forbid|warn]\n"
> +    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly][,multidevs=remap|forbid|warn]\n"
>      "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
>      QEMU_ARCH_ALL)
>  
>  STEXI
>  
> -@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}]
> -@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
> -@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
> +@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}][,multidevs=@var{multidevs}]
> +@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
> +@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly][,multidevs=@var{multidevs}]
>  @itemx -virtfs synth,mount_tag=@var{mount_tag}
>  @findex -virtfs
>  
> @@ -1399,6 +1399,27 @@ Specifies the default mode for newly created directories on the host. Works
>  only with security models "mapped-xattr" and "mapped-file".
>  @item mount_tag=@var{mount_tag}
>  Specifies the tag name to be used by the guest to mount this export point.
> +@item multidevs=@var{multidevs}
> +Specifies how to deal with multiple devices being shared with a 9p export.
> +Supported behaviours are either "remap", "forbid" or "warn". The latter is
> +the default behaviour on which virtfs 9p expects only one device to be
> +shared with the same export, and if more than one device is shared and
> +accessed via the same 9p export then only a warning message is logged
> +(once) by qemu on host side. In order to avoid file ID collisions on guest
> +you should either create a separate virtfs export for each device to be
> +shared with guests (recommended way) or you might use "remap" instead which
> +allows you to share multiple devices with only one export instead, which is
> +achieved by remapping the original inode numbers from host to guest in a
> +way that would prevent such collisions. Remapping inodes in such use cases
> +is required because the original device IDs from host are never passed and
> +exposed on guest. Instead all files of an export shared with virtfs always
> +share the same device id on guest. So two files with identical inode
> +numbers but from actually different devices on host would otherwise cause a
> +file ID collision and hence potential misbehaviours on guest. "forbid" on
> +the other hand assumes like "warn" that only one device is shared by the
> +same export, however it will not only log a warning message but also
> +deny access to additional devices on guest. Note though that "forbid" does
> +currently not block all possible file access operations.
>  @end table
>  ETEXI
>  
> diff --git a/vl.c b/vl.c
> index b426b32134..9cb29b483d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3320,7 +3320,7 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_virtfs: {
>                  QemuOpts *fsdev;
>                  QemuOpts *device;
> -                const char *writeout, *sock_fd, *socket, *path, *security_model;
> +                const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;
>  
>                  olist = qemu_find_opts("virtfs");
>                  if (!olist) {
> @@ -3380,6 +3380,10 @@ int main(int argc, char **argv, char **envp)
>                  qemu_opt_set_bool(fsdev, "readonly",
>                                    qemu_opt_get_bool(opts, "readonly", 0),
>                                    &error_abort);
> +                multidevs = qemu_opt_get(opts, "multidevs");
> +                if (multidevs) {
> +                    qemu_opt_set(fsdev, "multidevs", multidevs, &error_abort);
> +                }
>                  device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
>                                            &error_abort);
>                  qemu_opt_set(device, "driver", "virtio-9p-pci", &error_abort);

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Christian Schoenebeck via Qemu-devel]

On Freitag, 30. August 2019 14:22:38 CEST Greg Kurz wrote:
> Some more comments below.
[snip]
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 8cc65c2c67..c96ea51116 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include "trace.h"
> >  #include "migration/blocker.h"
> >  #include "sysemu/qtest.h"
[snip]
> > @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const
> > V9fsTransport *t,> 
> >          goto out;
> >      
> >      }
> > 
> > +    s->root_ino = stat.st_ino;
> 
> This isn't used anywhere. It looks like a leftover of the readdir fix
> in v5.

Yes, both correct. I intentionally left it though, since I found it a natural 
complement always capturing the root inode along to the root device.

> >      s->dev_id = stat.st_dev;
> > 
> > +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> > +    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
> > +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> > +
> > 
> >      s->ctx.fst = &fse->fst;
> >      fsdev_throttle_init(s->ctx.fst);
> > 
> > @@ -3687,6 +3827,7 @@ out:
> >          }
> >          g_free(s->tag);
> >          g_free(s->ctx.fs_root);
> > 
> > +        qpp_table_destroy(&s->qpp_table);
> > 
> >          v9fs_path_free(&path);
> >      
> >      }
> >      return rc;
> > 
> > @@ -3699,6 +3840,7 @@ void v9fs_device_unrealize_common(V9fsState *s,
> > Error **errp)> 
> >      }
> >      fsdev_throttle_cleanup(s->ctx.fst);
> >      g_free(s->tag);
> > 
> > +    qpp_table_destroy(&s->qpp_table);
> > 
> >      g_free(s->ctx.fs_root);
> >  
> >  }
> > 
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 5e316178d5..a283b0193e 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -8,6 +8,7 @@
> > 
> >  #include "fsdev/9p-iov-marshal.h"
> >  #include "qemu/thread.h"
> >  #include "qemu/coroutine.h"
> > 
> > +#include "qemu/qht.h"
> > 
> >  enum {
> >  
> >      P9_TLERROR = 6,
> > 
> > @@ -235,6 +236,15 @@ struct V9fsFidState
> > 
> >      V9fsFidState *rclm_lst;
> >  
> >  };
> > 
> > +#define QPATH_INO_MASK        ((1ULL << 48) - 1)
> > +
> > +/* QID path prefix entry, see stat_to_qid */
> > +typedef struct {
> > +    dev_t dev;
> > +    uint16_t ino_prefix;
> > +    uint16_t qp_prefix;
> > +} QppEntry;
> > +
> > 
> >  struct V9fsState
> >  {
> >  
> >      QLIST_HEAD(, V9fsPDU) free_list;
> > 
> > @@ -256,7 +266,10 @@ struct V9fsState
> > 
> >      Error *migration_blocker;
> >      V9fsConf fsconf;
> >      V9fsQID root_qid;
> > 
> > +    ino_t root_ino;
> 
> Thinking again, I'm wondering if root_ino and dev_id could be used
> instead of root_qid in v9fs_walk()... Would you have a look at that
> if you decide to fix the readdir issue ?

I keep it in mind when looking at the postponed readdir() issue again.

> >      dev_t dev_id;
> > 
> > +    struct qht qpp_table;
> > +    uint16_t qp_prefix_next;
> > 
> >  };
> >  
> >  /* 9p2000.L open flags */
> > 

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Greg Kurz]

On Sun, 01 Sep 2019 20:56:16 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 30. August 2019 14:22:38 CEST Greg Kurz wrote:
> > Some more comments below.
> [snip]
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 8cc65c2c67..c96ea51116 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -25,6 +25,7 @@
> > > 
> > >  #include "trace.h"
> > >  #include "migration/blocker.h"
> > >  #include "sysemu/qtest.h"
> [snip]
> > > @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s, const
> > > V9fsTransport *t,> 
> > >          goto out;
> > >      
> > >      }
> > > 
> > > +    s->root_ino = stat.st_ino;
> > 
> > This isn't used anywhere. It looks like a leftover of the readdir fix
> > in v5.
> 
> Yes, both correct. I intentionally left it though, since I found it a natural 
> complement always capturing the root inode along to the root device.
> 

Fair enough. The local backend opens an fd to the root directory, to be used by
any access to the 9p share. I think root_dev/root_ino should be obtained with fstat()
on this fd, to be sure they are consistent. Maybe add an extra struct stat * argument
to the init function ? I'd rather see this done as a preparatory "backend to cache
9p root device/inode during init" patch.

> > >      s->dev_id = stat.st_dev;
> > > 
> > > +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> > > +    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
> > > +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> > > +
> > > 
> > >      s->ctx.fst = &fse->fst;
> > >      fsdev_throttle_init(s->ctx.fst);
> > > 
> > > @@ -3687,6 +3827,7 @@ out:
> > >          }
> > >          g_free(s->tag);
> > >          g_free(s->ctx.fs_root);
> > > 
> > > +        qpp_table_destroy(&s->qpp_table);
> > > 
> > >          v9fs_path_free(&path);
> > >      
> > >      }
> > >      return rc;
> > > 
> > > @@ -3699,6 +3840,7 @@ void v9fs_device_unrealize_common(V9fsState *s,
> > > Error **errp)> 
> > >      }
> > >      fsdev_throttle_cleanup(s->ctx.fst);
> > >      g_free(s->tag);
> > > 
> > > +    qpp_table_destroy(&s->qpp_table);
> > > 
> > >      g_free(s->ctx.fs_root);
> > >  
> > >  }
> > > 
> > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > > index 5e316178d5..a283b0193e 100644
> > > --- a/hw/9pfs/9p.h
> > > +++ b/hw/9pfs/9p.h
> > > @@ -8,6 +8,7 @@
> > > 
> > >  #include "fsdev/9p-iov-marshal.h"
> > >  #include "qemu/thread.h"
> > >  #include "qemu/coroutine.h"
> > > 
> > > +#include "qemu/qht.h"
> > > 
> > >  enum {
> > >  
> > >      P9_TLERROR = 6,
> > > 
> > > @@ -235,6 +236,15 @@ struct V9fsFidState
> > > 
> > >      V9fsFidState *rclm_lst;
> > >  
> > >  };
> > > 
> > > +#define QPATH_INO_MASK        ((1ULL << 48) - 1)
> > > +
> > > +/* QID path prefix entry, see stat_to_qid */
> > > +typedef struct {
> > > +    dev_t dev;
> > > +    uint16_t ino_prefix;
> > > +    uint16_t qp_prefix;
> > > +} QppEntry;
> > > +
> > > 
> > >  struct V9fsState
> > >  {
> > >  
> > >      QLIST_HEAD(, V9fsPDU) free_list;
> > > 
> > > @@ -256,7 +266,10 @@ struct V9fsState
> > > 
> > >      Error *migration_blocker;
> > >      V9fsConf fsconf;
> > >      V9fsQID root_qid;
> > > 
> > > +    ino_t root_ino;
> > 
> > Thinking again, I'm wondering if root_ino and dev_id could be used
> > instead of root_qid in v9fs_walk()... Would you have a look at that
> > if you decide to fix the readdir issue ?
> 
> I keep it in mind when looking at the postponed readdir() issue again.
> 
> > >      dev_t dev_id;
> > > 
> > > +    struct qht qpp_table;
> > > +    uint16_t qp_prefix_next;
> > > 
> > >  };
> > >  
> > >  /* 9p2000.L open flags */
> > > 
> 

Re: [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Christian Schoenebeck via Qemu-devel]

On Montag, 2. September 2019 13:49:34 CEST Greg Kurz wrote:
> On Sun, 01 Sep 2019 20:56:16 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 30. August 2019 14:22:38 CEST Greg Kurz wrote:
> > > Some more comments below.
> > 
> > [snip]
> > 
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > index 8cc65c2c67..c96ea51116 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -25,6 +25,7 @@
> > > > 
> > > >  #include "trace.h"
> > > >  #include "migration/blocker.h"
> > > >  #include "sysemu/qtest.h"
> > 
> > [snip]
> > 
> > > > @@ -3672,8 +3807,13 @@ int v9fs_device_realize_common(V9fsState *s,
> > > > const
> > > > V9fsTransport *t,>
> > > > 
> > > >          goto out;
> > > >      
> > > >      }
> > > > 
> > > > +    s->root_ino = stat.st_ino;
> > > 
> > > This isn't used anywhere. It looks like a leftover of the readdir fix
> > > in v5.
> > 
> > Yes, both correct. I intentionally left it though, since I found it a
> > natural complement always capturing the root inode along to the root
> > device.
> Fair enough. The local backend opens an fd to the root directory, to be used
> by any access to the 9p share. I think root_dev/root_ino should be obtained
> with fstat() on this fd, to be sure they are consistent. Maybe add an extra
> struct stat * argument to the init function ? I'd rather see this done as a
> preparatory "backend to cache 9p root device/inode during init" patch.

Convinced. I'll drop root_ino from this patch set for now.

[Qemu-devel] [PATCH v6 3/4] 9p: stat_to_qid: implement slow path

[Christian Schoenebeck via Qemu-devel]

stat_to_qid attempts via qid_path_prefixmap to map unique files (which are
identified by 64 bit inode nr and 32 bit device id) to a 64 QID path value.
However this implementation makes some assumptions about inode number
generation on the host.

If qid_path_prefixmap fails, we still have 48 bits available in the QID
path to fall back to a less memory efficient full mapping.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
[CS: - Rebased to https://github.com/gkurz/qemu/commits/9p-next
       (SHA1 177fd3b6a8).
     - Updated hash calls to new xxhash API.
     - Removed unnecessary parantheses in qpf_lookup_func().
     - Removed unnecessary g_malloc0() result checks.
     - Log error message when running out of prefixes in
       qid_path_fullmap().
     - Log error message about potential degraded performance in
       qid_path_prefixmap().
     - Fixed typo in comment. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/9pfs/9p.h |  9 ++++++++
 2 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c96ea51116..728641fb7f 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -579,23 +579,73 @@ static uint32_t qpp_hash(QppEntry e)
     return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
 }
 
+static uint32_t qpf_hash(QpfEntry e)
+{
+    return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
+}
+
 static bool qpp_lookup_func(const void *obj, const void *userp)
 {
     const QppEntry *e1 = obj, *e2 = userp;
     return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
 }
 
-static void qpp_table_remove(void *p, uint32_t h, void *up)
+static bool qpf_lookup_func(const void *obj, const void *userp)
+{
+    const QpfEntry *e1 = obj, *e2 = userp;
+    return e1->dev == e2->dev && e1->ino == e2->ino;
+}
+
+static void qp_table_remove(void *p, uint32_t h, void *up)
 {
     g_free(p);
 }
 
-static void qpp_table_destroy(struct qht *ht)
+static void qp_table_destroy(struct qht *ht)
 {
-    qht_iter(ht, qpp_table_remove, NULL);
+    qht_iter(ht, qp_table_remove, NULL);
     qht_destroy(ht);
 }
 
+static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
+                            uint64_t *path)
+{
+    QpfEntry lookup = {
+        .dev = stbuf->st_dev,
+        .ino = stbuf->st_ino
+    }, *val;
+    uint32_t hash = qpf_hash(lookup);
+
+    /* most users won't need the fullmap, so init the table lazily */
+    if (!pdu->s->qpf_table.map) {
+        qht_init(&pdu->s->qpf_table, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
+    }
+
+    val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
+
+    if (!val) {
+        if (pdu->s->qp_fullpath_next == 0) {
+            /* no more files can be mapped :'( */
+            error_report_once(
+                "9p: No more prefixes available for remapping inodes from "
+                "host to guest."
+            );
+            return -ENFILE;
+        }
+
+        val = g_malloc0(sizeof(QppEntry));
+        *val = lookup;
+
+        /* new unique inode and device combo */
+        val->path = pdu->s->qp_fullpath_next++;
+        pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+        qht_insert(&pdu->s->qpf_table, val, hash, NULL);
+    }
+
+    *path = val->path;
+    return 0;
+}
+
 /* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
  * to a unique QID path (64 bits). To avoid having to map and keep track
  * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
@@ -621,8 +671,7 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
         if (pdu->s->qp_prefix_next == 0) {
             /* we ran out of prefixes */
             error_report_once(
-                "9p: No more prefixes available for remapping inodes from "
-                "host to guest."
+                "9p: Potential degraded performance of inode remapping"
             );
             return -ENFILE;
         }
@@ -647,6 +696,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
     if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
         /* map inode+device to qid path (fast path) */
         err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+        if (err == -ENFILE) {
+            /* fast path didn't work, fall back to full map */
+            err = qid_path_fullmap(pdu, stbuf, &qidp->path);
+        }
         if (err) {
             return err;
         }
@@ -3813,6 +3866,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     /* QID path hash table. 1 entry ought to be enough for anybody ;) */
     qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
     s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+    s->qp_fullpath_next = 1;
 
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
@@ -3827,7 +3881,8 @@ out:
         }
         g_free(s->tag);
         g_free(s->ctx.fs_root);
-        qpp_table_destroy(&s->qpp_table);
+        qp_table_destroy(&s->qpp_table);
+        qp_table_destroy(&s->qpf_table);
         v9fs_path_free(&path);
     }
     return rc;
@@ -3840,7 +3895,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
     }
     fsdev_throttle_cleanup(s->ctx.fst);
     g_free(s->tag);
-    qpp_table_destroy(&s->qpp_table);
+    qp_table_destroy(&s->qpp_table);
+    qp_table_destroy(&s->qpf_table);
     g_free(s->ctx.fs_root);
 }
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index a283b0193e..f044a88a41 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -245,6 +245,13 @@ typedef struct {
     uint16_t qp_prefix;
 } QppEntry;
 
+/* QID path full entry, as above */
+typedef struct {
+    dev_t dev;
+    ino_t ino;
+    uint64_t path;
+} QpfEntry;
+
 struct V9fsState
 {
     QLIST_HEAD(, V9fsPDU) free_list;
@@ -269,7 +276,9 @@ struct V9fsState
     ino_t root_ino;
     dev_t dev_id;
     struct qht qpp_table;
+    struct qht qpf_table;
     uint16_t qp_prefix_next;
+    uint64_t qp_fullpath_next;
 };
 
 /* 9p2000.L open flags */
-- 
2.11.0

[Qemu-devel] [PATCH v6 4/4] 9p: Use variable length suffixes for inode remapping

[Christian Schoenebeck via Qemu-devel]

Use variable length suffixes for inode remapping instead of the fixed
16 bit size prefixes before. With this change the inode numbers on guest
will typically be much smaller (e.g. around >2^1 .. >2^7 instead of >2^48
with the previous fixed size inode remapping.

Additionally this solution is more efficient, since inode numbers in
practice can take almost their entire 64 bit range on guest as well, so
there is less likely a need for generating and tracking additional suffixes,
which might also be beneficial for nested virtualization where each level of
virtualization would shift up the inode bits and increase the chance of
expensive remapping actions.

The "Exponential Golomb" algorithm is used as basis for generating the
variable length suffixes. The algorithm has a parameter k which controls the
distribution of bits on increasing indeces (minimum bits at low index vs.
maximum bits at high index). With k=0 the generated suffixes look like:

Index Dec/Bin -> Generated Suffix Bin
1 [1] -> [1] (1 bits)
2 [10] -> [010] (3 bits)
3 [11] -> [110] (3 bits)
4 [100] -> [00100] (5 bits)
5 [101] -> [10100] (5 bits)
6 [110] -> [01100] (5 bits)
7 [111] -> [11100] (5 bits)
8 [1000] -> [0001000] (7 bits)
9 [1001] -> [1001000] (7 bits)
10 [1010] -> [0101000] (7 bits)
11 [1011] -> [1101000] (7 bits)
12 [1100] -> [0011000] (7 bits)
...
65533 [1111111111111101] ->  [1011111111111111000000000000000] (31 bits)
65534 [1111111111111110] ->  [0111111111111111000000000000000] (31 bits)
65535 [1111111111111111] ->  [1111111111111111000000000000000] (31 bits)
Hence minBits=1 maxBits=31

And with k=5 they would look like:

Index Dec/Bin -> Generated Suffix Bin
1 [1] -> [000001] (6 bits)
2 [10] -> [100001] (6 bits)
3 [11] -> [010001] (6 bits)
4 [100] -> [110001] (6 bits)
5 [101] -> [001001] (6 bits)
6 [110] -> [101001] (6 bits)
7 [111] -> [011001] (6 bits)
8 [1000] -> [111001] (6 bits)
9 [1001] -> [000101] (6 bits)
10 [1010] -> [100101] (6 bits)
11 [1011] -> [010101] (6 bits)
12 [1100] -> [110101] (6 bits)
...
65533 [1111111111111101] -> [0011100000000000100000000000] (28 bits)
65534 [1111111111111110] -> [1011100000000000100000000000] (28 bits)
65535 [1111111111111111] -> [0111100000000000100000000000] (28 bits)
Hence minBits=6 maxBits=28

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/9pfs/9p.h |  34 +++++++-
 2 files changed, 251 insertions(+), 30 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 728641fb7f..0359469cfa 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -26,6 +26,7 @@
 #include "migration/blocker.h"
 #include "sysemu/qtest.h"
 #include "qemu/xxhash.h"
+#include <math.h>
 
 int open_fd_hw;
 int total_open_fd;
@@ -572,6 +573,107 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_NAMED_PIPE |   \
                                 P9_STAT_MODE_SOCKET)
 
+/* Mirrors all bits of a byte. So e.g. binary 10100000 would become 00000101. */
+static inline uint8_t mirror8bit(uint8_t byte)
+{
+    return (byte * 0x0202020202ULL & 0x010884422010ULL) % 1023;
+}
+
+/* Same as mirror8bit() just for a 64 bit data type instead for a byte. */
+static inline uint64_t mirror64bit(uint64_t value)
+{
+    return ((uint64_t)mirror8bit( value        & 0xff) << 56) |
+           ((uint64_t)mirror8bit((value >> 8)  & 0xff) << 48) |
+           ((uint64_t)mirror8bit((value >> 16) & 0xff) << 40) |
+           ((uint64_t)mirror8bit((value >> 24) & 0xff) << 32) |
+           ((uint64_t)mirror8bit((value >> 32) & 0xff) << 24) |
+           ((uint64_t)mirror8bit((value >> 40) & 0xff) << 16) |
+           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
+           ((uint64_t)mirror8bit((value >> 56) & 0xff)      ) ;
+}
+
+/** @brief Parameter k for the Exponential Golomb algorihm to be used.
+ *
+ * The smaller this value, the smaller the minimum bit count for the Exp.
+ * Golomb generated affixes will be (at lowest index) however for the
+ * price of having higher maximum bit count of generated affixes (at highest
+ * index). Likewise increasing this parameter yields in smaller maximum bit
+ * count for the price of having higher minimum bit count.
+ *
+ * In practice that means: a good value for k depends on the expected amount
+ * of devices to be exposed by one export. For a small amount of devices k
+ * should be small, for a large amount of devices k might be increased
+ * instead. The default of k=0 should be fine for most users though.
+ *
+ * @b IMPORTANT: In case this ever becomes a runtime parameter; the value of
+ * k should not change as long as guest is still running! Because that would
+ * cause completely different inode numbers to be generated on guest.
+ */
+#define EXP_GOLOMB_K    0
+
+/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
+ *
+ * The Exponential Golomb algorithm generates @b prefixes (@b not suffixes!)
+ * with growing length and with the mathematical property of being
+ * "prefix-free". The latter means the generated prefixes can be prepended
+ * in front of arbitrary numbers and the resulting concatenated numbers are
+ * guaranteed to be always unique.
+ *
+ * This is a minor adjustment to the original Exp. Golomb algorithm in the
+ * sense that lowest allowed index (@param n) starts with 1, not with zero.
+ *
+ * @param n - natural number (or index) of the prefix to be generated
+ *            (1, 2, 3, ...)
+ * @param k - parameter k of Exp. Golomb algorithm to be used
+ *            (see comment on EXP_GOLOMB_K macro for details about k)
+ */
+static VariLenAffix expGolombEncode(uint64_t n, int k)
+{
+    const uint64_t value = n + (1 << k) - 1;
+    const int bits = (int) log2(value) + 1;
+    return (VariLenAffix) {
+        .type = AffixType_Prefix,
+        .value = value,
+        .bits = bits + MAX((bits - 1 - k), 0)
+    };
+}
+
+/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
+ *
+ * Simply mirror all bits of the affix value, for the purpose to preserve
+ * respectively the mathematical "prefix-free" or "suffix-free" property
+ * after the conversion.
+ *
+ * If a passed prefix is suitable to create unique numbers, then the
+ * returned suffix is suitable to create unique numbers as well (and vice
+ * versa).
+ */
+static VariLenAffix invertAffix(const VariLenAffix* affix)
+{
+    return (VariLenAffix) {
+        .type = (affix->type == AffixType_Suffix) ? AffixType_Prefix : AffixType_Suffix,
+        .value =  mirror64bit(affix->value) >> ((sizeof(affix->value) * 8) - affix->bits),
+        .bits = affix->bits
+    };
+}
+
+/** @brief Generates suffix numbers with "suffix-free" property.
+ *
+ * This is just a wrapper function on top of the Exp. Golomb algorithm.
+ *
+ * Since the Exp. Golomb algorithm generates prefixes, but we need suffixes,
+ * this function converts the Exp. Golomb prefixes into appropriate suffixes
+ * which are still suitable for generating unique numbers.
+ *
+ * @param n - natural number (or index) of the suffix to be generated
+ *            (1, 2, 3, ...)
+ */
+static VariLenAffix affixForIndex(uint64_t index)
+{
+    VariLenAffix prefix;
+    prefix = expGolombEncode(index, EXP_GOLOMB_K);
+    return invertAffix(&prefix); /* convert prefix to suffix */
+}
 
 /* creative abuse of tb_hash_func7, which is based on xxhash */
 static uint32_t qpp_hash(QppEntry e)
@@ -584,13 +686,19 @@ static uint32_t qpf_hash(QpfEntry e)
     return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
 }
 
-static bool qpp_lookup_func(const void *obj, const void *userp)
+static bool qpd_cmp_func(const void *obj, const void *userp)
+{
+    const QpdEntry *e1 = obj, *e2 = userp;
+    return e1->dev == e2->dev;
+}
+
+static bool qpp_cmp_func(const void *obj, const void *userp)
 {
     const QppEntry *e1 = obj, *e2 = userp;
     return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
 }
 
-static bool qpf_lookup_func(const void *obj, const void *userp)
+static bool qpf_cmp_func(const void *obj, const void *userp)
 {
     const QpfEntry *e1 = obj, *e2 = userp;
     return e1->dev == e2->dev && e1->ino == e2->ino;
@@ -607,6 +715,54 @@ static void qp_table_destroy(struct qht *ht)
     qht_destroy(ht);
 }
 
+/*
+ * Returns how many (high end) bits of inode numbers of the passed fs
+ * device shall be used (in combination with the device number) to
+ * generate hash values for qpp_table entries.
+ *
+ * This function is required if variable length suffixes are used for inode
+ * number mapping on guest level. Since a device may end up having multiple
+ * entries in qpp_table, each entry most probably with a different suffix
+ * length, we thus need this function in conjunction with qpd_table to
+ * "agree" about a fix amount of bits (per device) to be always used for
+ * generating hash values for the purpose of accessing qpp_table in order
+ * get consistent behaviour when accessing qpp_table.
+ */
+static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t dev)
+{
+    QpdEntry lookup = {
+        .dev = dev
+    }, *val;
+    uint32_t hash = dev;
+    VariLenAffix affix;
+
+    val = qht_lookup(&pdu->s->qpd_table, &lookup, hash);
+    if (!val) {
+        val = g_malloc0(sizeof(QpdEntry));
+        *val = lookup;
+        affix = affixForIndex(pdu->s->qp_affix_next);
+        val->prefix_bits = affix.bits;
+        qht_insert(&pdu->s->qpd_table, val, hash, NULL);
+        pdu->s->qp_ndevices++;
+    }
+    return val->prefix_bits;
+}
+
+/** @brief Slow / full mapping host inode nr -> guest inode nr.
+ *
+ * This function performs a slower and much more costly remapping of an
+ * original file inode number on host to an appropriate different inode
+ * number on guest. For every (dev, inode) combination on host a new
+ * sequential number is generated, cached and exposed as inode number on
+ * guest.
+ *
+ * This is just a "last resort" fallback solution if the much faster/cheaper
+ * qid_path_prefixmap() failed. In practice this slow / full mapping is not
+ * expected ever to be used at all though.
+ *
+ * @see qid_path_prefixmap() for details
+ *
+ */
 static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
                             uint64_t *path)
 {
@@ -615,11 +771,7 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
         .ino = stbuf->st_ino
     }, *val;
     uint32_t hash = qpf_hash(lookup);
-
-    /* most users won't need the fullmap, so init the table lazily */
-    if (!pdu->s->qpf_table.map) {
-        qht_init(&pdu->s->qpf_table, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
-    }
+    VariLenAffix affix;
 
     val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
 
@@ -637,8 +789,11 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
         *val = lookup;
 
         /* new unique inode and device combo */
-        val->path = pdu->s->qp_fullpath_next++;
-        pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+        affix = affixForIndex(
+            1ULL << (sizeof(pdu->s->qp_affix_next) * 8)
+        );
+        val->path = (pdu->s->qp_fullpath_next++ << affix.bits) | affix.value;
+        pdu->s->qp_fullpath_next &= ((1ULL << (64 - affix.bits)) - 1);
         qht_insert(&pdu->s->qpf_table, val, hash, NULL);
     }
 
@@ -646,30 +801,59 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
     return 0;
 }
 
-/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
- * to a unique QID path (64 bits). To avoid having to map and keep track
- * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
- * the device id to the 16 highest bits of the QID path. The 48 lowest bits
- * of the QID path equal to the lowest bits of the inode number.
+/** @brief Quick mapping host inode nr -> guest inode nr.
  *
- * This takes advantage of the fact that inode number are usually not
- * random but allocated sequentially, so we have fewer items to keep
- * track of.
+ * This function performs quick remapping of an original file inode number
+ * on host to an appropriate different inode number on guest. This remapping
+ * of inodes is required to avoid inode nr collisions on guest which would
+ * happen if the 9p export contains more than 1 exported file system (or
+ * more than 1 file system data set), because unlike on host level where the
+ * files would have different device nrs, all files exported by 9p would
+ * share the same device nr on guest (the device nr of the virtual 9p device
+ * that is).
+ *
+ * Inode remapping is performed by chopping off high end bits of the original
+ * inode number from host, shifting the result upwards and then assigning a
+ * generated suffix number for the low end bits, where the same suffix number
+ * will be shared by all inodes with the same device id AND the same high end
+ * bits that have been chopped off. That approach utilizes the fact that inode
+ * numbers very likely share the same high end bits (i.e. due to their common
+ * sequential generation by file systems) and hence we only have to generate
+ * and track a very limited amount of suffixes in practice due to that.
+ *
+ * We generate variable size suffixes for that purpose. The 1st generated
+ * suffix will only have 1 bit and hence we only need to chop off 1 bit from
+ * the original inode number. The subsequent suffixes being generated will
+ * grow in (bit) size subsequently, i.e. the 2nd and 3rd suffix being
+ * generated will have 3 bits and hence we have to chop off 3 bits from their
+ * original inodes, and so on. That approach of using variable length suffixes
+ * (i.e. over fixed size ones) utilizes the fact that in practice only a very
+ * limited amount of devices are shared by the same export (e.g. typically
+ * less than 2 dozen devices per 9p export), so in practice we need to chop
+ * off less bits than with fixed size prefixes and yet are flexible to add
+ * new devices at runtime below host's export directory at any time without
+ * having to reboot guest nor requiring to reconfigure guest for that. And due
+ * to the very limited amount of original high end bits that we chop off that
+ * way, the total amount of suffixes we need to generate is less than by using
+ * fixed size prefixes and hence it also improves performance of the inode
+ * remapping algorithm, and finally has the nice side effect that the inode
+ * numbers on guest will be much smaller & human friendly. ;-)
  */
 static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
                                 uint64_t *path)
 {
+    const int ino_hash_bits = qid_inode_prefix_hash_bits(pdu, stbuf->st_dev);
     QppEntry lookup = {
         .dev = stbuf->st_dev,
-        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
+        .ino_prefix = (uint16_t) (stbuf->st_ino >> (64-ino_hash_bits))
     }, *val;
     uint32_t hash = qpp_hash(lookup);
 
     val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
 
     if (!val) {
-        if (pdu->s->qp_prefix_next == 0) {
-            /* we ran out of prefixes */
+        if (pdu->s->qp_affix_next == 0) {
+            /* we ran out of affixes */
             error_report_once(
                 "9p: Potential degraded performance of inode remapping"
             );
@@ -679,12 +863,13 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
         val = g_malloc0(sizeof(QppEntry));
         *val = lookup;
 
-        /* new unique inode prefix and device combo */
-        val->qp_prefix = pdu->s->qp_prefix_next++;
+        /* new unique inode affix and device combo */
+        val->qp_affix_index = pdu->s->qp_affix_next++;
+        val->qp_affix = affixForIndex(val->qp_affix_index);
         qht_insert(&pdu->s->qpp_table, val, hash, NULL);
     }
-
-    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+    /* assuming generated affix to be suffix type, not prefix */
+    *path = (stbuf->st_ino << val->qp_affix.bits) | val->qp_affix.value;
     return 0;
 }
 
@@ -3863,9 +4048,15 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     s->root_ino = stat.st_ino;
     s->dev_id = stat.st_dev;
 
-    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
-    qht_init(&s->qpp_table, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
-    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+    /* init inode remapping : */
+    /* hash table for variable length inode suffixes */
+    qht_init(&s->qpd_table, qpd_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
+    /* hash table for slow/full inode remapping (most users won't need it) */
+    qht_init(&s->qpf_table, qpf_cmp_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
+    /* hash table for quick inode remapping */
+    qht_init(&s->qpp_table, qpp_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
+    s->qp_ndevices = 0;
+    s->qp_affix_next = 1; /* reserve 0 to detect overflow */
     s->qp_fullpath_next = 1;
 
     s->ctx.fst = &fse->fst;
@@ -3881,6 +4072,7 @@ out:
         }
         g_free(s->tag);
         g_free(s->ctx.fs_root);
+        qp_table_destroy(&s->qpd_table);
         qp_table_destroy(&s->qpp_table);
         qp_table_destroy(&s->qpf_table);
         v9fs_path_free(&path);
@@ -3895,6 +4087,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
     }
     fsdev_throttle_cleanup(s->ctx.fst);
     g_free(s->tag);
+    qp_table_destroy(&s->qpd_table);
     qp_table_destroy(&s->qpp_table);
     qp_table_destroy(&s->qpf_table);
     g_free(s->ctx.fs_root);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index f044a88a41..597b2c7222 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -236,13 +236,39 @@ struct V9fsFidState
     V9fsFidState *rclm_lst;
 };
 
-#define QPATH_INO_MASK        ((1ULL << 48) - 1)
+typedef enum AffixType_t {
+    AffixType_Prefix,
+    AffixType_Suffix, /* A.k.a. postfix. */
+} AffixType_t;
+
+/** @brief Unique affix of variable length.
+ *
+ * An affix is (currently) either a suffix or a prefix, which is either
+ * going to be prepended (prefix) or appended (suffix) with some other
+ * number for the goal to generate unique numbers. Accordingly the
+ * suffixes (or prefixes) we generate @b must all have the mathematical
+ * property of being suffix-free (or prefix-free in case of prefixes)
+ * so that no matter what number we concatenate the affix with, that we
+ * always reliably get unique numbers as result after concatenation.
+ */
+typedef struct VariLenAffix {
+    AffixType_t type; /* Whether this affix is a suffix or a prefix. */
+    uint64_t value; /* Actual numerical value of this affix. */
+    int bits; /* Lenght of the affix, that is how many (of the lowest) bits of @c value must be used for appending/prepending this affix to its final resulting, unique number. */
+} VariLenAffix;
+
+/* See qid_inode_prefix_hash_bits(). */
+typedef struct {
+    dev_t dev; /* FS device on host. */
+    int prefix_bits; /* How many (high) bits of the original inode number shall be used for hashing. */
+} QpdEntry;
 
 /* QID path prefix entry, see stat_to_qid */
 typedef struct {
     dev_t dev;
     uint16_t ino_prefix;
-    uint16_t qp_prefix;
+    uint32_t qp_affix_index;
+    VariLenAffix qp_affix;
 } QppEntry;
 
 /* QID path full entry, as above */
@@ -275,9 +301,11 @@ struct V9fsState
     V9fsQID root_qid;
     ino_t root_ino;
     dev_t dev_id;
+    struct qht qpd_table;
     struct qht qpp_table;
     struct qht qpf_table;
-    uint16_t qp_prefix_next;
+    uint64_t qp_ndevices; /* Amount of entries in qpd_table. */
+    uint16_t qp_affix_next;
     uint64_t qp_fullpath_next;
 };
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/



Hi,

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

Type: series
Subject: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
Message-id: cover.1566503584.git.qemu_oss@crudebyte.com

=== 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
 * [new tag]         patchew/cover.1566503584.git.qemu_oss@crudebyte.com -> patchew/cover.1566503584.git.qemu_oss@crudebyte.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out '7bfe584e321946771692711ff83ad2b5850daca7'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
28d6fb8 9p: Use variable length suffixes for inode remapping
23fc4f6 9p: stat_to_qid: implement slow path
f227708 9p: Added virtfs option 'multidevs=remap|forbid|warn'
bb69de6 9p: Treat multiple devices on one export as an error

=== OUTPUT BEGIN ===
1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export as an error)
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>

total: 1 errors, 0 warnings, 175 lines checked

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

2/4 Checking commit f227708a87da (9p: Added virtfs option 'multidevs=remap|forbid|warn')
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>

WARNING: Block comments use a leading /* on a separate line
#167: FILE: hw/9pfs/9p.c:600:
+/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)

ERROR: line over 90 characters
#467: FILE: vl.c:3338:
+                const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;

total: 2 errors, 1 warnings, 394 lines checked

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

3/4 Checking commit 23fc4f6d5258 (9p: stat_to_qid: implement slow path)
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>

WARNING: line over 80 characters
#79: FILE: hw/9pfs/9p.c:622:
+        qht_init(&pdu->s->qpf_table, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);

total: 1 errors, 1 warnings, 142 lines checked

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

4/4 Checking commit 28d6fb899725 (9p: Use variable length suffixes for inode remapping)
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>

ERROR: space prohibited after that open parenthesis '('
#92: FILE: hw/9pfs/9p.c:586:
+    return ((uint64_t)mirror8bit( value        & 0xff) << 56) |

ERROR: space prohibited before that close parenthesis ')'
#98: FILE: hw/9pfs/9p.c:592:
+           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |

ERROR: space prohibited before that close parenthesis ')'
#99: FILE: hw/9pfs/9p.c:593:
+           ((uint64_t)mirror8bit((value >> 56) & 0xff)      ) ;

WARNING: Block comments use a leading /* on a separate line
#102: FILE: hw/9pfs/9p.c:596:
+/** @brief Parameter k for the Exponential Golomb algorihm to be used.

WARNING: Block comments use a leading /* on a separate line
#121: FILE: hw/9pfs/9p.c:615:
+/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).

WARNING: Block comments use a leading /* on a separate line
#148: FILE: hw/9pfs/9p.c:642:
+/** @brief Converts a suffix into a prefix, or a prefix into a suffix.

ERROR: "foo* bar" should be "foo *bar"
#158: FILE: hw/9pfs/9p.c:652:
+static VariLenAffix invertAffix(const VariLenAffix* affix)

WARNING: line over 80 characters
#161: FILE: hw/9pfs/9p.c:655:
+        .type = (affix->type == AffixType_Suffix) ? AffixType_Prefix : AffixType_Suffix,

WARNING: line over 80 characters
#162: FILE: hw/9pfs/9p.c:656:
+        .value =  mirror64bit(affix->value) >> ((sizeof(affix->value) * 8) - affix->bits),

WARNING: Block comments use a leading /* on a separate line
#167: FILE: hw/9pfs/9p.c:661:
+/** @brief Generates suffix numbers with "suffix-free" property.

WARNING: Block comments use a leading /* on a separate line
#246: FILE: hw/9pfs/9p.c:752:
+/** @brief Slow / full mapping host inode nr -> guest inode nr.

WARNING: Block comments use a leading /* on a separate line
#300: FILE: hw/9pfs/9p.c:805:
+/** @brief Quick mapping host inode nr -> guest inode nr.

ERROR: spaces required around that '-' (ctx:VxV)
#348: FILE: hw/9pfs/9p.c:849:
+        .ino_prefix = (uint16_t) (stbuf->st_ino >> (64-ino_hash_bits))
                                                       ^

WARNING: Block comments use a leading /* on a separate line
#429: FILE: hw/9pfs/9p.h:244:
+/** @brief Unique affix of variable length.

ERROR: line over 90 characters
#442: FILE: hw/9pfs/9p.h:257:
+    int bits; /* Lenght of the affix, that is how many (of the lowest) bits of @c value must be used for appending/prepending this affix to its final resulting, unique number. */

ERROR: line over 90 characters
#448: FILE: hw/9pfs/9p.h:263:
+    int prefix_bits; /* How many (high) bits of the original inode number shall be used for hashing. */

total: 8 errors, 9 warnings, 386 lines checked

Patch 4/4 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/cover.1566503584.git.qemu_oss@crudebyte.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Greg Kurz]

On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
no-reply@patchew.org wrote:

> Patchew URL: https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
> Message-id: cover.1566503584.git.qemu_oss@crudebyte.com
> 
> === 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
>  * [new tag]         patchew/cover.1566503584.git.qemu_oss@crudebyte.com -> patchew/cover.1566503584.git.qemu_oss@crudebyte.com
> Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
> Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
> Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
> Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
> Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
> Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
> Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
> Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
> Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
> Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
> Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
> Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
> Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
> Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
> Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
> Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
> Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
> Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
> Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
> Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
> Cloning into 'capstone'...
> Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
> Cloning into 'dtc'...
> Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
> Cloning into 'roms/QemuMacDrivers'...
> Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
> Cloning into 'roms/SLOF'...
> Submodule path 'roms/SLOF': checked out '7bfe584e321946771692711ff83ad2b5850daca7'
> Cloning into 'roms/edk2'...
> Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
> Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
> Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
> Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
> Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
> Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
> Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
> Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
> Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
> Cloning into 'boringssl'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
> Cloning into 'krb5'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
> Cloning into 'pyca-cryptography'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
> Cloning into 'roms/ipxe'...
> Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
> Cloning into 'roms/openbios'...
> Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
> Cloning into 'roms/openhackware'...
> Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
> Cloning into 'roms/opensbi'...
> Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
> Cloning into 'roms/qemu-palcode'...
> Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
> Cloning into 'roms/seabios'...
> Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
> Cloning into 'roms/seabios-hppa'...
> Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
> Cloning into 'roms/sgabios'...
> Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
> Cloning into 'roms/skiboot'...
> Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
> Cloning into 'roms/u-boot'...
> Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
> Cloning into 'roms/u-boot-sam460ex'...
> Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
> Cloning into 'slirp'...
> Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
> Cloning into 'tests/fp/berkeley-softfloat-3'...
> Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
> Cloning into 'tests/fp/berkeley-testfloat-3'...
> Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
> Cloning into 'ui/keycodemapdb'...
> Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
> Switched to a new branch 'test'
> 28d6fb8 9p: Use variable length suffixes for inode remapping
> 23fc4f6 9p: stat_to_qid: implement slow path
> f227708 9p: Added virtfs option 'multidevs=remap|forbid|warn'
> bb69de6 9p: Treat multiple devices on one export as an error
> 
> === OUTPUT BEGIN ===
> 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export as an error)
> ERROR: Author email address is mangled by the mailing list
> #2: 
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> 

This is problematic since it ends up in the Author: field in git. Please find
a way to fix that.

Other warnings/errors should also be fixed but they look trivial.

> total: 1 errors, 0 warnings, 175 lines checked
> 
> Patch 1/4 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 2/4 Checking commit f227708a87da (9p: Added virtfs option 'multidevs=remap|forbid|warn')
> ERROR: Author email address is mangled by the mailing list
> #2: 
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> 
> WARNING: Block comments use a leading /* on a separate line
> #167: FILE: hw/9pfs/9p.c:600:
> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> 
> ERROR: line over 90 characters
> #467: FILE: vl.c:3338:
> +                const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;
> 
> total: 2 errors, 1 warnings, 394 lines checked
> 
> Patch 2/4 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 3/4 Checking commit 23fc4f6d5258 (9p: stat_to_qid: implement slow path)
> ERROR: Author email address is mangled by the mailing list
> #2: 
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> 
> WARNING: line over 80 characters
> #79: FILE: hw/9pfs/9p.c:622:
> +        qht_init(&pdu->s->qpf_table, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
> 
> total: 1 errors, 1 warnings, 142 lines checked
> 
> Patch 3/4 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 4/4 Checking commit 28d6fb899725 (9p: Use variable length suffixes for inode remapping)
> ERROR: Author email address is mangled by the mailing list
> #2: 
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> 
> ERROR: space prohibited after that open parenthesis '('
> #92: FILE: hw/9pfs/9p.c:586:
> +    return ((uint64_t)mirror8bit( value        & 0xff) << 56) |
> 
> ERROR: space prohibited before that close parenthesis ')'
> #98: FILE: hw/9pfs/9p.c:592:
> +           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> 
> ERROR: space prohibited before that close parenthesis ')'
> #99: FILE: hw/9pfs/9p.c:593:
> +           ((uint64_t)mirror8bit((value >> 56) & 0xff)      ) ;
> 
> WARNING: Block comments use a leading /* on a separate line
> #102: FILE: hw/9pfs/9p.c:596:
> +/** @brief Parameter k for the Exponential Golomb algorihm to be used.
> 
> WARNING: Block comments use a leading /* on a separate line
> #121: FILE: hw/9pfs/9p.c:615:
> +/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
> 
> WARNING: Block comments use a leading /* on a separate line
> #148: FILE: hw/9pfs/9p.c:642:
> +/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
> 
> ERROR: "foo* bar" should be "foo *bar"
> #158: FILE: hw/9pfs/9p.c:652:
> +static VariLenAffix invertAffix(const VariLenAffix* affix)
> 
> WARNING: line over 80 characters
> #161: FILE: hw/9pfs/9p.c:655:
> +        .type = (affix->type == AffixType_Suffix) ? AffixType_Prefix : AffixType_Suffix,
> 
> WARNING: line over 80 characters
> #162: FILE: hw/9pfs/9p.c:656:
> +        .value =  mirror64bit(affix->value) >> ((sizeof(affix->value) * 8) - affix->bits),
> 
> WARNING: Block comments use a leading /* on a separate line
> #167: FILE: hw/9pfs/9p.c:661:
> +/** @brief Generates suffix numbers with "suffix-free" property.
> 
> WARNING: Block comments use a leading /* on a separate line
> #246: FILE: hw/9pfs/9p.c:752:
> +/** @brief Slow / full mapping host inode nr -> guest inode nr.
> 
> WARNING: Block comments use a leading /* on a separate line
> #300: FILE: hw/9pfs/9p.c:805:
> +/** @brief Quick mapping host inode nr -> guest inode nr.
> 
> ERROR: spaces required around that '-' (ctx:VxV)
> #348: FILE: hw/9pfs/9p.c:849:
> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> (64-ino_hash_bits))
>                                                        ^
> 
> WARNING: Block comments use a leading /* on a separate line
> #429: FILE: hw/9pfs/9p.h:244:
> +/** @brief Unique affix of variable length.
> 
> ERROR: line over 90 characters
> #442: FILE: hw/9pfs/9p.h:257:
> +    int bits; /* Lenght of the affix, that is how many (of the lowest) bits of @c value must be used for appending/prepending this affix to its final resulting, unique number. */
> 
> ERROR: line over 90 characters
> #448: FILE: hw/9pfs/9p.h:263:
> +    int prefix_bits; /* How many (high) bits of the original inode number shall be used for hashing. */
> 
> total: 8 errors, 9 warnings, 386 lines checked
> 
> Patch 4/4 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/cover.1566503584.git.qemu_oss@crudebyte.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via Qemu-devel]

On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
> 
> no-reply@patchew.org wrote:
> > Patchew URL:
> > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> > 
> > 
> > 
> > Hi,
> > 
> > This series seems to have some coding style problems. See output below for
> > more information:
[snip]
> > 
> > === OUTPUT BEGIN ===
> > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export
> > as an error) ERROR: Author email address is mangled by the mailing list
> > #2:
> > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> 
> This is problematic since it ends up in the Author: field in git. Please
> find a way to fix that.

Like in which way do you imagine that? And where is the actual practical 
problem? I mean every patch still has my signed-off-by tag with the correct 
email address ending up in git history.

The cause for this issue is that the domain is configured to require DKIM 
signatures for all outgoing emails. That's why mailman replaces my address by
"Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>" placeholder 
since it could not provide a valid signature.

There were good reasons for enabling DKIM and it is a good thing for all 
domains in general, since that ensures that (i.e. foreign) email addresses 
cannot be used as sender address if the actual sender is not authorized for 
sending emails with that address.

What I changed in the meantime though is that you should get all my patches 
directly to your personal address, not only from the list. Or did you receive 
v6 again just from the list?

> Other warnings/errors should also be fixed but they look trivial.

Yeah, they are trivial. *But* there is one thing ...

> > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > 
> > ERROR: space prohibited after that open parenthesis '('
> > #92: FILE: hw/9pfs/9p.c:586:
> > +    return ((uint64_t)mirror8bit( value        & 0xff) << 56) |
> > 
> > ERROR: space prohibited before that close parenthesis ')'
> > #98: FILE: hw/9pfs/9p.c:592:
> > +           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> > 
> > ERROR: space prohibited before that close parenthesis ')'
> > #99: FILE: hw/9pfs/9p.c:593:
> > +           ((uint64_t)mirror8bit((value >> 56) & 0xff)      ) ;

... I would like to ignore this specific bot whining, because that particular 
function looks much more readable the way it is (in that patch) right now.

> > WARNING: Block comments use a leading /* on a separate line
> > #102: FILE: hw/9pfs/9p.c:596:
> > +/** @brief Parameter k for the Exponential Golomb algorihm to be used.
> > 
> > WARNING: Block comments use a leading /* on a separate line
> > #121: FILE: hw/9pfs/9p.c:615:
> > +/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
> > 
> > WARNING: Block comments use a leading /* on a separate line
> > #148: FILE: hw/9pfs/9p.c:642:
> > +/** @brief Converts a suffix into a prefix, or a prefix into a suffix.

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Greg Kurz]

On Sun, 01 Sep 2019 21:28:45 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
> > 
> > no-reply@patchew.org wrote:
> > > Patchew URL:
> > > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> > > 
> > > 
> > > 
> > > Hi,
> > > 
> > > This series seems to have some coding style problems. See output below for
> > > more information:
> [snip]
> > > 
> > > === OUTPUT BEGIN ===
> > > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export
> > > as an error) ERROR: Author email address is mangled by the mailing list
> > > #2:
> > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > 
> > This is problematic since it ends up in the Author: field in git. Please
> > find a way to fix that.
> 
> Like in which way do you imagine that? And where is the actual practical 
> problem? I mean every patch still has my signed-off-by tag with the correct 
> email address ending up in git history.
> 

Yes, this only breaks Author: if the patch is applied from the list.

> The cause for this issue is that the domain is configured to require DKIM 
> signatures for all outgoing emails. That's why mailman replaces my address by
> "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>" placeholder 
> since it could not provide a valid signature.
> 
> There were good reasons for enabling DKIM and it is a good thing for all 
> domains in general, since that ensures that (i.e. foreign) email addresses 
> cannot be used as sender address if the actual sender is not authorized for 
> sending emails with that address.
> 

Don't know much about DKIM but google seems to confirm what you say. So,
this means that patchew will complain each time you post if we can't find
a proper way to address that... :-\

> What I changed in the meantime though is that you should get all my patches 
> directly to your personal address, not only from the list. Or did you receive 
> v6 again just from the list?
> 

I've received the patches in my mailbox but I prefer to use the patchwork's
pwclient CLI to apply patches... and patchwork captures the patches from
the list, so I end up having to patch the authorship manually anyway.

How are you sending patches ? With git send-email ? If so, maybe you can pass
something like --from='"Christian Schoenebeck" <qemu_oss@crudebyte.com>'.
Since this is a different string, git will assume you're sending someone else's
patch : it will automatically add an extra From: made out of the commit Author
as recorded in the git tree.

> > Other warnings/errors should also be fixed but they look trivial.
> 
> Yeah, they are trivial. *But* there is one thing ...
> 
> > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > > 
> > > ERROR: space prohibited after that open parenthesis '('
> > > #92: FILE: hw/9pfs/9p.c:586:
> > > +    return ((uint64_t)mirror8bit( value        & 0xff) << 56) |
> > > 
> > > ERROR: space prohibited before that close parenthesis ')'
> > > #98: FILE: hw/9pfs/9p.c:592:
> > > +           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> > > 
> > > ERROR: space prohibited before that close parenthesis ')'
> > > #99: FILE: hw/9pfs/9p.c:593:
> > > +           ((uint64_t)mirror8bit((value >> 56) & 0xff)      ) ;
> 
> ... I would like to ignore this specific bot whining, because that particular 
> function looks much more readable the way it is (in that patch) right now.

Prettier certainly but...

/* Same as mirror8bit() just for a 64 bit data type instead for a byte. */
static inline uint64_t mirror64bit(uint64_t value)
{
    return ((uint64_t)mirror8bit(value         & 0xff) << 56) |
           ((uint64_t)mirror8bit((value >> 8)  & 0xff) << 48) |
           ((uint64_t)mirror8bit((value >> 16) & 0xff) << 40) |
           ((uint64_t)mirror8bit((value >> 24) & 0xff) << 32) |
           ((uint64_t)mirror8bit((value >> 32) & 0xff) << 24) |
           ((uint64_t)mirror8bit((value >> 40) & 0xff) << 16) |
           ((uint64_t)mirror8bit((value >> 48) & 0xff) <<  8) |
           ((uint64_t)mirror8bit((value >> 56) & 0xff));
}

... is readable enough IMHO and makes checkpatch happy :)

> 
> > > WARNING: Block comments use a leading /* on a separate line
> > > #102: FILE: hw/9pfs/9p.c:596:
> > > +/** @brief Parameter k for the Exponential Golomb algorihm to be used.
> > > 
> > > WARNING: Block comments use a leading /* on a separate line
> > > #121: FILE: hw/9pfs/9p.c:615:
> > > +/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
> > > 
> > > WARNING: Block comments use a leading /* on a separate line
> > > #148: FILE: hw/9pfs/9p.c:642:
> > > +/** @brief Converts a suffix into a prefix, or a prefix into a suffix.

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via Qemu-devel]

On Montag, 2. September 2019 17:34:32 CEST Greg Kurz wrote:
> On Sun, 01 Sep 2019 21:28:45 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> > > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
> > > 
> > > no-reply@patchew.org wrote:
> > > > Patchew URL:
> > > > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> > > > 
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > This series seems to have some coding style problems. See output below
> > > > for
> > 
> > > > more information:
> > [snip]
> > 
> > > > === OUTPUT BEGIN ===
> > > > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one
> > > > export
> > > > as an error) ERROR: Author email address is mangled by the mailing
> > > > list
> > > > #2:
> > > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > > 
> > > This is problematic since it ends up in the Author: field in git. Please
> > > find a way to fix that.
> > 
> > Like in which way do you imagine that? And where is the actual practical
> > problem? I mean every patch still has my signed-off-by tag with the
> > correct
> > email address ending up in git history.
> 
> Yes, this only breaks Author: if the patch is applied from the list.
> 
> > The cause for this issue is that the domain is configured to require DKIM
> > signatures for all outgoing emails. That's why mailman replaces my address
> > by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>"
> > placeholder since it could not provide a valid signature.
> > 
> > There were good reasons for enabling DKIM and it is a good thing for all
> > domains in general, since that ensures that (i.e. foreign) email addresses
> > cannot be used as sender address if the actual sender is not authorized
> > for
> > sending emails with that address.
> 
> Don't know much about DKIM but google seems to confirm what you say.

When you view the source of my emails you'll always see a "DKIM-Signature:" 
field in the email header, which is a signature of the email's body and the 
specific email header fields listed in the "DKIM-Signature:" block, so the 
original server can choose by itself which header fields to include ("h=...") 
for signing, but the standard requires the From: field must always be 
included.

A receiving server then obtains the public key from the domain's DNS records 
and checks if the DKIM signature of the email was correct:
https://en.wikipedia.org/wiki/DomainKeys_Identified_Mail

Additionally the receiving server obtains the so called "DMARC" entry from the 
domain's DNS records. The "DMARC" DNS entry contains the domain's general 
policies how receiving email servers shall handle verification of emails of 
this domain. For instance the DMARC entry may list SMTP servers (e.g. IPs, DNS 
names) eligble to send emails on behalf of the domain at all, and it defines 
what receiving email servers shall do with emails which were identified of not 
being from an eligible source (e.g. sender IP not listed in DMARC entry or 
missing or wrong DKIM signature in email header). For instance if the policy 
is "quarantine" in the DMARC entry then receiving servers are advised to 
simply drop invalid emails:  https://en.wikipedia.org/wiki/DMARC

> So, this means that patchew will complain each time you post if we can't
> find a proper way to address that... :-\

Well, mailman is handling this correctly. It replaces the "From:" field with a 
placeholder and instead adds my actual email address as "Reply-To:" field. 
That's the common way to handle this on mailing lists, as also mentioned here:
https://en.wikipedia.org/wiki/DMARC#From:_rewriting

So IMO patchew should automatically use the value of "Reply-To:" in that case 
as author of patches instead.

Reducing security cannot be the solution.

> > What I changed in the meantime though is that you should get all my
> > patches
> > directly to your personal address, not only from the list. Or did you
> > receive v6 again just from the list?
> 
> I've received the patches in my mailbox but I prefer to use the patchwork's
> pwclient CLI to apply patches... and patchwork captures the patches from
> the list, so I end up having to patch the authorship manually anyway.
> 
> How are you sending patches ? With git send-email ? If so, maybe you can
> pass something like --from='"Christian Schoenebeck"
> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
> assume you're sending someone else's patch : it will automatically add an
> extra From: made out of the commit Author as recorded in the git tree.

I use "git format-patch ..." to dump the invidiual emails as raw email sources 
and then I'll send those raw emails from the command line. So I have even more 
control of what is exactly sent out and could of course also add custom email 
header fields if required, if that would solve the situation somehow, i.e. 
manually as first test and later in automated way. That's not the issue here.

The problem however is that there is probably not any header field I could add 
that would solve the problem. Because I guess patchew is really just taking 
the first "From:" field as author, period.

I think the source files are available, so I could check that.

> > > Other warnings/errors should also be fixed but they look trivial.
> > 
> > Yeah, they are trivial. *But* there is one thing ...
> > 
> > > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > > > 
> > > > ERROR: space prohibited after that open parenthesis '('
> > > > #92: FILE: hw/9pfs/9p.c:586:
> > > > +    return ((uint64_t)mirror8bit( value        & 0xff) << 56) |
> > > > 
> > > > ERROR: space prohibited before that close parenthesis ')'
> > > > #98: FILE: hw/9pfs/9p.c:592:
> > > > +           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> > > > 
> > > > ERROR: space prohibited before that close parenthesis ')'
> > > > #99: FILE: hw/9pfs/9p.c:593:
> > > > +           ((uint64_t)mirror8bit((value >> 56) & 0xff)      ) ;
> > 
> > ... I would like to ignore this specific bot whining, because that
> > particular function looks much more readable the way it is (in that
> > patch) right now.
> Prettier certainly but...
> 
> /* Same as mirror8bit() just for a 64 bit data type instead for a byte. */
> static inline uint64_t mirror64bit(uint64_t value)
> {
>     return ((uint64_t)mirror8bit(value         & 0xff) << 56) |
>            ((uint64_t)mirror8bit((value >> 8)  & 0xff) << 48) |
>            ((uint64_t)mirror8bit((value >> 16) & 0xff) << 40) |
>            ((uint64_t)mirror8bit((value >> 24) & 0xff) << 32) |
>            ((uint64_t)mirror8bit((value >> 32) & 0xff) << 24) |
>            ((uint64_t)mirror8bit((value >> 40) & 0xff) << 16) |
>            ((uint64_t)mirror8bit((value >> 48) & 0xff) <<  8) |
>            ((uint64_t)mirror8bit((value >> 56) & 0xff));
> }
> 
> ... is readable enough IMHO and makes checkpatch happy :)

Well, okay :)

[Qemu-devel] DMARC/DKIM and qemu-devel list settings

[Ian Kelling]

At FSF, we've been working on this issue recently. I was planning to
send a general message to qemu-devel, but someone brought it up in a
thread below, so I'm doing it now.

Currently, a message sent to qemu-devel from a domain that publishes a
strict DMARC policy gets what mailman calls "Munge From". For example,
for a message sent to the list:

From: Anne Example Person <exampleperson@examplepersonsdomain>

Is modified my Mailman and sent to subscribers as:

From: Anne Example Person via Qemu-devel <qemu-devel@nongnu.org>
Reply-To: Anne Example Person <exampleperson@examplepersonsdomain>

We've recently made possible an alternative solution that does not need
munging and I call the unmodified message fix. Currently, mailman adds
"[Qemu-devel] " to the subject of messages. Modifying the message breaks
DKIM message signature and thus DMARC. In short: turn that off, and we
can stop from munging. Many lists are already this way, including most
popular @gnu and @nongnu lists, and this week we are doing a mass
conversion of lists which never touched DMARC related list settings (not
qemu-devel). Instead of using the subject prefix to identify a list,
subscribers can use the List-Id, To, and Cc headers.  List information
can also be be put in the welcome email to subscribers and the list
information page by list administrators.

Without going into all of the details, here's a few points about why we
concluded the unmodified message fix is better for discussion
lists. Email clients don't all treat munged messages the same way as
unmunged, and humans read these headers so it can confuse people,
causing messages not to be sent to the expected recipients. GNU Mailman
has an option to do "Munge From" always, but does not recommend using
it[1]. While we're not bound by what others do, it's worth noting that
other very large free software communities like Debian GNU/Linux have
adopted the unmodified message fix[2]. The unmodified messages fix
avoids breaking DKIM cryptographic signatures, which show the message
was authorized by the signing domain, which seems generally better for
security. Additionally, patchew has problems, as seen in the below
thread, subject was "[PATCH v6 0/4] 9p: Fix file ID collisions".

There is a small additional wrinkle. Very rarely, someone will send a
message to the list with a bad DKIM signature and publish a strict DMARC
policy, and in that case, we are forced to munge. I've searched all
messages posted to nongnu and gnu lists and, its always by someone
sending via their own mail server, or small enough to consider it that,
so its reasonable to ask them fix their DKIM signatures or turn off
their strict DMARC. I plan to setup an autoresponder to do that
automatically. Another case is if someone sends an html only email,
qemu-devel is configured to convert it to plaintext. That modifies the
message, and if its from a strict DMARC domain, the from munging is
done. Again, you can tell them to stop sending html only email.

I don't know who has the Qemu-devel list admin password, but whoever has
it can adopt the unmodified message fix by changing
dmarc_moderation_action to Accept here:
https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and
remove subject_prefix here
https://lists.nongnu.org/mailman/admin/qemu-devel/general

If the list admins went missing, email mailman@gnu.org and we can sort
out new ones eventually.

A few additional notes for completeness. We announced some of this at
https://lists.gnu.org/archive/html/savannah-hackers-public/2019-06/msg00018.html,
which includes information about other kinds of lists. For the unusual
cases of munging I described, we do from munging through exim because
mailman is not smart enough to only munge in these edge cases, and I'll
document that soon here[1].

[1]: https://wiki.list.org/DEV/DMARC
[2]: https://lists.debian.org/debian-devel-announce/2015/08/msg00003.html


Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> writes:

> On Montag, 2. September 2019 17:34:32 CEST Greg Kurz wrote:
>> On Sun, 01 Sep 2019 21:28:45 +0200
>> 
>> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
>> > > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
>> > > 
>> > > no-reply@patchew.org wrote:
>> > > > Patchew URL:
>> > > > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
>> > > > 
>> > > > 
>> > > > 
>> > > > Hi,
>> > > > 
>> > > > This series seems to have some coding style problems. See output below
>> > > > for
>> > 
>> > > > more information:
>> > [snip]
>> > 
>> > > > === OUTPUT BEGIN ===
>> > > > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one
>> > > > export
>> > > > as an error) ERROR: Author email address is mangled by the mailing
>> > > > list
>> > > > #2:
>> > > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>> > > 
>> > > This is problematic since it ends up in the Author: field in git. Please
>> > > find a way to fix that.
>> > 
>> > Like in which way do you imagine that? And where is the actual practical
>> > problem? I mean every patch still has my signed-off-by tag with the
>> > correct
>> > email address ending up in git history.
>> 
>> Yes, this only breaks Author: if the patch is applied from the list.
>> 
>> > The cause for this issue is that the domain is configured to require DKIM
>> > signatures for all outgoing emails. That's why mailman replaces my address
>> > by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>"
>> > placeholder since it could not provide a valid signature.
>> > 
>> > There were good reasons for enabling DKIM and it is a good thing for all
>> > domains in general, since that ensures that (i.e. foreign) email addresses
>> > cannot be used as sender address if the actual sender is not authorized
>> > for
>> > sending emails with that address.
>> 
>> Don't know much about DKIM but google seems to confirm what you say.
>
> When you view the source of my emails you'll always see a "DKIM-Signature:" 
> field in the email header, which is a signature of the email's body and the 
> specific email header fields listed in the "DKIM-Signature:" block, so the 
> original server can choose by itself which header fields to include ("h=...") 
> for signing, but the standard requires the From: field must always be 
> included.
>
> A receiving server then obtains the public key from the domain's DNS records 
> and checks if the DKIM signature of the email was correct:
> https://en.wikipedia.org/wiki/DomainKeys_Identified_Mail
>
> Additionally the receiving server obtains the so called "DMARC" entry from the 
> domain's DNS records. The "DMARC" DNS entry contains the domain's general 
> policies how receiving email servers shall handle verification of emails of 
> this domain. For instance the DMARC entry may list SMTP servers (e.g. IPs, DNS 
> names) eligble to send emails on behalf of the domain at all, and it defines 
> what receiving email servers shall do with emails which were identified of not 
> being from an eligible source (e.g. sender IP not listed in DMARC entry or 
> missing or wrong DKIM signature in email header). For instance if the policy 
> is "quarantine" in the DMARC entry then receiving servers are advised to 
> simply drop invalid emails:  https://en.wikipedia.org/wiki/DMARC
>
>> So, this means that patchew will complain each time you post if we can't
>> find a proper way to address that... :-\
>
> Well, mailman is handling this correctly. It replaces the "From:" field with a 
> placeholder and instead adds my actual email address as "Reply-To:" field. 
> That's the common way to handle this on mailing lists, as also mentioned here:
> https://en.wikipedia.org/wiki/DMARC#From:_rewriting
>
> So IMO patchew should automatically use the value of "Reply-To:" in that case 
> as author of patches instead.
>
> Reducing security cannot be the solution.
>
>> > What I changed in the meantime though is that you should get all my
>> > patches
>> > directly to your personal address, not only from the list. Or did you
>> > receive v6 again just from the list?
>> 
>> I've received the patches in my mailbox but I prefer to use the patchwork's
>> pwclient CLI to apply patches... and patchwork captures the patches from
>> the list, so I end up having to patch the authorship manually anyway.
>> 
>> How are you sending patches ? With git send-email ? If so, maybe you can
>> pass something like --from='"Christian Schoenebeck"
>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
>> assume you're sending someone else's patch : it will automatically add an
>> extra From: made out of the commit Author as recorded in the git tree.
>
> I use "git format-patch ..." to dump the invidiual emails as raw email sources 
> and then I'll send those raw emails from the command line. So I have even more 
> control of what is exactly sent out and could of course also add custom email 
> header fields if required, if that would solve the situation somehow, i.e. 
> manually as first test and later in automated way. That's not the issue here.
>
> The problem however is that there is probably not any header field I could add 
> that would solve the problem. Because I guess patchew is really just taking 
> the first "From:" field as author, period.
>
> I think the source files are available, so I could check that.
>

Re: [Qemu-devel] DMARC/DKIM and qemu-devel list settings

[Daniel P. Berrangé]

On Tue, Sep 03, 2019 at 03:11:08PM -0400, Ian Kelling wrote:
> At FSF, we've been working on this issue recently. I was planning to
> send a general message to qemu-devel, but someone brought it up in a
> thread below, so I'm doing it now.
> 
> Currently, a message sent to qemu-devel from a domain that publishes a
> strict DMARC policy gets what mailman calls "Munge From". For example,
> for a message sent to the list:
> 
> From: Anne Example Person <exampleperson@examplepersonsdomain>
> 
> Is modified my Mailman and sent to subscribers as:
> 
> From: Anne Example Person via Qemu-devel <qemu-devel@nongnu.org>
> Reply-To: Anne Example Person <exampleperson@examplepersonsdomain>
> 
> We've recently made possible an alternative solution that does not need
> munging and I call the unmodified message fix. Currently, mailman adds
> "[Qemu-devel] " to the subject of messages. Modifying the message breaks
> DKIM message signature and thus DMARC. In short: turn that off, and we
> can stop from munging. Many lists are already this way, including most
> popular @gnu and @nongnu lists, and this week we are doing a mass
> conversion of lists which never touched DMARC related list settings (not
> qemu-devel). Instead of using the subject prefix to identify a list,
> subscribers can use the List-Id, To, and Cc headers.  List information
> can also be be put in the welcome email to subscribers and the list
> information page by list administrators.
> 
> Without going into all of the details, here's a few points about why we
> concluded the unmodified message fix is better for discussion
> lists. Email clients don't all treat munged messages the same way as
> unmunged, and humans read these headers so it can confuse people,
> causing messages not to be sent to the expected recipients. GNU Mailman
> has an option to do "Munge From" always, but does not recommend using
> it[1]. While we're not bound by what others do, it's worth noting that
> other very large free software communities like Debian GNU/Linux have
> adopted the unmodified message fix[2]. The unmodified messages fix
> avoids breaking DKIM cryptographic signatures, which show the message
> was authorized by the signing domain, which seems generally better for
> security. Additionally, patchew has problems, as seen in the below
> thread, subject was "[PATCH v6 0/4] 9p: Fix file ID collisions".
> 
> There is a small additional wrinkle. Very rarely, someone will send a
> message to the list with a bad DKIM signature and publish a strict DMARC
> policy, and in that case, we are forced to munge. I've searched all
> messages posted to nongnu and gnu lists and, its always by someone
> sending via their own mail server, or small enough to consider it that,
> so its reasonable to ask them fix their DKIM signatures or turn off
> their strict DMARC. I plan to setup an autoresponder to do that
> automatically. Another case is if someone sends an html only email,
> qemu-devel is configured to convert it to plaintext. That modifies the
> message, and if its from a strict DMARC domain, the from munging is
> done. Again, you can tell them to stop sending html only email.

I think we should change mailman settings to *NOT* convert HTML to
plain text. It is pretty easy to setup mail clients to do this
conversion when viewing instead, which will avoid the DMARC problems.

eg with mutt you can add

  auto_view text/html
  alternative_order text/plain text/html

and in $HOME/.mailcap something like

  text/html; elinks -dump -localhost 1 -no-connect 1 -default-mime-type text/html %s; needsterminal; copiousoutput;



> 
> I don't know who has the Qemu-devel list admin password, but whoever has
> it can adopt the unmodified message fix by changing
> dmarc_moderation_action to Accept here:
> https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and
> remove subject_prefix here
> https://lists.nongnu.org/mailman/admin/qemu-devel/general
> 
> If the list admins went missing, email mailman@gnu.org and we can sort
> out new ones eventually.
> 
> A few additional notes for completeness. We announced some of this at
> https://lists.gnu.org/archive/html/savannah-hackers-public/2019-06/msg00018.html,
> which includes information about other kinds of lists. For the unusual
> cases of munging I described, we do from munging through exim because
> mailman is not smart enough to only munge in these edge cases, and I'll
> document that soon here[1].
> 
> [1]: https://wiki.list.org/DEV/DMARC
> [2]: https://lists.debian.org/debian-devel-announce/2015/08/msg00003.html

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: [Qemu-devel] DMARC/DKIM and qemu-devel list settings

[Ian Kelling]

Daniel P. Berrangé <berrange@redhat.com> writes:
>
> I think we should change mailman settings to *NOT* convert HTML to
> plain text. It is pretty easy to setup mail clients to do this
> conversion when viewing instead, which will avoid the DMARC problems.
>
> eg with mutt you can add
>
>   auto_view text/html
>   alternative_order text/plain text/html
>
> and in $HOME/.mailcap something like
>
>   text/html; elinks -dump -localhost 1 -no-connect 1 -default-mime-type text/html %s; needsterminal; copiousoutput;
>

Both are reasonable, pick your poison. I use emacs gnus and mu4e, both
convert to plain text by default afaik. Mailman simply calls out to lynx
to do it.

To change this, a list admin will just toggle convert_html_to_plaintext
at https://lists.nongnu.org/mailman/admin/qemu-devel/contentfilter

-- 
Ian Kelling | Senior Systems Administrator, Free Software Foundation
GPG Key: B125 F60B 7B28 7FF6 A2B7  DF8F 170A F0E2 9542 95DF
https://fsf.org | https://gnu.org

Re: [Qemu-devel] DMARC/DKIM and qemu-devel list settings

[Peter Maydell]

On Tue, 3 Sep 2019 at 20:11, Ian Kelling <iank@fsf.org> wrote:
> I don't know who has the Qemu-devel list admin password, but whoever has
> it can adopt the unmodified message fix by changing
> dmarc_moderation_action to Accept here:
> https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and
> remove subject_prefix here
> https://lists.nongnu.org/mailman/admin/qemu-devel/general

I'm one of the list admins, at least for the main qemu-devel
list; some of the sublists have different admins (and
perhaps different settings -- there's no way to conveniently
say "manage all 5 of these lists with the same policies,
so it's easy for them to get out of sync, deliberately
or accidentally).

I have been considering whether we change how we're handling
the DMARC problem for the list. I picked munge-the-email
initially because I think we didn't really understand the
consequences in terms of patchmail, and also because there
was a group of subscribers who complained that they liked
the [qemu-devel] tag, used it for filtering email, etc.
I think overall my opinion has shifted to thinking that
the downsides of munge-the-email are too great and we should
indeed switch to not modifying the message at all.

thanks
-- PMM

Re: [Qemu-devel] DMARC/DKIM and qemu-devel list settings

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 3 Sep 2019 at 20:11, Ian Kelling <iank@fsf.org> wrote:
>> I don't know who has the Qemu-devel list admin password, but whoever has
>> it can adopt the unmodified message fix by changing
>> dmarc_moderation_action to Accept here:
>> https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and
>> remove subject_prefix here
>> https://lists.nongnu.org/mailman/admin/qemu-devel/general
>
> I'm one of the list admins, at least for the main qemu-devel
> list; some of the sublists have different admins (and
> perhaps different settings -- there's no way to conveniently
> say "manage all 5 of these lists with the same policies,
> so it's easy for them to get out of sync, deliberately
> or accidentally).
>
> I have been considering whether we change how we're handling
> the DMARC problem for the list. I picked munge-the-email
> initially because I think we didn't really understand the
> consequences in terms of patchmail, and also because there
> was a group of subscribers who complained that they liked
> the [qemu-devel] tag, used it for filtering email, etc.
> I think overall my opinion has shifted to thinking that
> the downsides of munge-the-email are too great and we should
> indeed switch to not modifying the message at all.

Yes, please.

Re: [Qemu-devel] DMARC/DKIM and qemu-devel list settings

[Stefan Hajnoczi]

On Wed, Sep 4, 2019 at 4:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 3 Sep 2019 at 20:11, Ian Kelling <iank@fsf.org> wrote:
> > I don't know who has the Qemu-devel list admin password, but whoever has
> > it can adopt the unmodified message fix by changing
> > dmarc_moderation_action to Accept here:
> > https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and
> > remove subject_prefix here
> > https://lists.nongnu.org/mailman/admin/qemu-devel/general
>
> I'm one of the list admins, at least for the main qemu-devel
> list; some of the sublists have different admins (and
> perhaps different settings -- there's no way to conveniently
> say "manage all 5 of these lists with the same policies,
> so it's easy for them to get out of sync, deliberately
> or accidentally).
>
> I have been considering whether we change how we're handling
> the DMARC problem for the list. I picked munge-the-email
> initially because I think we didn't really understand the
> consequences in terms of patchmail, and also because there
> was a group of subscribers who complained that they liked
> the [qemu-devel] tag, used it for filtering email, etc.
> I think overall my opinion has shifted to thinking that
> the downsides of munge-the-email are too great and we should
> indeed switch to not modifying the message at all.

Yes, I think so too.

Thanks for notifying us, Ian!

Stefan

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 3683 bytes --]

On 9/2/19 5:29 PM, Christian Schoenebeck via Qemu-devel wrote:

>>>>> === OUTPUT BEGIN ===
>>>>> 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one
>>>>> export
>>>>> as an error) ERROR: Author email address is mangled by the mailing
>>>>> list
>>>>> #2:
>>>>> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>>>>
>>>> This is problematic since it ends up in the Author: field in git. Please
>>>> find a way to fix that.
>>>
>>> Like in which way do you imagine that? And where is the actual practical
>>> problem? I mean every patch still has my signed-off-by tag with the
>>> correct
>>> email address ending up in git history.
>>
>> Yes, this only breaks Author: if the patch is applied from the list.

Except that many maintainers DO apply mail from the list (thanks to 'git
am').  Fixing patchew to unmunge things is an appealing idea, but would
not fix the problem for maintainers not cloning from patchew, so even if
patchew avoids the problem locally, it should still continue to warn
about the problem.

>>
>>> The cause for this issue is that the domain is configured to require DKIM
>>> signatures for all outgoing emails. That's why mailman replaces my address
>>> by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>"
>>> placeholder since it could not provide a valid signature.

And when you know that mailman is going to munge your address, the fix
is to configure git to output 'From: correct name <correct@example.com>'
as the first line of the BODY of the message, since 'git am' favors the
unmunged From: from the body over the munged From: from the headers.


> 
>> So, this means that patchew will complain each time you post if we can't
>> find a proper way to address that... :-\
> 
> Well, mailman is handling this correctly. It replaces the "From:" field with a 
> placeholder and instead adds my actual email address as "Reply-To:" field. 
> That's the common way to handle this on mailing lists, as also mentioned here:
> https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> 
> So IMO patchew should automatically use the value of "Reply-To:" in that case 
> as author of patches instead.
> 
> Reducing security cannot be the solution.

No, there's no need to reduce security.  Just change your local git
configuration to produce a 'From:' line in the commit body..

>> How are you sending patches ? With git send-email ? If so, maybe you can
>> pass something like --from='"Christian Schoenebeck"
>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
>> assume you're sending someone else's patch : it will automatically add an
>> extra From: made out of the commit Author as recorded in the git tree.

I think it is probably as simple as a 'git config' command to tell git
to always put a 'From:' in the body of self-authored patches when using
git format-patch; however, as I don't suffer from munged emails, I
haven't actually tested what that setting would be.

> 
> I use "git format-patch ..." to dump the invidiual emails as raw email sources 
> and then I'll send those raw emails from the command line. So I have even more 
> control of what is exactly sent out and could of course also add custom email 
> header fields if required, if that would solve the situation somehow, i.e. 
> manually as first test and later in automated way. That's not the issue here.

Working around the problem does not require munging email headers, but
adding a line to the email body.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via Qemu-devel]

On Dienstag, 3. September 2019 21:38:15 CEST Eric Blake wrote:
> On 9/2/19 5:29 PM, Christian Schoenebeck via Qemu-devel wrote:
> >>>>> === OUTPUT BEGIN ===
> >>>>> 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one
> >>>>> export
> >>>>> as an error) ERROR: Author email address is mangled by the mailing
> >>>>> list
> >>>>> #2:
> >>>>> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> >>>> 
> >>>> This is problematic since it ends up in the Author: field in git.
> >>>> Please
> >>>> find a way to fix that.
> >>> 
> >>> Like in which way do you imagine that? And where is the actual practical
> >>> problem? I mean every patch still has my signed-off-by tag with the
> >>> correct
> >>> email address ending up in git history.
> >> 
> >> Yes, this only breaks Author: if the patch is applied from the list.
> 
> Except that many maintainers DO apply mail from the list (thanks to 'git
> am').  Fixing patchew to unmunge things is an appealing idea, but would
> not fix the problem for maintainers not cloning from patchew, so even if
> patchew avoids the problem locally, it should still continue to warn
> about the problem.
> 
> >>> The cause for this issue is that the domain is configured to require
> >>> DKIM
> >>> signatures for all outgoing emails. That's why mailman replaces my
> >>> address
> >>> by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>"
> >>> placeholder since it could not provide a valid signature.
> 
> And when you know that mailman is going to munge your address, the fix
> is to configure git to output 'From: correct name <correct@example.com>'
> as the first line of the BODY of the message, since 'git am' favors the
> unmunged From: from the body over the munged From: from the headers.

Ah I see, I will try that with the next 9p patch set round (probably 
tomorrow). Thanks for the hint!

I actually had a quick glimpse on the patchew sources yesterday to see if 
there was some undocumented alternative header like "X-git-author:" or 
something like that, but could not find one.

> > Well, mailman is handling this correctly. It replaces the "From:" field
> > with a placeholder and instead adds my actual email address as
> > "Reply-To:" field. That's the common way to handle this on mailing lists,
> > as also mentioned here:
> > https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> > 
> > So IMO patchew should automatically use the value of "Reply-To:" in that
> > case as author of patches instead.
> > 
> > Reducing security cannot be the solution.
> 
> No, there's no need to reduce security.  Just change your local git
> configuration to produce a 'From:' line in the commit body..

Got it. :)

> >> How are you sending patches ? With git send-email ? If so, maybe you can
> >> pass something like --from='"Christian Schoenebeck"
> >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
> >> assume you're sending someone else's patch : it will automatically add an
> >> extra From: made out of the commit Author as recorded in the git tree.
> 
> I think it is probably as simple as a 'git config' command to tell git
> to always put a 'From:' in the body of self-authored patches when using
> git format-patch; however, as I don't suffer from munged emails, I
> haven't actually tested what that setting would be.
> 
> > I use "git format-patch ..." to dump the invidiual emails as raw email
> > sources and then I'll send those raw emails from the command line. So I
> > have even more control of what is exactly sent out and could of course
> > also add custom email header fields if required, if that would solve the
> > situation somehow, i.e. manually as first test and later in automated
> > way. That's not the issue here.
> Working around the problem does not require munging email headers, but
> adding a line to the email body.

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via Qemu-devel]

On Mittwoch, 4. September 2019 15:02:30 CEST Christian Schoenebeck wrote:
> > > Well, mailman is handling this correctly. It replaces the "From:" field
> > > with a placeholder and instead adds my actual email address as
> > > "Reply-To:" field. That's the common way to handle this on mailing
> > > lists,
> > > as also mentioned here:
> > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> > > 
> > > So IMO patchew should automatically use the value of "Reply-To:" in that
> > > case as author of patches instead.
> > > 
> > > Reducing security cannot be the solution.
> > 
> > No, there's no need to reduce security.  Just change your local git
> > configuration to produce a 'From:' line in the commit body..
> 
> Got it. :)
> 
> > >> How are you sending patches ? With git send-email ? If so, maybe you
> > >> can
> > >> pass something like --from='"Christian Schoenebeck"
> > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
> > >> assume you're sending someone else's patch : it will automatically add
> > >> an
> > >> extra From: made out of the commit Author as recorded in the git tree.
> > 
> > I think it is probably as simple as a 'git config' command to tell git
> > to always put a 'From:' in the body of self-authored patches when using
> > git format-patch; however, as I don't suffer from munged emails, I
> > haven't actually tested what that setting would be.

Well, I tried that Eric. The expected solution would be enabling this git 
setting:

git config [--global] format.from true
https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom

But as you can already read from the manual, the overall behaviour of git 
regarding a separate "From:" line in the email body was intended solely for 
the use case sender != author. So in practice (at least in my git version) git 
always makes a raw string comparison between sender (name and email) string 
and author string and only adds the separate From: line to the body if they 
differ.

Hence also "git format-patch --from=" only works here if you use a different 
author string (name and email) there, otherwise on a perfect string match it 
is simply ignored and you end up with only one "From:" in the email header.

So eventually I added one extra character in my name for now and removed it 
manually in the dumped emails subsequently (see today's
"[PATCH v7 0/3] 9p: Fix file ID collisions").

Besides that direct string comparison restriction; git also seems to have a 
bug here. Because even if you have sender != author, then git falsely uses 
author as sender of the cover letter, whereas the emails of the individual 
patches are encoded correctly.

Best regards,
Christian Schoenebeck

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Greg Kurz]

On Thu, 05 Sep 2019 14:25:13 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 4. September 2019 15:02:30 CEST Christian Schoenebeck wrote:
> > > > Well, mailman is handling this correctly. It replaces the "From:" field
> > > > with a placeholder and instead adds my actual email address as
> > > > "Reply-To:" field. That's the common way to handle this on mailing
> > > > lists,
> > > > as also mentioned here:
> > > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> > > > 
> > > > So IMO patchew should automatically use the value of "Reply-To:" in that
> > > > case as author of patches instead.
> > > > 
> > > > Reducing security cannot be the solution.
> > > 
> > > No, there's no need to reduce security.  Just change your local git
> > > configuration to produce a 'From:' line in the commit body..
> > 
> > Got it. :)
> > 
> > > >> How are you sending patches ? With git send-email ? If so, maybe you
> > > >> can
> > > >> pass something like --from='"Christian Schoenebeck"
> > > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
> > > >> assume you're sending someone else's patch : it will automatically add
> > > >> an
> > > >> extra From: made out of the commit Author as recorded in the git tree.
> > > 
> > > I think it is probably as simple as a 'git config' command to tell git
> > > to always put a 'From:' in the body of self-authored patches when using
> > > git format-patch; however, as I don't suffer from munged emails, I
> > > haven't actually tested what that setting would be.
> 
> Well, I tried that Eric. The expected solution would be enabling this git 
> setting:
> 
> git config [--global] format.from true
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> But as you can already read from the manual, the overall behaviour of git 
> regarding a separate "From:" line in the email body was intended solely for 
> the use case sender != author. So in practice (at least in my git version) git 
> always makes a raw string comparison between sender (name and email) string 
> and author string and only adds the separate From: line to the body if they 
> differ.
> 
> Hence also "git format-patch --from=" only works here if you use a different 
> author string (name and email) there, otherwise on a perfect string match it 
> is simply ignored and you end up with only one "From:" in the email header.
> 
> So eventually I added one extra character in my name for now and removed it 
> manually in the dumped emails subsequently (see today's
> "[PATCH v7 0/3] 9p: Fix file ID collisions").
> 

Hence my proposal in some other mail to pass a different string to
git send-email, but I guess this also works for git format-patch.

eg, adding double quotes around your "firstname name"

 --from='"Christian Schoenebeck" <qemu_oss@crudebyte.com>'

> Besides that direct string comparison restriction; git also seems to have a 
> bug here. Because even if you have sender != author, then git falsely uses 
> author as sender of the cover letter, whereas the emails of the individual 
> patches are encoded correctly.
> 
> Best regards,
> Christian Schoenebeck
> 
> 

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via]

On Donnerstag, 5. September 2019 14:59:31 CEST Greg Kurz wrote:
> On Thu, 05 Sep 2019 14:25:13 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Mittwoch, 4. September 2019 15:02:30 CEST Christian Schoenebeck wrote:
> > > > > Well, mailman is handling this correctly. It replaces the "From:"
> > > > > field
> > > > > with a placeholder and instead adds my actual email address as
> > > > > "Reply-To:" field. That's the common way to handle this on mailing
> > > > > lists,
> > > > > as also mentioned here:
> > > > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> > > > > 
> > > > > So IMO patchew should automatically use the value of "Reply-To:" in
> > > > > that
> > > > > case as author of patches instead.
> > > > > 
> > > > > Reducing security cannot be the solution.
> > > > 
> > > > No, there's no need to reduce security.  Just change your local git
> > > > configuration to produce a 'From:' line in the commit body..
> > > 
> > > Got it. :)
> > > 
> > > > >> How are you sending patches ? With git send-email ? If so, maybe
> > > > >> you
> > > > >> can
> > > > >> pass something like --from='"Christian Schoenebeck"
> > > > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git
> > > > >> will
> > > > >> assume you're sending someone else's patch : it will automatically
> > > > >> add
> > > > >> an
> > > > >> extra From: made out of the commit Author as recorded in the git
> > > > >> tree.
> > > > 
> > > > I think it is probably as simple as a 'git config' command to tell git
> > > > to always put a 'From:' in the body of self-authored patches when
> > > > using
> > > > git format-patch; however, as I don't suffer from munged emails, I
> > > > haven't actually tested what that setting would be.
> > 
> > Well, I tried that Eric. The expected solution would be enabling this git
> > setting:
> > 
> > git config [--global] format.from true
> > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfro
> > m
> > 
> > But as you can already read from the manual, the overall behaviour of git
> > regarding a separate "From:" line in the email body was intended solely
> > for
> > the use case sender != author. So in practice (at least in my git version)
> > git always makes a raw string comparison between sender (name and email)
> > string and author string and only adds the separate From: line to the
> > body if they differ.
> > 
> > Hence also "git format-patch --from=" only works here if you use a
> > different author string (name and email) there, otherwise on a perfect
> > string match it is simply ignored and you end up with only one "From:" in
> > the email header.
> > 
> > So eventually I added one extra character in my name for now and removed
> > it
> > manually in the dumped emails subsequently (see today's
> > "[PATCH v7 0/3] 9p: Fix file ID collisions").
> 
> Hence my proposal in some other mail to pass a different string to
> git send-email, but I guess this also works for git format-patch.
> 
> eg, adding double quotes around your "firstname name"
> 
>  --from='"Christian Schoenebeck" <qemu_oss@crudebyte.com>'

Yeah, I will use that for now, since it just works^TM (I tested it).

Because for some reason my emails are still mangled on this list. Probably I 
still have to drop more header fields from dkim's "h=..." setting. We'll see.

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2641 bytes --]

[adding git list]

On 9/5/19 7:25 AM, Christian Schoenebeck wrote:

>>>>> How are you sending patches ? With git send-email ? If so, maybe you
>>>>> can
>>>>> pass something like --from='"Christian Schoenebeck"
>>>>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
>>>>> assume you're sending someone else's patch : it will automatically add
>>>>> an
>>>>> extra From: made out of the commit Author as recorded in the git tree.
>>>
>>> I think it is probably as simple as a 'git config' command to tell git
>>> to always put a 'From:' in the body of self-authored patches when using
>>> git format-patch; however, as I don't suffer from munged emails, I
>>> haven't actually tested what that setting would be.
> 
> Well, I tried that Eric. The expected solution would be enabling this git 
> setting:
> 
> git config [--global] format.from true
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> But as you can already read from the manual, the overall behaviour of git 
> regarding a separate "From:" line in the email body was intended solely for 
> the use case sender != author. So in practice (at least in my git version) git 
> always makes a raw string comparison between sender (name and email) string 
> and author string and only adds the separate From: line to the body if they 
> differ.
> 
> Hence also "git format-patch --from=" only works here if you use a different 
> author string (name and email) there, otherwise on a perfect string match it 
> is simply ignored and you end up with only one "From:" in the email header.

git folks:

How hard would it be to improve 'git format-patch'/'git send-email' to
have an option to ALWAYS output a From: line in the body, even when the
sender is the author, for the case of a mailing list that munges the
mail headers due to DMARC/DKIM reasons?

> 
> So eventually I added one extra character in my name for now and removed it 
> manually in the dumped emails subsequently (see today's
> "[PATCH v7 0/3] 9p: Fix file ID collisions").
> 
> Besides that direct string comparison restriction; git also seems to have a 
> bug here. Because even if you have sender != author, then git falsely uses 
> author as sender of the cover letter, whereas the emails of the individual 
> patches are encoded correctly.

At any rate, I'm glad that you have figured out a workaround, even if
painful, while we wait for git to provide what we really need.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2641 bytes --]

[adding git list]

On 9/5/19 7:25 AM, Christian Schoenebeck wrote:

>>>>> How are you sending patches ? With git send-email ? If so, maybe you
>>>>> can
>>>>> pass something like --from='"Christian Schoenebeck"
>>>>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
>>>>> assume you're sending someone else's patch : it will automatically add
>>>>> an
>>>>> extra From: made out of the commit Author as recorded in the git tree.
>>>
>>> I think it is probably as simple as a 'git config' command to tell git
>>> to always put a 'From:' in the body of self-authored patches when using
>>> git format-patch; however, as I don't suffer from munged emails, I
>>> haven't actually tested what that setting would be.
> 
> Well, I tried that Eric. The expected solution would be enabling this git 
> setting:
> 
> git config [--global] format.from true
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> But as you can already read from the manual, the overall behaviour of git 
> regarding a separate "From:" line in the email body was intended solely for 
> the use case sender != author. So in practice (at least in my git version) git 
> always makes a raw string comparison between sender (name and email) string 
> and author string and only adds the separate From: line to the body if they 
> differ.
> 
> Hence also "git format-patch --from=" only works here if you use a different 
> author string (name and email) there, otherwise on a perfect string match it 
> is simply ignored and you end up with only one "From:" in the email header.

git folks:

How hard would it be to improve 'git format-patch'/'git send-email' to
have an option to ALWAYS output a From: line in the body, even when the
sender is the author, for the case of a mailing list that munges the
mail headers due to DMARC/DKIM reasons?

> 
> So eventually I added one extra character in my name for now and removed it 
> manually in the dumped emails subsequently (see today's
> "[PATCH v7 0/3] 9p: Fix file ID collisions").
> 
> Besides that direct string comparison restriction; git also seems to have a 
> bug here. Because even if you have sender != author, then git falsely uses 
> author as sender of the cover letter, whereas the emails of the individual 
> patches are encoded correctly.

At any rate, I'm glad that you have figured out a workaround, even if
painful, while we wait for git to provide what we really need.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Jeff King]

On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:

> > But as you can already read from the manual, the overall behaviour of git 
> > regarding a separate "From:" line in the email body was intended solely for 
> > the use case sender != author. So in practice (at least in my git version) git 
> > always makes a raw string comparison between sender (name and email) string 
> > and author string and only adds the separate From: line to the body if they 
> > differ.
> > 
> > Hence also "git format-patch --from=" only works here if you use a different 
> > author string (name and email) there, otherwise on a perfect string match it 
> > is simply ignored and you end up with only one "From:" in the email header.
> 
> git folks:
> 
> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?

It wouldn't be very hard to ask format-patch to just handle this
unconditionally. Something like:

diff --git a/pretty.c b/pretty.c
index e4ed14effe..9cf79d7874 100644
--- a/pretty.c
+++ b/pretty.c
@@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
 		map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
 
 	if (cmit_fmt_is_mail(pp->fmt)) {
-		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
+		if (pp->always_use_in_body_from ||
+		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
 			struct strbuf buf = STRBUF_INIT;
 
 			strbuf_addstr(&buf, "From: ");

but most of the work would be ferrying that option from the command line
down to the pretty-print code.

That would work in conjunction with "--from" to avoid a duplicate. It
might require send-email learning about the option to avoid doing its
own in-body-from management. If you only care about send-email, it might
be easier to just add the option there.

-Peff

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Jeff King]

On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:

> > But as you can already read from the manual, the overall behaviour of git 
> > regarding a separate "From:" line in the email body was intended solely for 
> > the use case sender != author. So in practice (at least in my git version) git 
> > always makes a raw string comparison between sender (name and email) string 
> > and author string and only adds the separate From: line to the body if they 
> > differ.
> > 
> > Hence also "git format-patch --from=" only works here if you use a different 
> > author string (name and email) there, otherwise on a perfect string match it 
> > is simply ignored and you end up with only one "From:" in the email header.
> 
> git folks:
> 
> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?

It wouldn't be very hard to ask format-patch to just handle this
unconditionally. Something like:

diff --git a/pretty.c b/pretty.c
index e4ed14effe..9cf79d7874 100644
--- a/pretty.c
+++ b/pretty.c
@@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
 		map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
 
 	if (cmit_fmt_is_mail(pp->fmt)) {
-		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
+		if (pp->always_use_in_body_from ||
+		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
 			struct strbuf buf = STRBUF_INIT;
 
 			strbuf_addstr(&buf, "From: ");

but most of the work would be ferrying that option from the command line
down to the pretty-print code.

That would work in conjunction with "--from" to avoid a duplicate. It
might require send-email learning about the option to avoid doing its
own in-body-from management. If you only care about send-email, it might
be easier to just add the option there.

-Peff

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck]

On Montag, 9. September 2019 16:25:12 CEST Jeff King wrote:
> On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:
> > > But as you can already read from the manual, the overall behaviour of
> > > git
> > > regarding a separate "From:" line in the email body was intended solely
> > > for
> > > the use case sender != author. So in practice (at least in my git
> > > version) git always makes a raw string comparison between sender (name
> > > and email) string and author string and only adds the separate From:
> > > line to the body if they differ.
> > > 
> > > Hence also "git format-patch --from=" only works here if you use a
> > > different author string (name and email) there, otherwise on a perfect
> > > string match it is simply ignored and you end up with only one "From:"
> > > in the email header.> 
> > git folks:
> > 
> > How hard would it be to improve 'git format-patch'/'git send-email' to
> > have an option to ALWAYS output a From: line in the body, even when the
> > sender is the author, for the case of a mailing list that munges the
> > mail headers due to DMARC/DKIM reasons?
> 
> It wouldn't be very hard to ask format-patch to just handle this
> unconditionally. Something like:
> 
> diff --git a/pretty.c b/pretty.c
> index e4ed14effe..9cf79d7874 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
>  		map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
> 
>  	if (cmit_fmt_is_mail(pp->fmt)) {
> -		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> +		if (pp->always_use_in_body_from ||
> +		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
>  			struct strbuf buf = STRBUF_INIT;
> 
>  			strbuf_addstr(&buf, "From: ");
> 
> but most of the work would be ferrying that option from the command line
> down to the pretty-print code.
> 
> That would work in conjunction with "--from" to avoid a duplicate. It
> might require send-email learning about the option to avoid doing its
> own in-body-from management. If you only care about send-email, it might
> be easier to just add the option there.

Would it simplify the changes in git if that would be made a
"git config [--global]" setting only? That is, would that probably simplify 
that task to one simple function call there in pretty.c?

On the other hand, considering the already existing --from argument and 
"format.from" config option:
https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom

Wouldn't it make sense to just drop the currently existing sender != author 
string comparison in git and simply always add the "From:" line to the email's 
body if "format.from yes" is used, instead of introducing a suggested 2nd 
(e.g. "always-from") option? I mean sure automatically removing redundant 
information in the generated emails if sender == author sounds nice on first 
thought, but does it address anything useful in practice to justify 
introduction of a 2nd related option?

Best regards,
Christian Schoenebeck

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck via]

On Montag, 9. September 2019 16:25:12 CEST Jeff King wrote:
> On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:
> > > But as you can already read from the manual, the overall behaviour of
> > > git
> > > regarding a separate "From:" line in the email body was intended solely
> > > for
> > > the use case sender != author. So in practice (at least in my git
> > > version) git always makes a raw string comparison between sender (name
> > > and email) string and author string and only adds the separate From:
> > > line to the body if they differ.
> > > 
> > > Hence also "git format-patch --from=" only works here if you use a
> > > different author string (name and email) there, otherwise on a perfect
> > > string match it is simply ignored and you end up with only one "From:"
> > > in the email header.> 
> > git folks:
> > 
> > How hard would it be to improve 'git format-patch'/'git send-email' to
> > have an option to ALWAYS output a From: line in the body, even when the
> > sender is the author, for the case of a mailing list that munges the
> > mail headers due to DMARC/DKIM reasons?
> 
> It wouldn't be very hard to ask format-patch to just handle this
> unconditionally. Something like:
> 
> diff --git a/pretty.c b/pretty.c
> index e4ed14effe..9cf79d7874 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
>  		map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
> 
>  	if (cmit_fmt_is_mail(pp->fmt)) {
> -		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> +		if (pp->always_use_in_body_from ||
> +		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
>  			struct strbuf buf = STRBUF_INIT;
> 
>  			strbuf_addstr(&buf, "From: ");
> 
> but most of the work would be ferrying that option from the command line
> down to the pretty-print code.
> 
> That would work in conjunction with "--from" to avoid a duplicate. It
> might require send-email learning about the option to avoid doing its
> own in-body-from management. If you only care about send-email, it might
> be easier to just add the option there.

Would it simplify the changes in git if that would be made a
"git config [--global]" setting only? That is, would that probably simplify 
that task to one simple function call there in pretty.c?

On the other hand, considering the already existing --from argument and 
"format.from" config option:
https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom

Wouldn't it make sense to just drop the currently existing sender != author 
string comparison in git and simply always add the "From:" line to the email's 
body if "format.from yes" is used, instead of introducing a suggested 2nd 
(e.g. "always-from") option? I mean sure automatically removing redundant 
information in the generated emails if sender == author sounds nice on first 
thought, but does it address anything useful in practice to justify 
introduction of a 2nd related option?

Best regards,
Christian Schoenebeck

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Jeff King]

On Mon, Sep 23, 2019 at 01:19:18PM +0200, Christian Schoenebeck wrote:

> >  	if (cmit_fmt_is_mail(pp->fmt)) {
> > -		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> > +		if (pp->always_use_in_body_from ||
> > +		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
> >  			struct strbuf buf = STRBUF_INIT;
> > 
> >  			strbuf_addstr(&buf, "From: ");
> > 
> > but most of the work would be ferrying that option from the command line
> > down to the pretty-print code.
> > 
> > That would work in conjunction with "--from" to avoid a duplicate. It
> > might require send-email learning about the option to avoid doing its
> > own in-body-from management. If you only care about send-email, it might
> > be easier to just add the option there.
> 
> Would it simplify the changes in git if that would be made a
> "git config [--global]" setting only? That is, would that probably simplify 
> that task to one simple function call there in pretty.c?

I think a config option would make sense, but we generally try to avoid
adding a config option that doesn't have a matching command-line option.

I also think saving implementation work there is orthogonal. You can as
easily make a global "always_use_in_body_from" as you can call a global
config_get_bool("format-patch.always_use_in_body_from"). :)

And anyway, it's not _that_ much work to pass it around. At least as
much would go into writing documentation and tests. One of the reasons I
left the patch above as a sketch is that I'm not 100% convinced this is
a useful feature. Somebody caring enough about it to make a real patch
would send a signal there.

> On the other hand, considering the already existing --from argument and 
> "format.from" config option:
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> Wouldn't it make sense to just drop the currently existing sender != author 
> string comparison in git and simply always add the "From:" line to the email's 
> body if "format.from yes" is used, instead of introducing a suggested 2nd 
> (e.g. "always-from") option? I mean sure automatically removing redundant 
> information in the generated emails if sender == author sounds nice on first 
> thought, but does it address anything useful in practice to justify 
> introduction of a 2nd related option?

Yes, the resulting mail would be correct, in the sense that it could be
applied just fine by git-am. But I think it would be uglier. IOW, I
consider the presence of the in-body From to be a clue that something
interesting is going on (like forwarding somebody else's patch). So from
my perspective, it would just be useless noise. Other communities may
have different opinions, though (I think I have seen some kernel folks
always including all of the possible in-body headers, including Date).
But it seems like it makes sense to keep both possibilities.

-Peff

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Jeff King]

On Mon, Sep 23, 2019 at 01:19:18PM +0200, Christian Schoenebeck wrote:

> >  	if (cmit_fmt_is_mail(pp->fmt)) {
> > -		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> > +		if (pp->always_use_in_body_from ||
> > +		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
> >  			struct strbuf buf = STRBUF_INIT;
> > 
> >  			strbuf_addstr(&buf, "From: ");
> > 
> > but most of the work would be ferrying that option from the command line
> > down to the pretty-print code.
> > 
> > That would work in conjunction with "--from" to avoid a duplicate. It
> > might require send-email learning about the option to avoid doing its
> > own in-body-from management. If you only care about send-email, it might
> > be easier to just add the option there.
> 
> Would it simplify the changes in git if that would be made a
> "git config [--global]" setting only? That is, would that probably simplify 
> that task to one simple function call there in pretty.c?

I think a config option would make sense, but we generally try to avoid
adding a config option that doesn't have a matching command-line option.

I also think saving implementation work there is orthogonal. You can as
easily make a global "always_use_in_body_from" as you can call a global
config_get_bool("format-patch.always_use_in_body_from"). :)

And anyway, it's not _that_ much work to pass it around. At least as
much would go into writing documentation and tests. One of the reasons I
left the patch above as a sketch is that I'm not 100% convinced this is
a useful feature. Somebody caring enough about it to make a real patch
would send a signal there.

> On the other hand, considering the already existing --from argument and 
> "format.from" config option:
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> Wouldn't it make sense to just drop the currently existing sender != author 
> string comparison in git and simply always add the "From:" line to the email's 
> body if "format.from yes" is used, instead of introducing a suggested 2nd 
> (e.g. "always-from") option? I mean sure automatically removing redundant 
> information in the generated emails if sender == author sounds nice on first 
> thought, but does it address anything useful in practice to justify 
> introduction of a 2nd related option?

Yes, the resulting mail would be correct, in the sense that it could be
applied just fine by git-am. But I think it would be uglier. IOW, I
consider the presence of the in-body From to be a clue that something
interesting is going on (like forwarding somebody else's patch). So from
my perspective, it would just be useless noise. Other communities may
have different opinions, though (I think I have seen some kernel folks
always including all of the possible in-body headers, including Date).
But it seems like it makes sense to keep both possibilities.

-Peff

git format.from (was: 9p: Fix file ID collisions)

[Christian Schoenebeck]

On Dienstag, 24. September 2019 00:24:15 CEST Jeff King wrote:
> > On the other hand, considering the already existing --from argument and
> > "format.from" config option:
> > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfro
> > m
> > 
> > Wouldn't it make sense to just drop the currently existing sender !=
> > author
> > string comparison in git and simply always add the "From:" line to the
> > email's body if "format.from yes" is used, instead of introducing a
> > suggested 2nd (e.g. "always-from") option? I mean sure automatically
> > removing redundant information in the generated emails if sender ==
> > author sounds nice on first thought, but does it address anything useful
> > in practice to justify introduction of a 2nd related option?
> 
> Yes, the resulting mail would be correct, in the sense that it could be
> applied just fine by git-am. But I think it would be uglier. IOW, I
> consider the presence of the in-body From to be a clue that something
> interesting is going on (like forwarding somebody else's patch). So from
> my perspective, it would just be useless noise. Other communities may
> have different opinions, though (I think I have seen some kernel folks
> always including all of the possible in-body headers, including Date).
> But it seems like it makes sense to keep both possibilities.

Exactly, current git behaviour is solely "prettier" (at first thought only 
though), but does not address anything useful in real life.

Current git behaviour does cause real life problems though: Many email lists 
are munging emails of patch senders whose domain is configured for requiring 
domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a 
DMARC). So original sender's From: header is then automatically replaced by an 
alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting

For instance the email header:

From: "Bob Bold" <bold@foo.com>

is automatically replaced by lists by something like

From: "Bob Bold via Somelist" <somelist@gnu.org>

And since git currently always drops the From: line from the email's body if
sender == author, as a consequence maintainers applying patches from such 
lists, always need to rewrite git history subsequently and have to replace 
patch author's identity manually for each commit to have their correct, real 
email address and real name in git history instead of something like
"Bob Bold via Somelist" <somelist@gnu.org>

So what do you find "uglier"? I prefer key info not being lost as default 
behaviour. :-)

Best regards,
Christian Schoenebeck

git format.from (was: 9p: Fix file ID collisions)

[Christian Schoenebeck via]

On Dienstag, 24. September 2019 00:24:15 CEST Jeff King wrote:
> > On the other hand, considering the already existing --from argument and
> > "format.from" config option:
> > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfro
> > m
> > 
> > Wouldn't it make sense to just drop the currently existing sender !=
> > author
> > string comparison in git and simply always add the "From:" line to the
> > email's body if "format.from yes" is used, instead of introducing a
> > suggested 2nd (e.g. "always-from") option? I mean sure automatically
> > removing redundant information in the generated emails if sender ==
> > author sounds nice on first thought, but does it address anything useful
> > in practice to justify introduction of a 2nd related option?
> 
> Yes, the resulting mail would be correct, in the sense that it could be
> applied just fine by git-am. But I think it would be uglier. IOW, I
> consider the presence of the in-body From to be a clue that something
> interesting is going on (like forwarding somebody else's patch). So from
> my perspective, it would just be useless noise. Other communities may
> have different opinions, though (I think I have seen some kernel folks
> always including all of the possible in-body headers, including Date).
> But it seems like it makes sense to keep both possibilities.

Exactly, current git behaviour is solely "prettier" (at first thought only 
though), but does not address anything useful in real life.

Current git behaviour does cause real life problems though: Many email lists 
are munging emails of patch senders whose domain is configured for requiring 
domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a 
DMARC). So original sender's From: header is then automatically replaced by an 
alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting

For instance the email header:

From: "Bob Bold" <bold@foo.com>

is automatically replaced by lists by something like

From: "Bob Bold via Somelist" <somelist@gnu.org>

And since git currently always drops the From: line from the email's body if
sender == author, as a consequence maintainers applying patches from such 
lists, always need to rewrite git history subsequently and have to replace 
patch author's identity manually for each commit to have their correct, real 
email address and real name in git history instead of something like
"Bob Bold via Somelist" <somelist@gnu.org>

So what do you find "uglier"? I prefer key info not being lost as default 
behaviour. :-)

Best regards,
Christian Schoenebeck

Re: git format.from (was: 9p: Fix file ID collisions)

[Jeff King]

On Tue, Sep 24, 2019 at 11:03:38AM +0200, Christian Schoenebeck wrote:

> > Yes, the resulting mail would be correct, in the sense that it could be
> > applied just fine by git-am. But I think it would be uglier. IOW, I
> > consider the presence of the in-body From to be a clue that something
> > interesting is going on (like forwarding somebody else's patch). So from
> > my perspective, it would just be useless noise. Other communities may
> > have different opinions, though (I think I have seen some kernel folks
> > always including all of the possible in-body headers, including Date).
> > But it seems like it makes sense to keep both possibilities.
> 
> Exactly, current git behaviour is solely "prettier" (at first thought only 
> though), but does not address anything useful in real life.

I wouldn't agree with that. By being pretty, it also is functionally
more useful (I can tell at a glance whether somebody is sending a patch
from another author).

> Current git behaviour does cause real life problems though: Many email lists 
> are munging emails of patch senders whose domain is configured for requiring 
> domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a 
> DMARC). So original sender's From: header is then automatically replaced by an 
> alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> 
> For instance the email header:
> 
> From: "Bob Bold" <bold@foo.com>
> 
> is automatically replaced by lists by something like
> 
> From: "Bob Bold via Somelist" <somelist@gnu.org>
> 
> And since git currently always drops the From: line from the email's body if
> sender == author, as a consequence maintainers applying patches from such 
> lists, always need to rewrite git history subsequently and have to replace 
> patch author's identity manually for each commit to have their correct, real 
> email address and real name in git history instead of something like
> "Bob Bold via Somelist" <somelist@gnu.org>
> 
> So what do you find "uglier"? I prefer key info not being lost as default 
> behaviour. :-)

Sure, for your list that munges From headers, always including an
in-body From is way better. But for those of us _not_ on such lists, I'd
much prefer not to force the in-body version on them.

-Peff

Re: git format.from (was: 9p: Fix file ID collisions)

[Jeff King]

On Tue, Sep 24, 2019 at 11:03:38AM +0200, Christian Schoenebeck wrote:

> > Yes, the resulting mail would be correct, in the sense that it could be
> > applied just fine by git-am. But I think it would be uglier. IOW, I
> > consider the presence of the in-body From to be a clue that something
> > interesting is going on (like forwarding somebody else's patch). So from
> > my perspective, it would just be useless noise. Other communities may
> > have different opinions, though (I think I have seen some kernel folks
> > always including all of the possible in-body headers, including Date).
> > But it seems like it makes sense to keep both possibilities.
> 
> Exactly, current git behaviour is solely "prettier" (at first thought only 
> though), but does not address anything useful in real life.

I wouldn't agree with that. By being pretty, it also is functionally
more useful (I can tell at a glance whether somebody is sending a patch
from another author).

> Current git behaviour does cause real life problems though: Many email lists 
> are munging emails of patch senders whose domain is configured for requiring 
> domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a 
> DMARC). So original sender's From: header is then automatically replaced by an 
> alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> 
> For instance the email header:
> 
> From: "Bob Bold" <bold@foo.com>
> 
> is automatically replaced by lists by something like
> 
> From: "Bob Bold via Somelist" <somelist@gnu.org>
> 
> And since git currently always drops the From: line from the email's body if
> sender == author, as a consequence maintainers applying patches from such 
> lists, always need to rewrite git history subsequently and have to replace 
> patch author's identity manually for each commit to have their correct, real 
> email address and real name in git history instead of something like
> "Bob Bold via Somelist" <somelist@gnu.org>
> 
> So what do you find "uglier"? I prefer key info not being lost as default 
> behaviour. :-)

Sure, for your list that munges From headers, always including an
in-body From is way better. But for those of us _not_ on such lists, I'd
much prefer not to force the in-body version on them.

-Peff

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Junio C Hamano]

Eric Blake <eblake@redhat.com> writes:

> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?

I'd say that it shouldn't be so hard to implement than realizing
what ahd why it is needed, designing what the end-user interaction
would be (i.e.  command line options?  configuration variables?
should it be per send-email destination?) and stating all of the
above clearly in the documentation and the proposed commit log
message.

The reason you are asking is...?  Am I smelling a volunteer?

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Junio C Hamano]

Eric Blake <eblake@redhat.com> writes:

> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?

I'd say that it shouldn't be so hard to implement than realizing
what ahd why it is needed, designing what the end-user interaction
would be (i.e.  command line options?  configuration variables?
should it be per send-email destination?) and stating all of the
above clearly in the documentation and the proposed commit log
message.

The reason you are asking is...?  Am I smelling a volunteer?