VirtFS/9p filesystem passthrough support (virtio-9p)

[jceel]

Summary

Finally I think this is good enough to start upstreaming process.

The patch contains virtio-9p bhyve piece and associated 9p server library, lib9p. lib9p is standalone and can be also used to build a TCP-based 9p server.

New bhyve(8) syntax:

-s <slot>,virtio-9p,sharename=/path/to/the/share # creates virtio-9p device called "sharename" in the guest pointing to /path/to/the/share directory.

As for now, virtio-9p doesn't work in capability mode (bhyve has to be built with WITHOUT_CAPSICUM defined).

Thanks to Chris Torek for writing large parts of the lib9p library and to Sean Eric Fagan for his contributions.

Test Plan

Add virtio-9p device to the bhyve(8) command line, as speciftied above. Boot linux guest. Type the following:

mount -t 9p -o cache=mmap -o msize=512000 testshare /mnt/9p

enjoy fast host filesystem access and overall awesomeness.

[jhb]

This does not appear to be capsicumized. The only thing I see is that 'rootcap' is initialized but not used. Ed's earlier question about what the upstream for lib9p is has also not been answered.

usr.sbin/bhyve/Makefile

7 ↗

(On Diff #52326)

I don't think you need the .PATH? You aren't adding source files from this directory, only using headers, correct?

usr.sbin/bhyve/pci_virtio_9p.c

30 ↗

(On Diff #52326)

Perhaps s/virtio/VirtIO/? And s/passtrough/passthrough/.

48 ↗

(On Diff #52326)

This should be with the other sys/* headers. In general we also try to sort headers within their respective groups (though sys/param.h should be first for the kernel headers as it is now)

255 ↗

(On Diff #52326)

I think you don't need to strdup rootpath. The existing bytes in 'opts' will be left alone since the earlier strsep() will have skipped over it already.

259 ↗

(On Diff #52326)

This means you can't put 'ro' first. I think it would be more flexible to think about handling syntax errors, etc. Also, if read-only mounts aren't supported, should we instead fail with an error until such time as it is supported?

while (...)  {
    if (strcmp(opt, "ro") == 0) {
        printf("virtio-9p: read-only mounts not supported\n");
        return (-1);
    } else if (strchr('=', opt) != NULL) {
        if (sharename != NULL) {
            printf("virtio-9p: more than one share name given\n");
            return (-1);
        }
        sharename = strep(&opt, "=");
        rootpath = opt;
    } else
        printf("virtio-9p: unknown option '%s'\n", opt);
}

265 ↗

(On Diff #52326)

Maybe defer allocating the softc to avoid leaking the memory here (though we exit immediately, but recently folks have been adding free()'s to appease coverity)

267 ↗

(On Diff #52326)

This set of rights aren't used?

277 ↗

(On Diff #52326)

This doesn't check for overflow. For example, what if the sharename is longer than VT9P_MAXTAGSZ? I would also probably just strdup() the tag name instead of padding the calloc and using strncpy, or else use memcpy or some such if the tag is not supposed to be NULL-terminated. In addition, it might be nice to defer allocating the softc until here to avoid having to free() it in earlier error conditions to avoid leaking memory:

if (strlen(sharename) > VT9P_MAXTAGSZ) {
    printf("virtio-9p: share name '%s' too long\n", sharename);
    return (-1);
}
sc = calloc(1, sizeof(*sc));
sc->vsc_config = malloc(sizeof(struct pci_vt9p_config) + strlen(sharename));
sc->vsc_config->tag_len = strlen(sharename);
memcpy(sc->vsc_config->tag, sharenane, sc-vsc_config->tag_len);

[markj]

contrib/lib9p/backend/fs.c

1413 ↗

(On Diff #52326)

Shouldn't this use mkfifoat()?

[jhb]

usr.sbin/bhyve/pci_virtio_9p.c

267 ↗

(On Diff #52326)

After talking with @markj some, I better understand what capsicumizing this meant (and your comments described), which is changing the library to use fooat(). Restricting rights on the rootfd using 'rootcap' is probably pointless since you need to allow pretty much all rights anyway, so I think you can just remove this unused variable and fix any remaining places that aren't yet using fooat() (I think Mark found one).

[emaste]

Note that my objection was specifically to the change that unconditionally disabled Capsicum in the Makefile, regardless of whether 9p support is included or not.

I really want to see this in the tree for development to happen. It's fine with me if we commit an uncapsicumized version that is not complied in by default.

[jceel]

usr.sbin/bhyve/Makefile

7 ↗

(On Diff #52326)

Indeed.

usr.sbin/bhyve/pci_virtio_9p.c

267 ↗

(On Diff #52326)

This is fixed now and the cap_rights_limit() is used on the rootcap. I agree that set of capabilities is broad, but it won't hurt to use it if it's already defined.

[jceel]

Fixed issues pointed out in the review. Added full read-only sharing support.

[jceel]

Proposed changeset passes make tinderbox run:

domino% make -j8 tinderbox
--- tinderbox ---
--- universe_prologue ---
--------------------------------------------------------------
>>> make universe started on Mon Dec 23 13:14:46 CET 2019
--------------------------------------------------------------
--- universe_riscv64_skip ---
--- universe_mips_skip ---
--- universe_sparc64_skip ---
--- universe-toolchain ---
--- universe_riscv64_skip ---
>> riscv skipped - install riscv64-xtoolchain-gcc port or package to build
--- universe_mips_skip ---
>> mips skipped - install mips-xtoolchain-gcc port or package to build
--- universe_sparc64_skip ---
>> sparc64 skipped - install sparc64-xtoolchain-gcc port or package to build
--------------------------------------------------------------
--- universe-toolchain ---
> Toolchain bootstrap started on Mon Dec 23 13:14:46 CET 2019
--------------------------------------------------------------
--------------------------------------------------------------
> Toolchain bootstrap completed on Mon Dec 23 14:05:46 CET 2019
--------------------------------------------------------------
--- universe_amd64_prologue ---
--- universe_arm_prologue ---
--- universe_arm64_prologue ---
--- universe_i386_prologue ---
--- universe_amd64_prologue ---
>> amd64 started on Mon Dec 23 14:05:46 CET 2019
--- universe_arm64_prologue ---
>> arm64 started on Mon Dec 23 14:05:46 CET 2019
--- universe_amd64_amd64 ---
--- universe_arm64_aarch64 ---
--- universe_i386_prologue ---
>> i386 started on Mon Dec 23 14:05:46 CET 2019
--- universe_arm_prologue ---
>> arm started on Mon Dec 23 14:05:46 CET 2019
--- universe_i386_i386 ---
--- universe_arm_armv6 ---
--- universe_arm_armv7 ---
--- universe_amd64_amd64 ---
>> amd64.amd64 buildworld started on Mon Dec 23 14:05:46 CET 2019
--- universe_i386_i386 ---
>> i386.i386 buildworld started on Mon Dec 23 14:05:46 CET 2019
--- universe_arm64_aarch64 ---
>> arm64.aarch64 buildworld started on Mon Dec 23 14:05:46 CET 2019
--- universe_arm_armv6 ---
>> arm.armv6 buildworld started on Mon Dec 23 14:05:46 CET 2019
>> arm.armv7 buildworld started on Mon Dec 23 14:05:46 CET 2019
>> arm.armv6 buildworld completed on Mon Dec 23 21:36:19 CET 2019
--- universe_i386_i386 ---
>> i386.i386 buildworld completed on Mon Dec 23 22:15:55 CET 2019
--- universe_i386_kernels ---
--- universe_kernels_prologue ---
>> i386 kernels started on Mon Dec 23 22:15:58 CET 2019
--- universe_kernconf_i386_GENERIC ---
>> i386.i386 GENERIC kernel started on Mon Dec 23 22:15:58 CET 2019
--- universe_kernconf_i386_GENERIC-NODEBUG ---
>> i386.i386 GENERIC-NODEBUG kernel started on Mon Dec 23 22:21:28 CET 2019
--- universe_arm64_aarch64 ---
>> arm64.aarch64 buildworld completed on Mon Dec 23 22:48:19 CET 2019
--- universe_arm64_kernels ---
--- universe_kernels_prologue ---
>> arm64 kernels started on Mon Dec 23 22:48:21 CET 2019
--- universe_kernconf_arm64_GENERIC ---
>> arm64.aarch64 GENERIC kernel started on Mon Dec 23 22:48:21 CET 2019
--- universe_i386_kernels ---
--- universe_kernconf_i386_LINT ---
>> i386.i386 LINT kernel started on Mon Dec 23 23:01:12 CET 2019
--- universe_kernconf_i386_GENERIC ---
>> i386.i386 GENERIC kernel completed on Mon Dec 23 23:18:22 CET 2019
--- universe_kernconf_i386_LINT-NOINET ---
>> i386.i386 LINT-NOINET kernel started on Mon Dec 23 23:18:22 CET 2019
--- universe_kernconf_i386_GENERIC-NODEBUG ---
>> i386.i386 GENERIC-NODEBUG kernel completed on Mon Dec 23 23:23:45 CET 2019
--- universe_kernconf_i386_LINT-NOINET6 ---
>> i386.i386 LINT-NOINET6 kernel started on Mon Dec 23 23:23:45 CET 2019
--- universe_arm64_kernels ---
>> arm64.aarch64 GENERIC kernel completed on Mon Dec 23 23:49:55 CET 2019
--- universe_kernconf_arm64_GENERIC-NODEBUG ---
>> arm64.aarch64 GENERIC-NODEBUG kernel started on Mon Dec 23 23:49:55 CET 2019
--- universe_arm_armv7 ---
>> arm.armv7 buildworld completed on Tue Dec 24 00:22:33 CET 2019
--- universe_arm_kernels ---
--- universe_kernels_prologue ---
>> arm kernels started on Tue Dec 24 00:22:37 CET 2019
--- universe_kernconf_arm_ALPINE ---
>> arm.armv7 ALPINE kernel started on Tue Dec 24 00:22:37 CET 2019
>> arm.armv7 ALPINE kernel completed on Tue Dec 24 00:33:08 CET 2019
--- universe_kernconf_arm_ARMADA38X ---
>> arm.armv7 ARMADA38X kernel started on Tue Dec 24 00:33:08 CET 2019
--- universe_arm64_kernels ---
>> arm64.aarch64 GENERIC-NODEBUG kernel completed on Tue Dec 24 00:37:30 CET 2019
--- universe_kernconf_arm64_GENERIC-UP ---
>> arm64.aarch64 GENERIC-UP kernel started on Tue Dec 24 00:37:30 CET 2019
--- universe_i386_kernels ---
>> i386.i386 LINT-NOINET6 kernel completed on Tue Dec 24 00:40:26 CET 2019
--- universe_kernconf_i386_LINT-NOIP ---
>> i386.i386 LINT-NOIP kernel started on Tue Dec 24 00:40:26 CET 2019
--- universe_kernconf_i386_LINT ---
>> i386.i386 LINT kernel completed on Tue Dec 24 00:44:12 CET 2019
--- universe_kernconf_i386_MINIMAL ---
>> i386.i386 MINIMAL kernel started on Tue Dec 24 00:44:12 CET 2019
--- universe_arm_kernels ---
--- universe_kernconf_arm_EFIKA_MX ---
>> arm.armv7 EFIKA_MX kernel started on Tue Dec 24 00:44:34 CET 2019
--- universe_i386_kernels ---
--- universe_kernconf_i386_LINT-NOINET ---
>> i386.i386 LINT-NOINET kernel completed on Tue Dec 24 00:56:13 CET 2019
--- universe_kernconf_i386_PAE ---
>> i386.i386 PAE kernel started on Tue Dec 24 00:56:13 CET 2019
--- universe_arm_kernels ---
--- universe_kernconf_arm_ARMADA38X ---
>> arm.armv7 ARMADA38X kernel completed on Tue Dec 24 01:13:05 CET 2019
--- universe_kernconf_arm_GENERIC ---
>> arm.armv7 GENERIC kernel started on Tue Dec 24 01:13:05 CET 2019
--- universe_arm64_kernels ---
>> arm64.aarch64 GENERIC-UP kernel completed on Tue Dec 24 01:36:14 CET 2019
--- universe_kernconf_arm64_LINT ---
>> arm64.aarch64 LINT kernel started on Tue Dec 24 01:36:14 CET 2019
--- universe_arm_kernels ---
--- universe_kernconf_arm_EFIKA_MX ---
>> arm.armv7 EFIKA_MX kernel completed on Tue Dec 24 01:41:36 CET 2019
--- universe_kernconf_arm_GENERIC-MMCCAM ---
>> arm.armv7 GENERIC-MMCCAM kernel started on Tue Dec 24 01:41:36 CET 2019
--- universe_i386_kernels ---
--- universe_kernconf_i386_LINT-NOIP ---
>> i386.i386 LINT-NOIP kernel completed on Tue Dec 24 01:53:38 CET 2019
--- universe_kernconf_i386_MINIMAL ---
>> i386.i386 MINIMAL kernel completed on Tue Dec 24 01:56:31 CET 2019
--- universe_kernconf_i386_PAE ---
>> i386.i386 PAE kernel completed on Tue Dec 24 02:09:35 CET 2019
--- universe_kernels ---
>> i386 kernels completed on Tue Dec 24 02:09:35 CET 2019
--- universe_i386_done ---
>> i386 completed on Tue Dec 24 02:09:35 CET 2019
--- universe_arm_kernels ---
--- universe_kernconf_arm_GENERIC ---
>> arm.armv7 GENERIC kernel completed on Tue Dec 24 02:13:39 CET 2019
--- universe_kernconf_arm_GENERIC-NODEBUG ---
>> arm.armv7 GENERIC-NODEBUG kernel started on Tue Dec 24 02:13:39 CET 2019
--- universe_kernconf_arm_GENERIC-MMCCAM ---
>> arm.armv7 GENERIC-MMCCAM kernel completed on Tue Dec 24 02:17:18 CET 2019
--- universe_kernconf_arm_IMX53 ---
>> arm.armv7 IMX53 kernel started on Tue Dec 24 02:17:18 CET 2019
--- universe_arm64_kernels ---
>> arm64.aarch64 LINT kernel completed on Tue Dec 24 02:36:51 CET 2019
--- universe_kernels ---
>> arm64 kernels completed on Tue Dec 24 02:36:51 CET 2019
--- universe_arm64_done ---
>> arm64 completed on Tue Dec 24 02:36:51 CET 2019
--- universe_arm_kernels ---
>> arm.armv7 IMX53 kernel completed on Tue Dec 24 02:40:40 CET 2019
--- universe_kernconf_arm_IMX6 ---
>> arm.armv7 IMX6 kernel started on Tue Dec 24 02:40:40 CET 2019
--- universe_kernconf_arm_GENERIC-NODEBUG ---
>> arm.armv7 GENERIC-NODEBUG kernel completed on Tue Dec 24 02:42:49 CET 2019
--- universe_kernconf_arm_LINT-V7 ---
>> arm.armv7 LINT-V7 kernel started on Tue Dec 24 02:42:49 CET 2019
--- universe_kernconf_arm_IMX6 ---
>> arm.armv7 IMX6 kernel completed on Tue Dec 24 02:56:16 CET 2019
--- universe_kernconf_arm_RPI-B ---
>> arm.armv6 RPI-B kernel started on Tue Dec 24 02:56:16 CET 2019
--- universe_kernconf_arm_LINT-V7 ---
>> arm.armv7 LINT-V7 kernel completed on Tue Dec 24 03:24:13 CET 2019
--- universe_kernconf_arm_SOCFPGA ---
>> arm.armv7 SOCFPGA kernel started on Tue Dec 24 03:24:13 CET 2019
--- universe_kernconf_arm_TEGRA124 ---
>> arm.armv7 TEGRA124 kernel started on Tue Dec 24 03:29:48 CET 2019
--- universe_kernconf_arm_RPI-B ---
>> arm.armv6 RPI-B kernel completed on Tue Dec 24 03:31:12 CET 2019
--- universe_kernconf_arm_VERSATILEPB ---
>> arm.armv6 VERSATILEPB kernel started on Tue Dec 24 03:31:12 CET 2019
--- universe_amd64_amd64 ---
>> amd64.amd64 buildworld completed on Tue Dec 24 03:33:35 CET 2019
--- universe_amd64_kernels ---
--- universe_kernels_prologue ---
>> amd64 kernels started on Tue Dec 24 03:33:37 CET 2019
--- universe_kernconf_amd64_GENERIC ---
>> amd64.amd64 GENERIC kernel started on Tue Dec 24 03:33:37 CET 2019
--- universe_arm_kernels ---
--- universe_kernconf_arm_SOCFPGA ---
>> arm.armv7 SOCFPGA kernel completed on Tue Dec 24 03:35:47 CET 2019
--- universe_kernconf_arm_VYBRID ---
>> arm.armv7 VYBRID kernel started on Tue Dec 24 03:35:47 CET 2019
--- universe_kernconf_arm_VERSATILEPB ---
>> arm.armv6 VERSATILEPB kernel completed on Tue Dec 24 03:36:24 CET 2019
--- universe_kernconf_arm_ZEDBOARD ---
>> arm.armv7 ZEDBOARD kernel started on Tue Dec 24 03:36:24 CET 2019
--- universe_kernconf_arm_TEGRA124 ---
>> arm.armv7 TEGRA124 kernel completed on Tue Dec 24 03:59:50 CET 2019
--- universe_kernconf_arm_VYBRID ---
>> arm.armv7 VYBRID kernel completed on Tue Dec 24 04:04:08 CET 2019
--- universe_kernconf_arm_ZEDBOARD ---
>> arm.armv7 ZEDBOARD kernel completed on Tue Dec 24 04:07:47 CET 2019
--- universe_kernels ---
>> arm kernels completed on Tue Dec 24 04:07:47 CET 2019
--- universe_arm_done ---
>> arm completed on Tue Dec 24 04:07:47 CET 2019
--- universe_amd64_kernels ---
--- universe_kernconf_amd64_GENERIC-KCSAN ---
>> amd64.amd64 GENERIC-KCSAN kernel started on Tue Dec 24 04:07:47 CET 2019
--- universe_kernconf_amd64_GENERIC ---
>> amd64.amd64 GENERIC kernel completed on Tue Dec 24 04:10:36 CET 2019
--- universe_kernconf_amd64_GENERIC-MMCCAM ---
>> amd64.amd64 GENERIC-MMCCAM kernel started on Tue Dec 24 04:10:36 CET 2019
--- universe_kernconf_amd64_GENERIC-NODEBUG ---
>> amd64.amd64 GENERIC-NODEBUG kernel started on Tue Dec 24 04:18:48 CET 2019
--- universe_kernconf_amd64_GENERIC-KCSAN ---
>> amd64.amd64 GENERIC-KCSAN kernel completed on Tue Dec 24 04:37:35 CET 2019
--- universe_kernconf_amd64_LINT ---
>> amd64.amd64 LINT kernel started on Tue Dec 24 04:37:35 CET 2019
--- universe_kernconf_amd64_GENERIC-MMCCAM ---
>> amd64.amd64 GENERIC-MMCCAM kernel completed on Tue Dec 24 04:43:11 CET 2019
--- universe_kernconf_amd64_LINT-NOINET ---
>> amd64.amd64 LINT-NOINET kernel started on Tue Dec 24 04:43:11 CET 2019
--- universe_kernconf_amd64_GENERIC-NODEBUG ---
>> amd64.amd64 GENERIC-NODEBUG kernel completed on Tue Dec 24 04:48:13 CET 2019
--- universe_kernconf_amd64_LINT-NOINET6 ---
>> amd64.amd64 LINT-NOINET6 kernel started on Tue Dec 24 04:48:13 CET 2019
--- universe_kernconf_amd64_LINT ---
>> amd64.amd64 LINT kernel completed on Tue Dec 24 05:23:28 CET 2019
--- universe_kernconf_amd64_LINT-NOIP ---
>> amd64.amd64 LINT-NOIP kernel started on Tue Dec 24 05:23:28 CET 2019
--- universe_kernconf_amd64_LINT-NOINET ---
>> amd64.amd64 LINT-NOINET kernel completed on Tue Dec 24 05:26:07 CET 2019
--- universe_kernconf_amd64_MINIMAL ---
>> amd64.amd64 MINIMAL kernel started on Tue Dec 24 05:26:07 CET 2019
--- universe_kernconf_amd64_LINT-NOINET6 ---
>> amd64.amd64 LINT-NOINET6 kernel completed on Tue Dec 24 05:26:57 CET 2019
--- universe_kernconf_amd64_MINIMAL ---
>> amd64.amd64 MINIMAL kernel completed on Tue Dec 24 05:44:57 CET 2019
--- universe_kernconf_amd64_LINT-NOIP ---
>> amd64.amd64 LINT-NOIP kernel completed on Tue Dec 24 05:48:39 CET 2019
--- universe_kernels ---
>> amd64 kernels completed on Tue Dec 24 05:48:39 CET 2019
--- universe_amd64_done ---
>> amd64 completed on Tue Dec 24 05:48:39 CET 2019
--- universe_epilogue ---
--------------------------------------------------------------
>>> make universe completed on Tue Dec 24 05:48:39 CET 2019
(started Mon Dec 23 13:14:46 CET 2019)
--------------------------------------------------------------

[jceel]

Added bsd.libnames.mk and src.libnames.mk changes left out in previous update.

[jceel]

Updated missing lib/Makefile changes

[emaste]

The next time you upload please include context as described in https://wiki.freebsd.org/Phabricator.
I think it's probably also worth splitting lib9p itself out into a separate review. (if you do that you can keep this as the one that adds it to bhyve)

[emaste]

lib/lib9p/Makefile

11–21 ↗

(On Diff #27273)

still unsorted

[jhb]

usr.sbin/bhyve/pci_virtio_9p.c

262 ↗

(On Diff #66436)

Maybe use a continue and then still check for invalid options:

if (strcmp(opt, "ro") == 0) {
    DPRINTF(...);
    ro = true;
    continue;
}

printf("virtio-9p: unknown option '%s'\n", opt);

275 ↗

(On Diff #66436)

It's fine to use VT9P_MAXTAGZ directly rather than strlen().

[jceel]

• Sort lib9p SRCS in the Makefile
• virtio-9p: error out on invalid options.

[jhb]

So this looks good to me, but the latest upload lost all the lib9p sources. I finally found the lib9p upstream, though the only version I see is in your GitHub space? I would be fine with still using that as an upstream though and putting the sources into contrib/lib9p (despite the last call where I said it could move). I think it would be good to first import lib9p into the vendor/ space and then contrib/, then a commit to hook it up to the build (being able to hook the tests up to run as part of the kyua test suite runs would be a bonus), and finally the commit to add it to bhyve 3rd.

[emaste]

In D10335#516241, @jhb wrote:

I think it would be good to first import lib9p into the vendor/ space and then contrib/

I agree, and I can help with the process if necessary.

[jceel]

In D10335#516241, @jhb wrote:

So this looks good to me, but the latest upload lost all the lib9p sources. I finally found the lib9p upstream, though the only version I see is in your GitHub space? I would be fine with still using that as an upstream though and putting the sources into contrib/lib9p (despite the last call where I said it could move). I think it would be good to first import lib9p into the vendor/ space and then contrib/, then a commit to hook it up to the build (being able to hook the tests up to run as part of the kyua test suite runs would be a bonus), and finally the commit to add it to bhyve 3rd.

Can you elaborate on what I did wrong with the latest patch update? I generated it using arc diff HEAD^^ -- update D10335 which resulted in a diff only in files changed since last upload (two git commits from my development branch on github).

Regarding the vendor/ space - currently lib9p doesn't have any specific release process or version numbering - I'm simply importing it to contrib/ space by putting lib9p git hash in the commit message. Shall I just do lib9p-<sha256> kind of imports into vendor/?

[jhb]

In D10335#516265, @jceel wrote:

In D10335#516241, @jhb wrote:

So this looks good to me, but the latest upload lost all the lib9p sources. I finally found the lib9p upstream, though the only version I see is in your GitHub space? I would be fine with still using that as an upstream though and putting the sources into contrib/lib9p (despite the last call where I said it could move). I think it would be good to first import lib9p into the vendor/ space and then contrib/, then a commit to hook it up to the build (being able to hook the tests up to run as part of the kyua test suite runs would be a bonus), and finally the commit to add it to bhyve 3rd.

Can you elaborate on what I did wrong with the latest patch update? I generated it using arc diff HEAD^^ -- update D10335 which resulted in a diff only in files changed since last upload (two git commits from my development branch on github).

Yes, you want to tell arc the root of where to diff against and always upload a "full" diff. This means that I normally do something like arc diff --update Dxxxx master and it finds the common revision between my branch and 'master' (which I keep in sync with FreeBSD's 'master') as the basis of where to diff from. IOW, you don't give arc incremental diffs, but always give it the full diff each time. It is able to let you compare those full diffs in its UI.

Regarding the vendor/ space - currently lib9p doesn't have any specific release process or version numbering - I'm simply importing it to contrib/ space by putting lib9p git hash in the commit message. Shall I just do lib9p-<sha256> kind of imports into vendor/?

Yes, using the git hash as the version number is fine. I think there are projects that already do that today such as llvm imports.

[jceel]

Removed contrib/lib9p/ and moved all lib9p sources under lib/. Consolidated two Makefiles (upstream contrib/lib9p/Makefile and local lib/lib9p/Makefile) into one.

[jhb]

So assuming a non-contrib model, the library needs to look like it is a normal base system library. This means no separate COPYRIGHT file, no README.md file, no GNUmakefile or apple_endian.h. Each file needs an accurate copyright statement. I think all the files do, but none of them mention libipx mentioned in COPYRIGHT, so if libipx bits are used, then the files using those need to have a proper attribution in their license statement.

lib/lib9p/Makefile

7 ↗

(On Diff #70580)

You probably don't want to override DEBUG_FLAGS here and let it by the default of '-g'. When are the cases when you would want L9P_DEBUG and ACE_DEBUG defined vs not? For example, do you want them always on (add them to CFLAGS), do you only want them enabled on HEAD (similar to MALLOC_PRODUCTION), or do you want to have a new makefile variable to turn them on for people hacking on lib9p directly but not on by default?

[imp]

Why the move from contrib?

[jhb]

In D10335#539180, @imp wrote:

Why the move from contrib?

emaste@ felt it was better to not do it in contrib/. I'm not sure there are any other realistic non-FreeBSD consumers either. Perhaps xhyve would pick it up for use on OS X, but it's not clear that will happen.

[jceel]

In D10335#539175, @jhb wrote:

So assuming a non-contrib model, the library needs to look like it is a normal base system library. This means no separate COPYRIGHT file, no README.md file, no GNUmakefile or apple_endian.h. Each file needs an accurate copyright statement. I think all the files do, but none of them mention libipx mentioned in COPYRIGHT, so if libipx bits are used, then the files using those need to have a proper attribution in their license statement.

OK, so this is definitely not possible because the upstream:

• is hosted on github
• is buildable as a standalone library
• can be built with GNU Make
• can be fitted to xhyve as well

Are we going back to the vendor import then?

[jhb]

Well, do we think the GH will still be used once this is imported? If we think only FreeBSD will use this and that it will be fine to just live as a FreeBSD library then importing into lib/lib9p is fine and there's no need for the other bits that would get dropped. The argument for not doing vendor was that we didn't have a real upstream. If we have a real upstream, then it needs to be in contrib/.

BTW, even in upstream I think the issue of which files contain bits from libipx is probably still relevant and it would be good to update the per-file copyright statements to ACK that library in the relevant files.

[jceel]

In D10335#539242, @jhb wrote:

Well, do we think the GH will still be used once this is imported? If we think only FreeBSD will use this and that it will be fine to just live as a FreeBSD library then importing into lib/lib9p is fine and there's no need for the other bits that would get dropped. The argument for not doing vendor was that we didn't have a real upstream. If we have a real upstream, then it needs to be in contrib/.

BTW, even in upstream I think the issue of which files contain bits from libipx is probably still relevant and it would be good to update the per-file copyright statements to ACK that library in the relevant files.

Yes, it will be used for non-FreeBSD consumers such as xhyve. I don't want this library to dissolve in FreeBSD source tree. Sounds like vendor import and then going back to contrib/ is the only way to do it.

[jhb]

Can you refresh this with the updating lib9p/Makefile you plan to use?

[jceel]

This comment was removed by jceel.

[jceel]

In D10335#547251, @jhb wrote:

Can you refresh this with the updating lib9p/Makefile you plan to use?

The only change I'm planning is to revert to the previous version of the patch utilizing contrib/lib9p.

[jhb]

Surely the Makefile now needs .PATH added to find the sources in contrib/? Also, there was earlier feedback on lib/lib9p/Makefile and the Makefile hasn't been updated since then.

[bcr]

OK from manpages, with the usual hint to bump the .Dd when the commit happens.

[emaste]

Will you refresh this review now that the library is in vendor (and merged to contrib)?

[jceel]

Revert back to previous concept (sort of).

Remove contrib/lib9p from the diff as it is committed to HEAD already.

[emaste]

Looking good. Two inline comments and I have not yet reviewed usr.sbin/bhyve/pci_virtio_9p.c

lib/lib9p/Makefile

4–6 ↗

(On Diff #73123)

-DWITH_CASPER repeated twice, and odd combination of -D and -I on the same line in one case and separate lines in other case. I would probably put the two -I entries on one line and -D on its own.

7 ↗

(On Diff #70580)

still need to address that comment

[rgrimes]

I was asked to look at this and see if I could help get the Makefile issues moved forward. I have commented inline.

lib/lib9p/Makefile

4–6 ↗

(On Diff #73123)

Suggested replacement for lines 4 to 6 that I think would take care of Ed's comment:

CFLAGS+=        -DWITH_CASPER
CFLAGS+=        -I${.CURDIR}
CFLAGS+=        -I${.CURDIR}/../../contrib/lib9p

7 ↗

(On Diff #70580)

I would just delete this line which addresses the concern quickly and allows this review to move forward unless there is some great reason that this debug stuff is needed in the production code.

[jceel]

Clean up makefile.

[jhb]

I think this is fine now at least for the lib9p bits as next commit (i.e. everything except usr.sbin/bhyve which I still think should be a 3rd commit).

Once it's committed it would be good to refresh with the latest version of the bhyve bits.

[jceel]

In D10335#568530, @jhb wrote:

I think this is fine now at least for the lib9p bits as next commit (i.e. everything except usr.sbin/bhyve which I still think should be a 3rd commit).

Once it's committed it would be good to refresh with the latest version of the bhyve bits.

I would like to commit this revision as it is now. If you have no further comments, then please approve it.

[rgrimes]

I still see request changes markers from emaste and jhb/bhyve, what are these changes?

[emaste]

In D10335#573502, @rgrimes wrote:

I still see request changes markers from emaste and jhb/bhyve, what are these changes?

The status from jhb and me means that changes were requested on an earlier version.
I have not yet had a chance to review what remains in detail, but believe all of the issues I observed earlier have been addressed and have no objections at this point.