Changeset View
Standalone View
usr.sbin/bhyve/pci_virtio_9p.c
Show All 21 Lines | |||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | ||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | ||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | ||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | ||||
* SUCH DAMAGE. | * SUCH DAMAGE. | ||||
*/ | */ | ||||
/* | /* | ||||
* VirtIO filesystem passthrough using 9p protocol. | * VirtIO filesystem passthrough using 9p protocol. | ||||
jhb: Perhaps s/virtio/VirtIO/? And s/passtrough/passthrough/. | |||||
*/ | */ | ||||
#include <sys/cdefs.h> | #include <sys/cdefs.h> | ||||
__FBSDID("$FreeBSD$"); | __FBSDID("$FreeBSD$"); | ||||
#include <sys/param.h> | #include <sys/param.h> | ||||
#include <sys/linker_set.h> | #include <sys/linker_set.h> | ||||
#include <sys/uio.h> | #include <sys/uio.h> | ||||
#include <sys/capsicum.h> | #include <sys/capsicum.h> | ||||
#include <errno.h> | #include <errno.h> | ||||
#include <fcntl.h> | #include <fcntl.h> | ||||
#include <stdio.h> | #include <stdio.h> | ||||
#include <stdlib.h> | #include <stdlib.h> | ||||
#include <string.h> | #include <string.h> | ||||
#include <unistd.h> | #include <unistd.h> | ||||
#include <assert.h> | #include <assert.h> | ||||
#include <pthread.h> | #include <pthread.h> | ||||
Done Inline ActionsThis 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) jhb: This should be with the other sys/* headers. In general we also try to sort headers within… | |||||
#include <lib9p.h> | #include <lib9p.h> | ||||
#include <backend/fs.h> | #include <backend/fs.h> | ||||
#include "bhyverun.h" | #include "bhyverun.h" | ||||
#include "pci_emul.h" | #include "pci_emul.h" | ||||
#include "virtio.h" | #include "virtio.h" | ||||
▲ Show 20 Lines • Show All 190 Lines • ▼ Show 20 Lines | pci_vt9p_init(struct vmctx *ctx, struct pci_devinst *pi, char *opts) | ||||
while ((opt = strsep(&opts, ",")) != NULL) { | while ((opt = strsep(&opts, ",")) != NULL) { | ||||
if (strchr(opt, '=') != NULL) { | if (strchr(opt, '=') != NULL) { | ||||
if (sharename != NULL) { | if (sharename != NULL) { | ||||
printf("virtio-9p: more than one share name given\n"); | printf("virtio-9p: more than one share name given\n"); | ||||
return (1); | return (1); | ||||
} | } | ||||
sharename = strsep(&opt, "="); | sharename = strsep(&opt, "="); | ||||
rootpath = opt; | rootpath = opt; | ||||
Done Inline ActionsI 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. jhb: I think you don't need to strdup rootpath. The existing bytes in 'opts' will be left alone… | |||||
continue; | continue; | ||||
} | } | ||||
if (strcmp(opt, "ro") == 0) { | if (strcmp(opt, "ro") == 0) { | ||||
Done Inline ActionsThis 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); } jhb: This means you can't put 'ro' first. I think it would be more flexible to think about handling… | |||||
DPRINTF(("read-only mount requested\r\n")); | DPRINTF(("read-only mount requested\r\n")); | ||||
ro = true; | ro = true; | ||||
continue; | |||||
Done Inline ActionsMaybe 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); jhb: Maybe use a continue and then still check for invalid options:
```
if (strcmp(opt, "ro")… | |||||
} | } | ||||
printf("virtio-9p: invalid option '%s'\n", opt); | |||||
Done Inline ActionsMaybe 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) jhb: Maybe defer allocating the softc to avoid leaking the memory here (though we exit immediately… | |||||
return (1); | |||||
} | } | ||||
Done Inline ActionsThis set of rights aren't used? jhb: This set of rights aren't used? | |||||
Done Inline ActionsAfter 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). jhb: After talking with @markj some, I better understand what capsicumizing this meant (and your… | |||||
Done Inline ActionsThis 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: This is fixed now and the cap_rights_limit() is used on the rootcap. I agree that set of… | |||||
if (strlen(sharename) > VT9P_MAXTAGSZ) { | if (strlen(sharename) > VT9P_MAXTAGSZ) { | ||||
printf("virtio-9p: share name too long\n"); | printf("virtio-9p: share name too long\n"); | ||||
return (1); | return (1); | ||||
} | } | ||||
rootfd = open(rootpath, O_DIRECTORY); | rootfd = open(rootpath, O_DIRECTORY); | ||||
if (rootfd < 0) | if (rootfd < 0) | ||||
Done Inline ActionsIt's fine to use VT9P_MAXTAGZ directly rather than strlen(). jhb: It's fine to use VT9P_MAXTAGZ directly rather than strlen(). | |||||
return (-1); | return (-1); | ||||
Done Inline ActionsThis 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); jhb: This doesn't check for overflow. For example, what if the sharename is longer than… | |||||
sc = calloc(1, sizeof(struct pci_vt9p_softc)); | sc = calloc(1, sizeof(struct pci_vt9p_softc)); | ||||
sc->vsc_config = calloc(1, sizeof(struct pci_vt9p_config) + | sc->vsc_config = calloc(1, sizeof(struct pci_vt9p_config) + | ||||
VT9P_MAXTAGSZ); | VT9P_MAXTAGSZ); | ||||
pthread_mutex_init(&sc->vsc_mtx, NULL); | pthread_mutex_init(&sc->vsc_mtx, NULL); | ||||
cap_rights_init(&rootcap, | cap_rights_init(&rootcap, | ||||
CAP_LOOKUP, CAP_ACL_CHECK, CAP_ACL_DELETE, CAP_ACL_GET, | CAP_LOOKUP, CAP_ACL_CHECK, CAP_ACL_DELETE, CAP_ACL_GET, | ||||
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines |
Perhaps s/virtio/VirtIO/? And s/passtrough/passthrough/.