thr3ads.net - Linux Virtualization - [PATCH 2/3] qemu: Implement virtio-pstore device [Aug 2016]

If this information is useful, please help other people find it:
Share via:

Daniel P. Berrange

2016-Aug-24 22:00 UTC

[PATCH 2/3] qemu: Implement virtio-pstore device

> diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> new file mode 100644
> index 0000000..b8fb4be
> --- /dev/null
> +++ b/hw/virtio/virtio-pstore.c
> @@ -0,0 +1,699 @@
> +/*
> + * Virtio Pstore Device
> + *
> + * Copyright (C) 2016  LG Electronics
> + *
> + * Authors:
> + *  Namhyung Kim  <namhyung at gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdio.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/iov.h"
> +#include "qemu-common.h"
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "qapi/visitor.h"
> +#include "qapi-event.h"
> +#include "io/channel-util.h"
> +#include "trace.h"
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +#include "hw/virtio/virtio-pstore.h"
> +
> +#define PSTORE_DEFAULT_BUFSIZE   (16 * 1024)
> +#define PSTORE_DEFAULT_FILE_MAX  5
> +
> +/* the index should match to the type value */
> +static const char *virtio_pstore_file_prefix[] = {
> +    "unknown-",		/* VIRTIO_PSTORE_TYPE_UNKNOWN */
> +    "dmesg-",		/* VIRTIO_PSTORE_TYPE_DMESG */
> +};
> +
> +static char *virtio_pstore_to_filename(VirtIOPstore *s,
> +                                       struct virtio_pstore_req *req)
> +{
> +    const char *basename;
> +    unsigned long long id;
> +    unsigned int type = le16_to_cpu(req->type);
> +    unsigned int flags = le32_to_cpu(req->flags);
> +
> +    if (type < ARRAY_SIZE(virtio_pstore_file_prefix)) {
> +        basename = virtio_pstore_file_prefix[type];
> +    } else {
> +        basename = "unknown-";
> +    }
> +
> +    id = s->id++;
> +    return g_strdup_printf("%s/%s%llu%s", s->directory,
basename, id,
> +                            flags & VIRTIO_PSTORE_FL_COMPRESSED ?
".enc.z" : "");
> +}
> +
> +static char *virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> +                                         struct virtio_pstore_fileinfo
*info)
> +{
> +    char *filename;
> +    unsigned int idx;
> +
> +    filename = g_strdup_printf("%s/%s", s->directory, name);
> +    if (filename == NULL)
> +        return NULL;
> +
> +    for (idx = 0; idx < ARRAY_SIZE(virtio_pstore_file_prefix); idx++) {
> +        if (g_str_has_prefix(name, virtio_pstore_file_prefix[idx])) {
> +            info->type = idx;
> +            name += strlen(virtio_pstore_file_prefix[idx]);
> +            break;
> +        }
> +    }
> +
> +    if (idx == ARRAY_SIZE(virtio_pstore_file_prefix)) {
> +        g_free(filename);
> +        return NULL;
> +    }
> +
> +    qemu_strtoull(name, NULL, 0, &info->id);
> +
> +    info->flags = 0;
> +    if (g_str_has_suffix(name, ".enc.z")) {
> +        info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> +    }
> +
> +    return filename;
> +}
> +
> +static int prefix_idx;
> +static int prefix_count;
> +static int prefix_len;
> +
> +static int filter_pstore(const struct dirent *de)
> +{
> +    int i;
> +
> +    for (i = 0; i < prefix_count; i++) {
> +        const char *prefix = virtio_pstore_file_prefix[prefix_idx + i];
> +
> +        if (g_str_has_prefix(de->d_name, prefix)) {
> +            return 1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int sort_pstore(const struct dirent **a, const struct dirent **b)
> +{
> +    uint64_t id_a, id_b;
> +
> +    qemu_strtoull((*a)->d_name + prefix_len, NULL, 0, &id_a);
> +    qemu_strtoull((*b)->d_name + prefix_len, NULL, 0, &id_b);
> +
> +    return id_a - id_b;
> +}
> +
> +static int rotate_pstore_file(VirtIOPstore *s, unsigned short type)
AFAIK you're not actually doing file rotation here - that implies a
fixed base filename, with .0, .1, .2, etc suffixes where we rename
files each time. It looks like you are assuming separate filenames,
and are merely deleting the oldest each time.
> +{
> +    int ret = 0;
> +    int i, num;
> +    char *filename;
> +    struct dirent **files;
> +
> +    if (type >= ARRAY_SIZE(virtio_pstore_file_prefix)) {
> +        type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> +    }
> +
> +    prefix_idx = type;
> +    prefix_len = strlen(virtio_pstore_file_prefix[type]);
> +    prefix_count = 1;  /* only scan current type */
> +
> +    /* delete the oldest file in the same type */
> +    num = scandir(s->directory, &files, filter_pstore,
sort_pstore);
> +    if (num < 0)
> +        return num;
> +    if (num < (int)s->file_max)
> +        goto out;
> +
> +    filename = g_strdup_printf("%s/%s", s->directory,
files[0]->d_name);
> +    if (filename == NULL) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    ret = unlink(filename);



> +static gboolean pstore_async_read_fn(QIOChannel *ioc, GIOCondition
condition,
> +                                     gpointer data)
> +{
> +    struct pstore_read_arg *rarg = data;
> +    struct virtio_pstore_fileinfo *info = &rarg->info;
> +    VirtIOPstore *vps = rarg->vps;
> +    VirtQueueElement *elem = rarg->elem;
> +    struct virtio_pstore_res res;
> +    size_t offset = sizeof(res) + sizeof(*info);
> +    struct iovec *sg = elem->in_sg;
> +    unsigned int sg_num = elem->in_num;
> +    Error *err = NULL;
> +    ssize_t len;
> +    int ret;
> +
> +    /* skip res and fileinfo */
> +    iov_discard_front(&sg, &sg_num, sizeof(res) + sizeof(*info));
> +
> +    len = qio_channel_readv(rarg->ioc, sg, sg_num, &err);
> +    if (len < 0) {
> +        if (errno == EAGAIN) {
> +            len = 0;
> +        }
> +        ret = -1;
> +    } else {
> +        info->len = cpu_to_le32(len);
> +        ret = 0;
> +    }
> +
> +    res.cmd  = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> +    res.type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
> +    res.ret  = cpu_to_le32(ret);
> +
> +    /* now copy res and fileinfo */
> +    iov_from_buf(elem->in_sg, elem->in_num, 0, &res,
sizeof(res));
> +    iov_from_buf(elem->in_sg, elem->in_num, sizeof(res), info,
sizeof(*info));
> +
> +    len += offset;
> +    virtqueue_push(vps->rvq, elem, len);
> +    virtio_notify(VIRTIO_DEVICE(vps), vps->rvq);
> +
> +    return G_SOURCE_REMOVE;
G_SOURCE_REMOVE was added in glib 2.32, but QEMU only permits
stuff that is present in 2.22. Just use "FALSE" instead.
> +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, VirtQueueElement
*elem)
> +{
> +    char *filename = NULL;
> +    int fd, idx;
> +    struct stat stbuf;
> +    struct pstore_read_arg *rarg = NULL;
> +    Error *err = NULL;
> +    int ret = -1;
> +
> +    if (s->file_idx >= s->num_file) {
> +        return 0;
> +    }
> +
> +    rarg = g_malloc(sizeof(*rarg));
> +    if (rarg == NULL) {
> +        return -1;
> +    }
> +
> +    idx = s->file_idx++;
> +    filename = virtio_pstore_from_filename(s, s->files[idx]->d_name,
> +                                           &rarg->info);
> +    if (filename == NULL) {
> +        goto out;
> +    }
> +
> +    fd = open(filename, O_RDONLY);
> +    if (fd < 0) {
> +        error_report("cannot open %s", filename);
> +        goto out;
> +    }
> +
> +    if (fstat(fd, &stbuf) < 0) {
> +        goto out;
> +    }
> +
> +    rarg->vps            = s;
> +    rarg->elem           = elem;
> +    rarg->info.id        = cpu_to_le64(rarg->info.id);
> +    rarg->info.type      = cpu_to_le16(rarg->info.type);
> +    rarg->info.flags     = cpu_to_le32(rarg->info.flags);
> +    rarg->info.time_sec  = cpu_to_le64(stbuf.st_ctim.tv_sec);
> +    rarg->info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> +
> +    rarg->ioc = qio_channel_new_fd(fd, &err);
You should just use qio_channel_open_path() and avoid the earlier
call to open()
> +    if (err) {
> +        error_reportf_err(err, "cannot create io channel: ");
> +        goto out;
> +    }
> +
> +    qio_channel_set_blocking(rarg->ioc, false, &err);
> +    qio_channel_add_watch(rarg->ioc, G_IO_IN, pstore_async_read_fn,
rarg,
> +                          free_rarg_fn);
> +    g_free(filename);
> +    return 1;
> +
> +out:
> +    g_free(filename);
> +    g_free(rarg);
> +
> +    return ret;
> +}
> +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, VirtQueueElement
*elem,
> +                                      struct virtio_pstore_req *req)
> +{
> +    unsigned short type = le16_to_cpu(req->type);
> +    char *filename = NULL;
> +    int fd;
> +    int flags = O_WRONLY | O_CREAT | O_TRUNC;
> +    struct pstore_write_arg *warg = NULL;
> +    Error *err = NULL;
> +    int ret = -1;
> +
> +    /* do not keep same type of files more than 'file-max' */
> +    rotate_pstore_file(s, type);
> +
> +    filename = virtio_pstore_to_filename(s, req);
> +    if (filename == NULL) {
> +        return -1;
> +    }
> +
> +    warg = g_malloc(sizeof(*warg));
> +    if (warg == NULL) {
> +        goto out;
> +    }
> +
> +    fd = open(filename, flags, 0644);
> +    if (fd < 0) {
> +        error_report("cannot open %s", filename);
> +        ret = fd;
> +        goto out;
> +    }
> +
> +    warg->vps            = s;
> +    warg->elem           = elem;
> +    warg->req            = req;
> +
> +    warg->ioc = qio_channel_new_fd(fd, &err);
Same point about using new_path() instead of new_fd()
> +    if (err) {
> +        error_reportf_err(err, "cannot create io channel: ");
> +        goto out;
> +    }
> +
> +    qio_channel_set_blocking(warg->ioc, false, &err);
> +    qio_channel_add_watch(warg->ioc, G_IO_OUT, pstore_async_write_fn,
warg,
> +                          free_warg_fn);
> +    g_free(filename);
> +    return 1;
> +
> +out:
> +    g_free(filename);
> +    g_free(warg);
> +    return ret;
> +}
> +
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> +    VirtQueueElement *elem;
> +    struct virtio_pstore_req req;
> +    struct virtio_pstore_res res;
> +    ssize_t len = 0;
> +    int ret;
> +
> +    for (;;) {
> +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +        if (!elem) {
> +            return;
> +        }
> +
> +        if (elem->out_num < 1 || elem->in_num < 1) {
> +            error_report("request or response buffer is
missing");
> +            exit(1);
> +        }
> +
> +        if (elem->out_num > 2 || elem->in_num > 3) {
> +            error_report("invalid number of input/output
buffer");
> +            exit(1);
> +        }
> +
> +        len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req,
sizeof(req));
> +        if (len != (ssize_t)sizeof(req)) {
> +            error_report("invalid request size: %ld",
(long)len);
> +            exit(1);
> +        }
> +        res.cmd  = req.cmd;
> +        res.type = req.type;
> +
> +        switch (le16_to_cpu(req.cmd)) {
> +        case VIRTIO_PSTORE_CMD_OPEN:
> +            ret = virtio_pstore_do_open(s);
> +            break;
> +        case VIRTIO_PSTORE_CMD_CLOSE:
> +            ret = virtio_pstore_do_close(s);
> +            break;
> +        case VIRTIO_PSTORE_CMD_ERASE:
> +            ret = virtio_pstore_do_erase(s, &req);
> +            break;
> +        case VIRTIO_PSTORE_CMD_READ:
> +            ret = virtio_pstore_do_read(s, elem);
> +            if (ret == 1) {
> +                /* async channel io */
> +                continue;
> +            }
> +            break;
> +        case VIRTIO_PSTORE_CMD_WRITE:
> +            ret = virtio_pstore_do_write(s, elem, &req);
> +            if (ret == 1) {
> +                /* async channel io */
> +                continue;
> +            }
> +            break;
> +        default:
> +            ret = -1;
> +            break;
> +        }
> +
> +        res.ret = ret;
> +
> +        iov_from_buf(elem->in_sg, elem->in_num, 0, &res,
sizeof(res));
> +        virtqueue_push(vq, elem, sizeof(res) + len);
> +
> +        virtio_notify(vdev, vq);
> +        g_free(elem);
> +
> +        if (ret < 0) {
> +            return;
> +        }
> +    }
> +}
Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Namhyung Kim

2016-Aug-26 04:48 UTC

head link

[PATCH 2/3] qemu: Implement virtio-pstore device

Hi Daniel,

On Wed, Aug 24, 2016 at 06:00:51PM -0400, Daniel P. Berrange
wrote:> 
> > diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> > new file mode 100644
> > index 0000000..b8fb4be
> > --- /dev/null
> > +++ b/hw/virtio/virtio-pstore.c
> > @@ -0,0 +1,699 @@
> > +/*
> > + * Virtio Pstore Device
> > + *
> > + * Copyright (C) 2016  LG Electronics
> > + *
> > + * Authors:
> > + *  Namhyung Kim  <namhyung at gmail.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/iov.h"
> > +#include "qemu-common.h"
> > +#include "qemu/cutils.h"
> > +#include "qemu/error-report.h"
> > +#include "sysemu/kvm.h"
> > +#include "qapi/visitor.h"
> > +#include "qapi-event.h"
> > +#include "io/channel-util.h"
> > +#include "trace.h"
> > +
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-bus.h"
> > +#include "hw/virtio/virtio-access.h"
> > +#include "hw/virtio/virtio-pstore.h"
> > +
> > +#define PSTORE_DEFAULT_BUFSIZE   (16 * 1024)
> > +#define PSTORE_DEFAULT_FILE_MAX  5
> > +
> > +/* the index should match to the type value */
> > +static const char *virtio_pstore_file_prefix[] = {
> > +    "unknown-",		/* VIRTIO_PSTORE_TYPE_UNKNOWN */
> > +    "dmesg-",		/* VIRTIO_PSTORE_TYPE_DMESG */
> > +};
> > +
> > +static char *virtio_pstore_to_filename(VirtIOPstore *s,
> > +                                       struct virtio_pstore_req *req)
> > +{
> > +    const char *basename;
> > +    unsigned long long id;
> > +    unsigned int type = le16_to_cpu(req->type);
> > +    unsigned int flags = le32_to_cpu(req->flags);
> > +
> > +    if (type < ARRAY_SIZE(virtio_pstore_file_prefix)) {
> > +        basename = virtio_pstore_file_prefix[type];
> > +    } else {
> > +        basename = "unknown-";
> > +    }
> > +
> > +    id = s->id++;
> > +    return g_strdup_printf("%s/%s%llu%s", s->directory,
basename, id,
> > +                            flags & VIRTIO_PSTORE_FL_COMPRESSED ?
".enc.z" : "");
> > +}
> > +
> > +static char *virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> > +                                         struct
virtio_pstore_fileinfo *info)
> > +{
> > +    char *filename;
> > +    unsigned int idx;
> > +
> > +    filename = g_strdup_printf("%s/%s", s->directory,
name);
> > +    if (filename == NULL)
> > +        return NULL;
> > +
> > +    for (idx = 0; idx < ARRAY_SIZE(virtio_pstore_file_prefix);
idx++) {
> > +        if (g_str_has_prefix(name, virtio_pstore_file_prefix[idx])) {
> > +            info->type = idx;
> > +            name += strlen(virtio_pstore_file_prefix[idx]);
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (idx == ARRAY_SIZE(virtio_pstore_file_prefix)) {
> > +        g_free(filename);
> > +        return NULL;
> > +    }
> > +
> > +    qemu_strtoull(name, NULL, 0, &info->id);
> > +
> > +    info->flags = 0;
> > +    if (g_str_has_suffix(name, ".enc.z")) {
> > +        info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> > +    }
> > +
> > +    return filename;
> > +}
> > +
> > +static int prefix_idx;
> > +static int prefix_count;
> > +static int prefix_len;
> > +
> > +static int filter_pstore(const struct dirent *de)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < prefix_count; i++) {
> > +        const char *prefix = virtio_pstore_file_prefix[prefix_idx +
i];
> > +
> > +        if (g_str_has_prefix(de->d_name, prefix)) {
> > +            return 1;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int sort_pstore(const struct dirent **a, const struct dirent
**b)
> > +{
> > +    uint64_t id_a, id_b;
> > +
> > +    qemu_strtoull((*a)->d_name + prefix_len, NULL, 0, &id_a);
> > +    qemu_strtoull((*b)->d_name + prefix_len, NULL, 0, &id_b);
> > +
> > +    return id_a - id_b;
> > +}
> > +
> > +static int rotate_pstore_file(VirtIOPstore *s, unsigned short type)
> 
> AFAIK you're not actually doing file rotation here - that implies a
> fixed base filename, with .0, .1, .2, etc suffixes where we rename
> files each time. It looks like you are assuming separate filenames,
> and are merely deleting the oldest each time.
Ah, right.  It's not rotation and I think it's enough for my purpose.
I need to change the name.
> 
> > +{
> > +    int ret = 0;
> > +    int i, num;
> > +    char *filename;
> > +    struct dirent **files;
> > +
> > +    if (type >= ARRAY_SIZE(virtio_pstore_file_prefix)) {
> > +        type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> > +    }
> > +
> > +    prefix_idx = type;
> > +    prefix_len = strlen(virtio_pstore_file_prefix[type]);
> > +    prefix_count = 1;  /* only scan current type */
> > +
> > +    /* delete the oldest file in the same type */
> > +    num = scandir(s->directory, &files, filter_pstore,
sort_pstore);
> > +    if (num < 0)
> > +        return num;
> > +    if (num < (int)s->file_max)
> > +        goto out;
> > +
> > +    filename = g_strdup_printf("%s/%s", s->directory,
files[0]->d_name);
> > +    if (filename == NULL) {
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    ret = unlink(filename);
> 
> 
> 
> 
> 
> > +static gboolean pstore_async_read_fn(QIOChannel *ioc, GIOCondition
condition,
> > +                                     gpointer data)
> > +{
> > +    struct pstore_read_arg *rarg = data;
> > +    struct virtio_pstore_fileinfo *info = &rarg->info;
> > +    VirtIOPstore *vps = rarg->vps;
> > +    VirtQueueElement *elem = rarg->elem;
> > +    struct virtio_pstore_res res;
> > +    size_t offset = sizeof(res) + sizeof(*info);
> > +    struct iovec *sg = elem->in_sg;
> > +    unsigned int sg_num = elem->in_num;
> > +    Error *err = NULL;
> > +    ssize_t len;
> > +    int ret;
> > +
> > +    /* skip res and fileinfo */
> > +    iov_discard_front(&sg, &sg_num, sizeof(res) +
sizeof(*info));
> > +
> > +    len = qio_channel_readv(rarg->ioc, sg, sg_num, &err);
> > +    if (len < 0) {
> > +        if (errno == EAGAIN) {
> > +            len = 0;
> > +        }
> > +        ret = -1;
> > +    } else {
> > +        info->len = cpu_to_le32(len);
> > +        ret = 0;
> > +    }
> > +
> > +    res.cmd  = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> > +    res.type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
> > +    res.ret  = cpu_to_le32(ret);
> > +
> > +    /* now copy res and fileinfo */
> > +    iov_from_buf(elem->in_sg, elem->in_num, 0, &res,
sizeof(res));
> > +    iov_from_buf(elem->in_sg, elem->in_num, sizeof(res), info,
sizeof(*info));
> > +
> > +    len += offset;
> > +    virtqueue_push(vps->rvq, elem, len);
> > +    virtio_notify(VIRTIO_DEVICE(vps), vps->rvq);
> > +
> > +    return G_SOURCE_REMOVE;
> 
> G_SOURCE_REMOVE was added in glib 2.32, but QEMU only permits
> stuff that is present in 2.22. Just use "FALSE" instead.
Didn't know that, will change.
> 
> > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s,
VirtQueueElement *elem)
> > +{
> > +    char *filename = NULL;
> > +    int fd, idx;
> > +    struct stat stbuf;
> > +    struct pstore_read_arg *rarg = NULL;
> > +    Error *err = NULL;
> > +    int ret = -1;
> > +
> > +    if (s->file_idx >= s->num_file) {
> > +        return 0;
> > +    }
> > +
> > +    rarg = g_malloc(sizeof(*rarg));
> > +    if (rarg == NULL) {
> > +        return -1;
> > +    }
> > +
> > +    idx = s->file_idx++;
> > +    filename = virtio_pstore_from_filename(s,
s->files[idx]->d_name,
> > +                                           &rarg->info);
> > +    if (filename == NULL) {
> > +        goto out;
> > +    }
> > +
> > +    fd = open(filename, O_RDONLY);
> > +    if (fd < 0) {
> > +        error_report("cannot open %s", filename);
> > +        goto out;
> > +    }
> > +
> > +    if (fstat(fd, &stbuf) < 0) {
> > +        goto out;
> > +    }
> > +
> > +    rarg->vps            = s;
> > +    rarg->elem           = elem;
> > +    rarg->info.id        = cpu_to_le64(rarg->info.id);
> > +    rarg->info.type      = cpu_to_le16(rarg->info.type);
> > +    rarg->info.flags     = cpu_to_le32(rarg->info.flags);
> > +    rarg->info.time_sec  = cpu_to_le64(stbuf.st_ctim.tv_sec);
> > +    rarg->info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> > +
> > +    rarg->ioc = qio_channel_new_fd(fd, &err);
> 
> You should just use qio_channel_open_path() and avoid the earlier
> call to open()
I did it because to call fstat() using the fd and wanted to keep the
generic ioc pointer.

> 
> > +    if (err) {
> > +        error_reportf_err(err, "cannot create io channel:
");
> > +        goto out;
> > +    }
> > +
> > +    qio_channel_set_blocking(rarg->ioc, false, &err);
> > +    qio_channel_add_watch(rarg->ioc, G_IO_IN,
pstore_async_read_fn, rarg,
> > +                          free_rarg_fn);
> > +    g_free(filename);
> > +    return 1;
> > +
> > +out:
> > +    g_free(filename);
> > +    g_free(rarg);
> > +
> > +    return ret;
> > +}
> 
> 
> > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s,
VirtQueueElement *elem,
> > +                                      struct virtio_pstore_req *req)
> > +{
> > +    unsigned short type = le16_to_cpu(req->type);
> > +    char *filename = NULL;
> > +    int fd;
> > +    int flags = O_WRONLY | O_CREAT | O_TRUNC;
> > +    struct pstore_write_arg *warg = NULL;
> > +    Error *err = NULL;
> > +    int ret = -1;
> > +
> > +    /* do not keep same type of files more than 'file-max' */
> > +    rotate_pstore_file(s, type);
> > +
> > +    filename = virtio_pstore_to_filename(s, req);
> > +    if (filename == NULL) {
> > +        return -1;
> > +    }
> > +
> > +    warg = g_malloc(sizeof(*warg));
> > +    if (warg == NULL) {
> > +        goto out;
> > +    }
> > +
> > +    fd = open(filename, flags, 0644);
> > +    if (fd < 0) {
> > +        error_report("cannot open %s", filename);
> > +        ret = fd;
> > +        goto out;
> > +    }
> > +
> > +    warg->vps            = s;
> > +    warg->elem           = elem;
> > +    warg->req            = req;
> > +
> > +    warg->ioc = qio_channel_new_fd(fd, &err);
> 
> Same point about using new_path() instead of new_fd()
OK.
> 
> > +    if (err) {
> > +        error_reportf_err(err, "cannot create io channel:
");
> > +        goto out;
> > +    }
> > +
> > +    qio_channel_set_blocking(warg->ioc, false, &err);
> > +    qio_channel_add_watch(warg->ioc, G_IO_OUT,
pstore_async_write_fn, warg,
> > +                          free_warg_fn);
> > +    g_free(filename);
> > +    return 1;
> > +
> > +out:
> > +    g_free(filename);
> > +    g_free(warg);
> > +    return ret;
> > +}
> > +
> > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue
*vq)
> > +{
> > +    VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > +    VirtQueueElement *elem;
> > +    struct virtio_pstore_req req;
> > +    struct virtio_pstore_res res;
> > +    ssize_t len = 0;
> > +    int ret;
> > +
> > +    for (;;) {
> > +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > +        if (!elem) {
> > +            return;
> > +        }
> > +
> > +        if (elem->out_num < 1 || elem->in_num < 1) {
> > +            error_report("request or response buffer is
missing");
> > +            exit(1);
> > +        }
> > +
> > +        if (elem->out_num > 2 || elem->in_num > 3) {
> > +            error_report("invalid number of input/output
buffer");
> > +            exit(1);
> > +        }
> > +
> > +        len = iov_to_buf(elem->out_sg, elem->out_num, 0,
&req, sizeof(req));
> > +        if (len != (ssize_t)sizeof(req)) {
> > +            error_report("invalid request size: %ld",
(long)len);
> > +            exit(1);
> > +        }
> > +        res.cmd  = req.cmd;
> > +        res.type = req.type;
> > +
> > +        switch (le16_to_cpu(req.cmd)) {
> > +        case VIRTIO_PSTORE_CMD_OPEN:
> > +            ret = virtio_pstore_do_open(s);
> > +            break;
> > +        case VIRTIO_PSTORE_CMD_CLOSE:
> > +            ret = virtio_pstore_do_close(s);
> > +            break;
> > +        case VIRTIO_PSTORE_CMD_ERASE:
> > +            ret = virtio_pstore_do_erase(s, &req);
> > +            break;
> > +        case VIRTIO_PSTORE_CMD_READ:
> > +            ret = virtio_pstore_do_read(s, elem);
> > +            if (ret == 1) {
> > +                /* async channel io */
> > +                continue;
> > +            }
> > +            break;
> > +        case VIRTIO_PSTORE_CMD_WRITE:
> > +            ret = virtio_pstore_do_write(s, elem, &req);
> > +            if (ret == 1) {
> > +                /* async channel io */
> > +                continue;
> > +            }
> > +            break;
> > +        default:
> > +            ret = -1;
> > +            break;
> > +        }
> > +
> > +        res.ret = ret;
> > +
> > +        iov_from_buf(elem->in_sg, elem->in_num, 0, &res,
sizeof(res));
> > +        virtqueue_push(vq, elem, sizeof(res) + len);
> > +
> > +        virtio_notify(vdev, vq);
> > +        g_free(elem);
> > +
> > +        if (ret < 0) {
> > +            return;
> > +        }
> > +    }
> > +}
> 
> Regards,
> Daniel
As always, thanks for your review!

Thanks,
Namhyung

Daniel P. Berrange

2016-Aug-26 12:27 UTC

head link

[PATCH 2/3] qemu: Implement virtio-pstore device

On Fri, Aug 26, 2016 at 01:48:40PM +0900, Namhyung Kim
wrote:> Hi Daniel,
> 
> On Wed, Aug 24, 2016 at 06:00:51PM -0400, Daniel P. Berrange wrote:
> > > +    fd = open(filename, O_RDONLY);
> > > +    if (fd < 0) {
> > > +        error_report("cannot open %s", filename);
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (fstat(fd, &stbuf) < 0) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    rarg->vps            = s;
> > > +    rarg->elem           = elem;
> > > +    rarg->info.id        = cpu_to_le64(rarg->info.id);
> > > +    rarg->info.type      = cpu_to_le16(rarg->info.type);
> > > +    rarg->info.flags     = cpu_to_le32(rarg->info.flags);
> > > +    rarg->info.time_sec  = cpu_to_le64(stbuf.st_ctim.tv_sec);
> > > +    rarg->info.time_nsec =
cpu_to_le32(stbuf.st_ctim.tv_nsec);
> > > +
> > > +    rarg->ioc = qio_channel_new_fd(fd, &err);
> > 
> > You should just use qio_channel_open_path() and avoid the earlier
> > call to open()
> 
> I did it because to call fstat() using the fd and wanted to keep the
> generic ioc pointer.
I'd suggest just using a cast inline, eg

  fstat(QIO_CHANNEL_FILE(ioc)->fd, &stbuf)


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Possibly Parallel Threads

Search for more reasonably related threads

Linux Virtualization - Aug 2016 - [PATCH 2/3] qemu: Implement virtio-pstore device

[PATCH 2/3] qemu: Implement virtio-pstore device

[PATCH 2/3] qemu: Implement virtio-pstore device

[PATCH 2/3] qemu: Implement virtio-pstore device

Possibly Parallel Threads