[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process fo
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS |
Date: |
Tue, 15 Nov 2011 14:03:05 +0000 |
On Tue, Nov 15, 2011 at 11:57 AM, M. Mohan Kumar <address@hidden> wrote:
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> new file mode 100644
> index 0000000..69daf7c
> --- /dev/null
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -0,0 +1,271 @@
> +/*
> + * Helper for QEMU Proxy FS Driver
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + * M. Mohan Kumar <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +#include <stdio.h>
> +#include <sys/socket.h>
> +#include <string.h>
> +#include <sys/un.h>
> +#include <limits.h>
> +#include <signal.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <sys/resource.h>
> +#include <sys/stat.h>
> +#include <getopt.h>
> +#include <unistd.h>
> +#include <syslog.h>
> +#include <sys/prctl.h>
> +#include <sys/capability.h>
> +#include <sys/fsuid.h>
> +#include <stdarg.h>
> +#include "bswap.h"
Where is "bswap.h" used and why above <sys/socket.h>?
> +#include <sys/socket.h>
> +#include "qemu-common.h"
> +#include "virtio-9p-marshal.h"
> +#include "hw/9pfs/virtio-9p-proxy.h"
> +
> +#define PROGNAME "virtfs-proxy-helper"
> +
> +static struct option helper_opts[] = {
> + {"fd", required_argument, NULL, 'f'},
> + {"path", required_argument, NULL, 'p'},
> + {"nodaemon", no_argument, NULL, 'n'},
> +};
> +
> +int is_daemon;
static?
Also, please use the bool type from <stdbool.h>, it makes it easier
for readers who don't have to guess how the variable works (might be a
bitfield or reference count too).
> +static int socket_read(int sockfd, void *buff, ssize_t size)
> +{
> + int retval;
> +
> + do {
> + retval = read(sockfd, buff, size);
> + } while (retval < 0 && errno == EINTR);
> + if (retval != size) {
> + return -EIO;
> + }
Shouldn't this loop until size bytes have been read?
> + return retval;
> +}
> +
> +static int socket_write(int sockfd, void *buff, ssize_t size)
> +{
> + int retval;
> +
> + do {
> + retval = write(sockfd, buff, size);
> + } while (retval < 0 && errno == EINTR);
> + if (retval != size) {
> + return -EIO;
We could pass the actual -errno here if retval < 0.
> + }
> + return retval;
> +}
> +
> +static int read_request(int sockfd, struct iovec *iovec)
> +{
> + int retval;
> + ProxyHeader header;
> +
> + /* read the header */
> + retval = socket_read(sockfd, iovec->iov_base, sizeof(header));
> + if (retval != sizeof(header)) {
> + return -EIO;
> + }
> + /* unmarshal header */
> + proxy_unmarshal(iovec, 1, 0, "dd", &header.type, &header.size);
> + /* read the request */
> + retval = socket_read(sockfd, iovec->iov_base + sizeof(header),
> header.size);
> + if (retval != header.size) {
> + return -EIO;
> + }
> + return header.type;
> +}
Size checks are missing and we're trusting what the client sends!
> +
> +static void usage(char *prog)
> +{
> + fprintf(stderr, "usage: %s\n"
> + " -p|--path <path> 9p path to export\n"
> + " {-f|--fd <socket-descriptor>} socket file descriptor to be
> used\n"
> + " [-n|--nodaemon] Run as a normal program\n",
> + basename(prog));
> +}
> +
> +static int process_requests(int sock)
> +{
> + int type;
> + struct iovec iovec;
> +
> + iovec.iov_base = g_malloc(BUFF_SZ);
> + iovec.iov_len = BUFF_SZ;
> + while (1) {
> + type = read_request(sock, &iovec);
> + if (type <= 0) {
> + goto error;
> + }
> + }
> + (void)socket_write;
> +error:
> + g_free(iovec.iov_base);
> + return -1;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int sock;
> + char rpath[PATH_MAX];
> + struct stat stbuf;
> + int c, option_index;
> +
> + is_daemon = 1;
> + rpath[0] = '\0';
> + sock = -1;
> + while (1) {
> + option_index = 0;
> + c = getopt_long(argc, argv, "p:nh?f:", helper_opts,
> + &option_index);
> + if (c == -1) {
> + break;
> + }
> + switch (c) {
> + case 'p':
> + strcpy(rpath, optarg);
Buffer overflow. The whole thing would be simpler like this:
const char *rpath = "";
[...]
case 'p':
rpath = optarg;
break;
> + break;
> + case 'n':
> + is_daemon = 0;
> + break;
> + case 'f':
> + sock = atoi(optarg);
> + break;
> + case '?':
> + case 'h':
> + default:
> + usage(argv[0]);
> + return -1;
The convention is for programs to exit with 1 (EXIT_FAILURE) on error.
> + break;
> + }
> + }
> +
> + /* Parameter validation */
> + if (sock == -1 || rpath[0] == '\0') {
> + fprintf(stderr, "socket descriptor or path not specified\n");
> + usage(argv[0]);
> + return -1;
> + }
> +
> + if (lstat(rpath, &stbuf) < 0) {
> + fprintf(stderr, "invalid path \"%s\" specified?\n", rpath);
sterror() would provide further details on what went wrong.
> + return -1;
> + }
> +
> + if (!S_ISDIR(stbuf.st_mode)) {
> + fprintf(stderr, "specified path \"%s\" is not directory\n", rpath);
> + return -1;
> + }
> +
> + if (is_daemon) {
> + if (daemon(0, 0) < 0) {
> + fprintf(stderr, "daemon call failed\n");
> + return -1;
> + }
> + openlog(PROGNAME, LOG_PID, LOG_DAEMON);
> + }
> +
> + do_log(LOG_INFO, "Started");
> +
> + if (chroot(rpath) < 0) {
> + do_perror("chroot");
> + goto error;
> + }
> + umask(0);
> +
> + if (init_capabilities() < 0) {
> + goto error;
> + }
> +
> + process_requests(sock);
> +error:
> + do_log(LOG_INFO, "Done");
> + closelog();
> + return 0;
> +}
> diff --git a/hw/9pfs/virtio-9p-proxy.h b/hw/9pfs/virtio-9p-proxy.h
> index f5e1a02..120e940 100644
> --- a/hw/9pfs/virtio-9p-proxy.h
> +++ b/hw/9pfs/virtio-9p-proxy.h
> @@ -3,6 +3,16 @@
>
> #define BUFF_SZ (4 * 1024)
>
> +#define proxy_unmarshal(in_sg, in_elem, offset, fmt, args...) \
> + v9fs_unmarshal(in_sg, in_elem, offset, 0 /* convert */, fmt, ##args)
> +#define proxy_marshal(out_sg, out_elem, offset, fmt, args...) \
> + v9fs_marshal(out_sg, out_elem, offset, 0 /* convert */, fmt, ##args)
> +
> +union MsgControl {
> + struct cmsghdr cmsg;
> + char control[CMSG_SPACE(sizeof(int))];
> +};
This union isn't used in this patch.
Stefan
- [Qemu-devel] [PATCH V2 01/12] hw/9pfs: Move pdu_marshal/unmarshal code to a seperate file, (continued)
- [Qemu-devel] [PATCH V2 01/12] hw/9pfs: Move pdu_marshal/unmarshal code to a seperate file, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 06/12] hw/9pfs: Add stat/readlink/statfs for proxy FS, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 05/12] hw/9pfs: Create other filesystem objects, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 04/12] hw/9pfs: Open and create files, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 09/12] hw/9pfs: Proxy getversion, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 10/12] hw/9pfs: Documentation changes related to proxy fs, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 02/12] hw/9pfs: Add new proxy filesystem driver, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 12/12] hw/9pfs: Add support to use named socket for proxy FS, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS, M. Mohan Kumar, 2011/11/15
- Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCH V2 00/12] Proxy FS driver for VirtFS, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH V2 11/12] hw/9pfs: man page for proxy helper, M. Mohan Kumar, 2011/11/15
- [Qemu-devel] [PATCH 00/12] Proxy FS driver for VirtFS, M. Mohan Kumar, 2011/11/15