From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Daniel P. Berrange" Subject: Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device Date: Mon, 1 Aug 2016 10:24:39 +0100 Message-ID: <20160801092439.GF6455@redhat.com> References: <1469632111-23260-1-git-send-email-namhyung@kernel.org> <1469632111-23260-7-git-send-email-namhyung@kernel.org> <20160728132239.GM22677@redhat.com> <20160730085702.GB26275@danjae.aot.lge.com> Reply-To: "Daniel P. Berrange" Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20160730085702.GB26275@danjae.aot.lge.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Namhyung Kim Cc: Tony Luck , Kees Cook , kvm@vger.kernel.org, Radim Kr??m???? , LKML , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Steven Rostedt , virtualization@lists.linux-foundation.org, Minchan Kim , Anton Vorontsov , Anthony Liguori , Colin Cross , Paolo Bonzini , Ingo Molnar List-Id: virtualization@lists.linuxfoundation.org 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 :|