Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

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 :|