Page MenuHomeFreeBSD

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

Authored by jceel on Apr 9 2017, 9:45 PM.

Details

Reviewers
emaste
rgrimes
jhb
Group Reviewers
bhyve
manpages
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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 30507

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
grehan edited edge metadata.Apr 9 2017, 10:17 PM

Any thoughts on the interaction with capsicum ? Seems like you just need blanket access underneath a particular directory.

jceel added a comment.Apr 9 2017, 10:21 PM

Any thoughts on the interaction with capsicum ? Seems like you just need blanket access underneath a particular directory.

Yeah, absolutely. I'm not too proficient in capsicum, but that sounds easy enough :-)

You'll have to use openat(), with the fd set up prior to cap_enter(). Here's a small example from theraven@freebsd.org, who I'm sure will be happy to provide further advice :) http://www.informit.com/articles/article.aspx?p=1924012&seqNum=2

araujo added a subscriber: araujo.Apr 10 2017, 4:52 PM

Is there another upstream repo where you'll maintain lib9p? I'm slightly surprised to see bespoke code landing in contrib/

emaste requested changes to this revision.Apr 10 2017, 5:51 PM

I've looked at the changes exclusive of the new files in contrib/lib9p, and am marking this as needs revision for the Capsicum issue discussed inline.

If/when you upload a new diff with changes for this review, can you please add full/more context to make it easier? See https://wiki.freebsd.org/Phabricator.

lib/lib9p/Makefile
12–22

sort these?

usr.sbin/bhyve/Makefile
68

This can't be committed as is (unconditionally disabling Capsicum in bhyve). For initial testing purposes you could add some sort of off-by-default conditional block that disables Capsicum and enables 9p, although it's preferable if we can address Capsicum in 3p before commit.

This revision now requires changes to proceed.Apr 10 2017, 5:51 PM

Is there another upstream repo where you'll maintain lib9p? I'm slightly surprised to see bespoke code landing in contrib/

Yes, the upstream is maintained at https://github.com/freenas/lib9p. I would like to keep it in a separate repo, as xhyve folks would also like to use it (as a git submodule).

emaste added a comment.Dec 5 2017, 8:26 PM

Yes, the upstream is maintained at https://github.com/freenas/lib9p. I would like to keep it in a separate repo, as xhyve folks would also like to use it (as a git submodule).

Ok, thanks.

Any update on a reworked patch that avoids universally forcing capsicum off for bhyve?

grehan added a subscriber: tychon.Dec 6 2017, 1:19 AM

Adding tychon@ as a subscriber: he's been doing some 9P work in recent times.

seanc added a subscriber: seanc.Jan 17 2018, 6:46 PM
emaste added a subscriber: jhb.Nov 20 2018, 5:32 PM
krion added a subscriber: krion.Dec 23 2018, 7:51 PM
jceel updated this revision to Diff 52326.Dec 27 2018, 1:33 AM

This update adds the Capsicum/Casper support to lib9p. It definitely needs more polishing, though.

dch added a subscriber: dch.Jan 25 2019, 11:20 AM
rgrimes removed a reviewer: grehan.Feb 20 2019, 5:55 PM
jhb requested changes to this revision.Jul 30 2019, 8:09 PM

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

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
31

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

49

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)

256

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.

260

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);
}
266

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)

268

This set of rights aren't used?

278

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);
This revision now requires changes to proceed.Jul 30 2019, 8:09 PM
markj added a subscriber: markj.Aug 1 2019, 10:35 PM
markj added inline comments.
contrib/lib9p/backend/fs.c
1413 ↗(On Diff #52326)

Shouldn't this use mkfifoat()?

jhb added inline comments.Aug 2 2019, 3:50 PM
usr.sbin/bhyve/pci_virtio_9p.c
268

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).

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 marked 9 inline comments as done.Dec 20 2019, 12:18 PM
jceel added inline comments.
usr.sbin/bhyve/Makefile
7

Indeed.

usr.sbin/bhyve/pci_virtio_9p.c
268

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 updated this revision to Diff 65849.Dec 20 2019, 12:24 PM
jceel marked an inline comment as done.

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

jceel added a comment.Dec 24 2019, 3:40 PM

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 updated this revision to Diff 66435.Jan 7 2020, 12:28 PM

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

jceel updated this revision to Diff 66436.Jan 7 2020, 2:18 PM

Updated missing lib/Makefile changes

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 added inline comments.Jan 22 2020, 7:32 PM
lib/lib9p/Makefile
12–22

still unsorted

jhb added inline comments.Jan 30 2020, 5:24 PM
usr.sbin/bhyve/pci_virtio_9p.c
263

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);
276

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

lwhsu added a subscriber: lwhsu.Jan 31 2020, 4:16 AM
jceel updated this revision to Diff 67588.Jan 31 2020, 10:20 PM
  • Sort lib9p SRCS in the Makefile
  • virtio-9p: error out on invalid options.
jceel marked 4 inline comments as done.Jan 31 2020, 10:22 PM
jhb added a comment.Feb 5 2020, 8:20 PM

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 added a comment.Feb 5 2020, 8:38 PM
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 added a comment.Feb 5 2020, 8:45 PM
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 added a comment.Feb 7 2020, 9:57 PM
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.

swills added a subscriber: swills.Feb 15 2020, 12:58 PM
jceel updated this revision to Diff 70580.Apr 14 2020, 10:38 PM

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 added a comment.Apr 20 2020, 7:54 PM

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

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 added a comment.Apr 20 2020, 8:09 PM

Why the move from contrib?

jhb added a comment.Apr 20 2020, 10:06 PM
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.

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 added a comment.Apr 20 2020, 11:18 PM

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.

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.

0mp added a subscriber: 0mp.Apr 21 2020, 10:29 PM
jhb added a comment.Fri, May 15, 5:07 PM

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

jceel added a comment.Fri, May 15, 5:38 PM
This comment was removed by jceel.
jceel added a comment.Fri, May 15, 5:39 PM
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 added a comment.Mon, May 18, 8:28 PM

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 accepted this revision as: manpages.Wed, May 20, 11:12 AM
bcr added a subscriber: bcr.

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

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