Daniel P. Berrange
2016-Jul-28 13:22 UTC
[Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:> Add virtio pstore device to allow kernel log files saved on the host. > It will save the log files on the directory given by pstore device > option. > > $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ... > > (guest) # echo c > /proc/sysrq-trigger > > $ ls dir-xx > dmesg-1.enc.z dmesg-2.enc.z > > The log files are usually compressed using zlib. Users can see the log > messages directly on the host or on the guest (using pstore filesystem). > > The 'directory' property is required for virtio-pstore device to work. > It also adds 'bufsize' and 'console' (boolean) properties. > > Cc: Paolo Bonzini <pbonzini at redhat.com> > Cc: Radim Kr?m?? <rkrcmar at redhat.com> > Cc: "Michael S. Tsirkin" <mst at redhat.com> > Cc: Anthony Liguori <aliguori at amazon.com> > Cc: Anton Vorontsov <anton at enomsg.org> > Cc: Colin Cross <ccross at android.com> > Cc: Kees Cook <keescook at chromium.org> > Cc: Tony Luck <tony.luck at intel.com> > Cc: Steven Rostedt <rostedt at goodmis.org> > Cc: Ingo Molnar <mingo at kernel.org> > Cc: Minchan Kim <minchan at kernel.org> > Cc: kvm at vger.kernel.org > Cc: qemu-devel at nongnu.org > Cc: virtualization at lists.linux-foundation.org > Signed-off-by: Namhyung Kim <namhyung at kernel.org> > --- > hw/virtio/Makefile.objs | 2 +- > hw/virtio/virtio-pci.c | 54 +++ > hw/virtio/virtio-pci.h | 14 + > hw/virtio/virtio-pstore.c | 477 +++++++++++++++++++++++++ > include/hw/pci/pci.h | 1 + > include/hw/virtio/virtio-pstore.h | 34 ++ > include/standard-headers/linux/virtio_ids.h | 1 + > include/standard-headers/linux/virtio_pstore.h | 80 +++++ > qdev-monitor.c | 1 + > 9 files changed, 663 insertions(+), 1 deletion(-) > create mode 100644 hw/virtio/virtio-pstore.c > create mode 100644 include/hw/virtio/virtio-pstore.h > create mode 100644 include/standard-headers/linux/virtio_pstore.h > > diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs > index 3e2b175..aae7082 100644 > --- a/hw/virtio/Makefile.objs > +++ b/hw/virtio/Makefile.objs > @@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o > common-obj-y += virtio-mmio.o > > obj-y += virtio.o virtio-balloon.o > -obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o > +obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index f0677b7..d99a405 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2414,6 +2414,59 @@ static const TypeInfo virtio_host_pci_info = { > }; > #endif > > +/* virtio-pstore-pci */ > + > +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > +{ > + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev); > + DeviceState *vdev = DEVICE(&vps->vdev); > + Error *err = NULL; > + > + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > + object_property_set_bool(OBJECT(vdev), true, "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > +} > + > +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass); > + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass); > + > + k->realize = virtio_pstore_pci_realize; > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > + > + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET; > + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE; > + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION; > + pcidev_k->class_id = PCI_CLASS_OTHERS; > +} > + > +static void virtio_pstore_pci_instance_init(Object *obj) > +{ > + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj); > + > + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev), > + TYPE_VIRTIO_PSTORE); > + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev), > + "directory", &error_abort); > + object_property_add_alias(obj, "bufsize", OBJECT(&dev->vdev), > + "bufsize", &error_abort); > + object_property_add_alias(obj, "console", OBJECT(&dev->vdev), > + "console", &error_abort); > +} > + > +static const TypeInfo virtio_pstore_pci_info = { > + .name = TYPE_VIRTIO_PSTORE_PCI, > + .parent = TYPE_VIRTIO_PCI, > + .instance_size = sizeof(VirtIOPstorePCI), > + .instance_init = virtio_pstore_pci_instance_init, > + .class_init = virtio_pstore_pci_class_init, > +}; > + > /* virtio-pci-bus */ > > static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, > @@ -2483,6 +2536,7 @@ static void virtio_pci_register_types(void) > #ifdef CONFIG_VHOST_SCSI > type_register_static(&vhost_scsi_pci_info); > #endif > + type_register_static(&virtio_pstore_pci_info); > } > > type_init(virtio_pci_register_types) > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index e4548c2..b4c039f 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -31,6 +31,7 @@ > #ifdef CONFIG_VHOST_SCSI > #include "hw/virtio/vhost-scsi.h" > #endif > +#include "hw/virtio/virtio-pstore.h" > > typedef struct VirtIOPCIProxy VirtIOPCIProxy; > typedef struct VirtIOBlkPCI VirtIOBlkPCI; > @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI; > typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI; > typedef struct VirtIOInputHostPCI VirtIOInputHostPCI; > typedef struct VirtIOGPUPCI VirtIOGPUPCI; > +typedef struct VirtIOPstorePCI VirtIOPstorePCI; > > /* virtio-pci-bus */ > > @@ -311,6 +313,18 @@ struct VirtIOGPUPCI { > VirtIOGPU vdev; > }; > > +/* > + * virtio-pstore-pci: This extends VirtioPCIProxy. > + */ > +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci" > +#define VIRTIO_PSTORE_PCI(obj) \ > + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI) > + > +struct VirtIOPstorePCI { > + VirtIOPCIProxy parent_obj; > + VirtIOPstore vdev; > +}; > + > /* Virtio ABI version, if we increment this, we break the guest driver. */ > #define VIRTIO_PCI_ABI_VERSION 0 > > diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c > new file mode 100644 > index 0000000..2ca7786 > --- /dev/null > +++ b/hw/virtio/virtio-pstore.c > @@ -0,0 +1,477 @@ > +/* > + * 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. 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 "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" > + > + > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz, > + struct virtio_pstore_req *req) > +{ > + const char *basename; > + unsigned long long id = 0; > + unsigned int flags = le32_to_cpu(req->flags); > + > + switch (le16_to_cpu(req->type)) { > + case VIRTIO_PSTORE_TYPE_DMESG: > + basename = "dmesg"; > + id = s->id++; > + break; > + case VIRTIO_PSTORE_TYPE_CONSOLE: > + basename = "console"; > + if (s->console_id) { > + id = s->console_id; > + } else { > + id = s->console_id = s->id++; > + } > + break; > + default: > + basename = "unknown"; > + break; > + } > + > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, > + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");Please use g_strdup_printf() instead of splattering into a pre-allocated buffer than may or may not be large enough.> +} > + > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > + char *buf, size_t sz, > + struct virtio_pstore_fileinfo *info) > +{ > + snprintf(buf, sz, "%s/%s", s->directory, name); > + > + if (g_str_has_prefix(name, "dmesg-")) { > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > + name += strlen("dmesg-"); > + } else if (g_str_has_prefix(name, "console-")) { > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > + name += strlen("console-"); > + } else if (g_str_has_prefix(name, "unknown-")) { > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > + name += strlen("unknown-"); > + } > + > + qemu_strtoull(name, NULL, 0, &info->id); > + > + info->flags = 0; > + if (g_str_has_suffix(name, ".enc.z")) { > + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > + } > +} > + > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) > +{ > + s->dirp = opendir(s->directory); > + if (s->dirp == NULL) { > + return -1; > + } > + > + return 0; > +} > + > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg, > + unsigned int in_num, > + struct virtio_pstore_res *res) > +{ > + char path[PATH_MAX];Don't declare PATH_MAX sized variables> + int fd; > + ssize_t len; > + struct stat stbuf; > + struct dirent *dent; > + int sg_num = in_num; > + struct iovec sg[sg_num];'sg_num' is initialized from 'in_num' which comes from the guest, and I'm not seeing anything which is bounds-checking the 'in_num' value. So you've possibly got a security flaw here I think, if the guest can cause QEMU to allocate arbitrary stack memory & thus overflow by setting arbitrarily large in_num.> + struct virtio_pstore_fileinfo info; > + size_t offset = sizeof(*res) + sizeof(info); > + > + if (s->dirp == NULL) { > + return -1; > + } > + > + dent = readdir(s->dirp); > + while (dent) { > + if (dent->d_name[0] != '.') { > + break; > + } > + dent = readdir(s->dirp); > + } > + > + if (dent == NULL) { > + return 0; > + }So this seems to just be picking the first filename reported by readdir that isn't starting with '.'. Surely this can't the right logic when your corresponding do_write method can pick several different filenames, its potluck which do_read will give back.> + > + /* skip res and fileinfo */ > + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset, > + iov_size(in_sg, in_num) - offset); > + > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info); > + fd = open(path, O_RDONLY); > + if (fd < 0) { > + error_report("cannot open %s", path); > + return -1; > + } > + > + if (fstat(fd, &stbuf) < 0) { > + len = -1; > + goto out; > + } > + > + len = readv(fd, sg, sg_num); > + if (len < 0) { > + if (errno == EAGAIN) { > + len = 0; > + } > + goto out; > + } > + > + info.id = cpu_to_le64(info.id); > + info.type = cpu_to_le16(info.type); > + info.flags = cpu_to_le32(info.flags); > + info.len = cpu_to_le32(len); > + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); > + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); > + > + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info)); > + len += sizeof(info); > + > + out: > + close(fd); > + return len; > +} > + > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > + unsigned int out_num, > + struct virtio_pstore_req *req) > +{ > + char path[PATH_MAX]; > + int fd; > + ssize_t len; > + unsigned short type; > + int flags = O_WRONLY | O_CREAT; > + > + /* we already consume the req */ > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > + > + virtio_pstore_to_filename(s, path, sizeof(path), req); > + > + type = le16_to_cpu(req->type); > + > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > + flags |= O_TRUNC; > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > + flags |= O_APPEND;Using O_APPEND will cause the file to grow without bound on the host, which is highly undesirable, aka a security flaw.> + } > + > + fd = open(path, flags, 0644); > + if (fd < 0) { > + error_report("cannot open %s", path); > + return -1; > + } > + len = writev(fd, out_sg, out_num); > + close(fd); > + > + return len; > +}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-Jul-30 08:57 UTC
[Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote:> On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > Add virtio pstore device to allow kernel log files saved on the host. > > It will save the log files on the directory given by pstore device > > option. > > > > $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ... > > > > (guest) # echo c > /proc/sysrq-trigger > > > > $ ls dir-xx > > dmesg-1.enc.z dmesg-2.enc.z > > > > The log files are usually compressed using zlib. Users can see the log > > messages directly on the host or on the guest (using pstore filesystem). > > > > The 'directory' property is required for virtio-pstore device to work. > > It also adds 'bufsize' and 'console' (boolean) properties. > > > > Cc: Paolo Bonzini <pbonzini at redhat.com> > > Cc: Radim Kr??m???? <rkrcmar at redhat.com> > > Cc: "Michael S. Tsirkin" <mst at redhat.com> > > Cc: Anthony Liguori <aliguori at amazon.com> > > Cc: Anton Vorontsov <anton at enomsg.org> > > Cc: Colin Cross <ccross at android.com> > > Cc: Kees Cook <keescook at chromium.org> > > Cc: Tony Luck <tony.luck at intel.com> > > Cc: Steven Rostedt <rostedt at goodmis.org> > > Cc: Ingo Molnar <mingo at kernel.org> > > Cc: Minchan Kim <minchan at kernel.org> > > Cc: kvm at vger.kernel.org > > Cc: qemu-devel at nongnu.org > > Cc: virtualization at lists.linux-foundation.org > > Signed-off-by: Namhyung Kim <namhyung at kernel.org> > > ---[SNIP]> > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz, > > + struct virtio_pstore_req *req) > > +{ > > + const char *basename; > > + unsigned long long id = 0; > > + unsigned int flags = le32_to_cpu(req->flags); > > + > > + switch (le16_to_cpu(req->type)) { > > + case VIRTIO_PSTORE_TYPE_DMESG: > > + basename = "dmesg"; > > + id = s->id++; > > + break; > > + case VIRTIO_PSTORE_TYPE_CONSOLE: > > + basename = "console"; > > + if (s->console_id) { > > + id = s->console_id; > > + } else { > > + id = s->console_id = s->id++; > > + } > > + break; > > + default: > > + basename = "unknown"; > > + break; > > + } > > + > > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, > > + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : ""); > > Please use g_strdup_printf() instead of splattering into a pre-allocated > buffer than may or may not be large enough.OK.> > > +} > > + > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > > + char *buf, size_t sz, > > + struct virtio_pstore_fileinfo *info) > > +{ > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > + > > + if (g_str_has_prefix(name, "dmesg-")) { > > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > > + name += strlen("dmesg-"); > > + } else if (g_str_has_prefix(name, "console-")) { > > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > > + name += strlen("console-"); > > + } else if (g_str_has_prefix(name, "unknown-")) { > > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > > + name += strlen("unknown-"); > > + } > > + > > + qemu_strtoull(name, NULL, 0, &info->id); > > + > > + info->flags = 0; > > + if (g_str_has_suffix(name, ".enc.z")) { > > + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > > + } > > +} > > + > > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) > > +{ > > + s->dirp = opendir(s->directory); > > + if (s->dirp == NULL) { > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg, > > + unsigned int in_num, > > + struct virtio_pstore_res *res) > > +{ > > + char path[PATH_MAX]; > > Don't declare PATH_MAX sized variablesWill change to use g_strdup_printf() as you said.> > > + int fd; > > + ssize_t len; > > + struct stat stbuf; > > + struct dirent *dent; > > + int sg_num = in_num; > > + struct iovec sg[sg_num]; > > 'sg_num' is initialized from 'in_num' which comes from the > guest, and I'm not seeing anything which is bounds-checking > the 'in_num' value. So you've possibly got a security flaw here > I think, if the guest can cause QEMU to allocate arbitrary stack > memory & thus overflow by setting arbitrarily large in_num.Will add a bound check then.> > > + struct virtio_pstore_fileinfo info; > > + size_t offset = sizeof(*res) + sizeof(info); > > + > > + if (s->dirp == NULL) { > > + return -1; > > + } > > + > > + dent = readdir(s->dirp); > > + while (dent) { > > + if (dent->d_name[0] != '.') { > > + break; > > + } > > + dent = readdir(s->dirp); > > + } > > + > > + if (dent == NULL) { > > + return 0; > > + } > > So this seems to just be picking the first filename reported by > readdir that isn't starting with '.'. Surely this can't the right > logic when your corresponding do_write method can pick several > different filenames, its potluck which do_read will give back.Do you mean that it'd be better to check a list of known filenames and fail if not?> > > + > > + /* skip res and fileinfo */ > > + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset, > > + iov_size(in_sg, in_num) - offset); > > + > > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info); > > + fd = open(path, O_RDONLY); > > + if (fd < 0) { > > + error_report("cannot open %s", path); > > + return -1; > > + } > > + > > + if (fstat(fd, &stbuf) < 0) { > > + len = -1; > > + goto out; > > + } > > + > > + len = readv(fd, sg, sg_num); > > + if (len < 0) { > > + if (errno == EAGAIN) { > > + len = 0; > > + } > > + goto out; > > + } > > + > > + info.id = cpu_to_le64(info.id); > > + info.type = cpu_to_le16(info.type); > > + info.flags = cpu_to_le32(info.flags); > > + info.len = cpu_to_le32(len); > > + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); > > + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); > > + > > + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info)); > > + len += sizeof(info); > > + > > + out: > > + close(fd); > > + return len; > > +} > > + > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > + unsigned int out_num, > > + struct virtio_pstore_req *req) > > +{ > > + char path[PATH_MAX]; > > + int fd; > > + ssize_t len; > > + unsigned short type; > > + int flags = O_WRONLY | O_CREAT; > > + > > + /* we already consume the req */ > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > + > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > + > > + type = le16_to_cpu(req->type); > > + > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > + flags |= O_TRUNC; > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > + flags |= O_APPEND; > > Using O_APPEND will cause the file to grow without bound on the > host, which is highly undesirable, aka a security flaw.Right. The plan is to add size checking and to split if it exceeds some limit. And we can keep some number of recent files only. Thanks, Namhyung> > > + } > > + > > + fd = open(path, flags, 0644); > > + if (fd < 0) { > > + error_report("cannot open %s", path); > > + return -1; > > + } > > + len = writev(fd, out_sg, out_num); > > + close(fd); > > + > > + return len; > > +} > > > 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 :|
Daniel P. Berrange
2016-Aug-01 09:24 UTC
[Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
On Sat, Jul 30, 2016 at 05:57:02PM +0900, Namhyung Kim wrote:> On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote: > > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > > > + char *buf, size_t sz, > > > + struct virtio_pstore_fileinfo *info) > > > +{ > > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > > + > > > + if (g_str_has_prefix(name, "dmesg-")) { > > > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > > > + name += strlen("dmesg-"); > > > + } else if (g_str_has_prefix(name, "console-")) { > > > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > > > + name += strlen("console-"); > > > + } else if (g_str_has_prefix(name, "unknown-")) { > > > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > > > + name += strlen("unknown-"); > > > + }[snip]> > > + struct virtio_pstore_fileinfo info; > > > + size_t offset = sizeof(*res) + sizeof(info); > > > + > > > + if (s->dirp == NULL) { > > > + return -1; > > > + } > > > + > > > + dent = readdir(s->dirp); > > > + while (dent) { > > > + if (dent->d_name[0] != '.') { > > > + break; > > > + } > > > + dent = readdir(s->dirp); > > > + } > > > + > > > + if (dent == NULL) { > > > + return 0; > > > + } > > > > So this seems to just be picking the first filename reported by > > readdir that isn't starting with '.'. Surely this can't the right > > logic when your corresponding do_write method can pick several > > different filenames, its potluck which do_read will give back. > > Do you mean that it'd be better to check a list of known filenames and > fail if not?No, I mean that you have several different VIRTIO_PSTORE_TYPE_nnn and use a different file for each constant. When reading this directory though you're not looking for the file corresponding to any given VIRTIO_PSTORE_TYPE_nnn - you're simply reading whichever file is found first. So you might have just read a TYPE_CONSOLE or have read a TYPE_DMESG - it surely doesn't make sense to randonly read either TYPE_CONSOLE or TYPE_DMESG based on whatever order readdir() lists the files. 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 :|
Seemingly Similar Threads
- [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
- [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
- [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
- [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device
- [PATCH 6/7] qemu: Implement virtio-pstore device