Re: [Qemu-devel] [PATCH 1/2] virtio-9p: make virtio-9p available to all POSIX systems

[Blue Swirl]

On Mon, May 24, 2010 at 8:46 PM, Venkateswararao Jujjuri (JV)
<address@hidden> wrote:
> Blue Swirl wrote:
>> Field d_off in struct dirent is Linux specific.
>>
>> Signed-off-by: Blue Swirl <address@hidden>
>> ---
>>  Makefile.objs   |    8 ++++----
>>  Makefile.target |    2 +-
>>  hw/virtio-9p.c  |    2 +-
>>  hw/virtio-pci.c |    6 +++---
>>  hw/virtio.h     |    4 ++--
>>  qemu-config.c   |    4 ++--
>>  qemu-config.h   |    2 +-
>>  qemu-options.hx |    8 ++++----
>>  vl.c            |    8 ++++----
>>  9 files changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 1585101..b1a6e01 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -35,8 +35,8 @@ net-nested-$(CONFIG_SLIRP) += slirp.o
>>  net-nested-$(CONFIG_VDE) += vde.o
>>  net-obj-y += $(addprefix net/, $(net-nested-y))
>>
>> -fsdev-nested-$(CONFIG_LINUX) = qemu-fsdev.o
>> -fsdev-obj-$(CONFIG_LINUX) += $(addprefix fsdev/, $(fsdev-nested-y))
>> +fsdev-nested-$(CONFIG_POSIX) = qemu-fsdev.o
>> +fsdev-obj-$(CONFIG_POSIX) += $(addprefix fsdev/, $(fsdev-nested-y))
>>
>>  ######################################################################
>>  # libqemu_common.a: Target independent part of system emulation. The
>> @@ -47,7 +47,7 @@ fsdev-obj-$(CONFIG_LINUX) += $(addprefix fsdev/,
>> $(fsdev-nested-y))
>>  common-obj-y = $(block-obj-y)
>>  common-obj-y += $(net-obj-y)
>>  common-obj-y += $(qobject-obj-y)
>> -common-obj-$(CONFIG_LINUX) += $(fsdev-obj-$(CONFIG_LINUX))
>> +common-obj-$(CONFIG_POSIX) += $(fsdev-obj-$(CONFIG_POSIX))
>>  common-obj-y += readline.o console.o async.o qemu-error.o
>>  common-obj-y += tcg-runtime.o host-utils.o
>>  common-obj-y += irq.o ioport.o input.o
>> @@ -229,7 +229,7 @@ sound-obj-$(CONFIG_CS4231A) += cs4231a.o
>>  adlib.o fmopl.o: QEMU_CFLAGS += -DBUILD_Y8950=0
>>  hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
>>
>> -hw-obj-$(CONFIG_LINUX) += virtio-9p-debug.o virtio-9p-local.o
>> +hw-obj-$(CONFIG_POSIX) += virtio-9p-debug.o virtio-9p-local.o
>>
>>  ######################################################################
>>  # libdis
>> diff --git a/Makefile.target b/Makefile.target
>> index fda5bf3..00e140f 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -168,7 +168,7 @@ obj-y += virtio-blk.o virtio-balloon.o
>> virtio-net.o virtio-serial-bus.o
>>  obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>>  obj-y += vhost_net.o
>>  obj-$(CONFIG_VHOST_NET) += vhost.o
>> -obj-$(CONFIG_LINUX) += virtio-9p.o
>> +obj-$(CONFIG_POSIX) += virtio-9p.o
>>  obj-y += rwhandler.o
>>  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>>  obj-$(CONFIG_NO_KVM) += kvm-stub.o
>> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
>> index e5d0112..68b0696 100644
>> --- a/hw/virtio-9p.c
>> +++ b/hw/virtio-9p.c
>> @@ -1447,8 +1447,8 @@ static void v9fs_read_post_dir_lstat(V9fsState
>> *s, V9fsReadState *vs,
>>      vs->count += vs->len;
>>      v9fs_stat_free(&vs->v9stat);
>>      v9fs_string_free(&vs->name);
>> -    vs->dir_pos = vs->dent->d_off;
>>      vs->dent = v9fs_do_readdir(s, vs->fidp->dir);
>> +    vs->dir_pos = v9fs_do_telldir(s, vs->fidp->dir);
>
>
> We need to save the the current dir position before making next readdir
> We need to seek back if we can't fit it into PDU.
> Hence moving the dir_pos after readdir is not a good idea.

Hmm, the manual page for readdir says:
       On Linux, the dirent structure is defined as follows:

           struct dirent {
               ino_t          d_ino;       /* inode number */
               off_t          d_off;       /* offset to the next dirent */
               unsigned short d_reclen;    /* length of this record */
               unsigned char  d_type;      /* type of file */
               char           d_name[256]; /* filename */
           };

My change was based on the comment for d_off.

But when I run this program:

$ cat dirent.c
#include <sys/types.h>
#include <dirent.h>
#include <stdio.h>

int main(int argc, const char **argv)
{
    DIR *d;
    struct dirent *entry;
    off_t pos;

    d = opendir(argv[1]);
    entry = readdir(d);
    do {
        pos = telldir(d);
        printf("name %s d_off %ld ino %ld pos %ld\n", entry->d_name,
entry->d_off, entry->d_ino, pos);
        entry = readdir(d);
    } while (entry);
    closedir(d);

    return 0;
}

d_off is equal to telldir value:

$ ./dirent /
name tmp d_off 206002973 ino 56 pos 206002973
name root d_off 224116791 ino 521217 pos 224116791
name vmlinuz.old d_off 255549115 ino 17 pos 255549115
name dev d_off 378658993 ino 374625 pos 378658993

I'll send an updated patch.

> BTW, Thanks for making VirtFS generic to all POSIX systems.
>
> Thanks,
> JV.
>
>>      v9fs_read_post_readdir(s, vs, err);
>>      return;
>>  out:
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 7ddf612..0a74781 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -102,7 +102,7 @@ typedef struct {
>>      BlockConf block;
>>      NICConf nic;
>>      uint32_t host_features;
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>      V9fsConf fsconf;
>>  #endif
>>      /* Max. number of ports we can have for a the virtio-serial device */
>> @@ -642,7 +642,7 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>>      return 0;
>>  }
>>
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  static int virtio_9p_init_pci(PCIDevice *pci_dev)
>>  {
>>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>> @@ -713,7 +713,7 @@ static PCIDeviceInfo virtio_info[] = {
>>          },
>>          .qdev.reset = virtio_pci_reset,
>>      },{
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>          .qdev.name = "virtio-9p-pci",
>>          .qdev.size = sizeof(VirtIOPCIProxy),
>>          .init      = virtio_9p_init_pci,
>> diff --git a/hw/virtio.h b/hw/virtio.h
>> index e4306cd..e77af13 100644
>> --- a/hw/virtio.h
>> +++ b/hw/virtio.h
>> @@ -20,7 +20,7 @@
>>  #include "sysemu.h"
>>  #include "block_int.h"
>>  #include "event_notifier.h"
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  #include "9p.h"
>>  #endif
>>
>> @@ -188,7 +188,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev,
>> BlockConf *conf);
>>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf);
>>  VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
>>  VirtIODevice *virtio_balloon_init(DeviceState *dev);
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf);
>>  #endif
>>
>> diff --git a/qemu-config.c b/qemu-config.c
>> index d500885..78e80e3 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -151,7 +151,7 @@ QemuOptsList qemu_chardev_opts = {
>>      },
>>  };
>>
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  QemuOptsList qemu_fsdev_opts = {
>>      .name = "fsdev",
>>      .implied_opt_name = "fstype",
>> @@ -169,7 +169,7 @@ QemuOptsList qemu_fsdev_opts = {
>>  };
>>  #endif
>>
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  QemuOptsList qemu_virtfs_opts = {
>>      .name = "virtfs",
>>      .implied_opt_name = "fstype",
>> diff --git a/qemu-config.h b/qemu-config.h
>> index dca69d4..5376935 100644
>> --- a/qemu-config.h
>> +++ b/qemu-config.h
>> @@ -3,7 +3,7 @@
>>
>>  extern QemuOptsList qemu_drive_opts;
>>  extern QemuOptsList qemu_chardev_opts;
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  extern QemuOptsList qemu_fsdev_opts;
>>  extern QemuOptsList qemu_virtfs_opts;
>>  #endif
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 03e95fd..34ed806 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -475,7 +475,7 @@ possible drivers and properties, use @code{-device ?} and
>> address@hidden @var{driver},?}.
>>  ETEXI
>>
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  DEFHEADING(File system options:)
>>
>>  DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
>> @@ -499,7 +499,7 @@ Options to each backend are described below.
>>
>>  Create a file-system-"device" for local-filesystem.
>>
>> address@hidden is only available on Linux.
>> address@hidden is only available on POSIX systems.
>>
>> address@hidden specifies the path to be exported. @option{path} is required.
>>
>> @@ -507,7 +507,7 @@ Create a file-system-"device" for local-filesystem.
>>  ETEXI
>>  #endif
>>
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  DEFHEADING(Virtual File system pass-through options:)
>>
>>  DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
>> @@ -531,7 +531,7 @@ Options to each backend are described below.
>>
>>  Create a Virtual file-system-pass through for local-filesystem.
>>
>> address@hidden is only available on Linux.
>> address@hidden is only available on POSIX systems.
>>
>> address@hidden specifies the path to be exported. @option{path} is required.
>>
>> diff --git a/vl.c b/vl.c
>> index d77b47c..d5c1e34 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -149,7 +149,7 @@ int main(int argc, char **argv)
>>  #include "qemu-option.h"
>>  #include "qemu-config.h"
>>  #include "qemu-objects.h"
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  #include "fsdev/qemu-fsdev.h"
>>  #endif
>>
>> @@ -2314,7 +2314,7 @@ static int chardev_init_func(QemuOpts *opts, void 
>> *opaque)
>>      return 0;
>>  }
>>
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>  static int fsdev_init_func(QemuOpts *opts, void *opaque)
>>  {
>>      int ret;
>> @@ -3090,7 +3090,7 @@ int main(int argc, char **argv, char **envp)
>>                      exit(1);
>>                  }
>>                  break;
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>              case QEMU_OPTION_fsdev:
>>                  opts = qemu_opts_parse(&qemu_fsdev_opts, optarg, 1);
>>                  if (!opts) {
>> @@ -3513,7 +3513,7 @@ int main(int argc, char **argv, char **envp)
>>
>>      if (qemu_opts_foreach(&qemu_chardev_opts, chardev_init_func, NULL, 1) 
>> != 0)
>>          exit(1);
>> -#ifdef CONFIG_LINUX
>> +#ifdef CONFIG_POSIX
>>      if (qemu_opts_foreach(&qemu_fsdev_opts, fsdev_init_func, NULL, 1) != 0) 
>> {
>>          exit(1);
>>      }
>
>
>