[Qemu-devel] [PATCH for-2.5 0/5] ivshmem: Last minute changes

[Qemu-devel] [PATCH for-2.5 0/5] ivshmem: Last minute changes

[Markus Armbruster]

Straightforward fixes [PATCH 1-4] and a property rename to prepare for
fixing of design flaws in the next development cycle [PATCH 5].

Markus Armbruster (5):
  qemu-doc: Fix ivshmem example markup
  qemu-doc: Fix ivshmem usage example with shm=...
  tests/ivshmem-test: Supply missing initializer in get_device()
  ivshmem: Mark questionable socket type test FIXME
  ivshmem: Rename property memdev to x-memdev for 2.5

 hw/misc/ivshmem.c    | 9 ++++-----
 qemu-doc.texi        | 8 ++++----
 tests/ivshmem-test.c | 3 ++-
 3 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 1/5] qemu-doc: Fix ivshmem example markup

[Markus Armbruster]

Use @var{foo} like we do everywhere else, not <foo>.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-doc.texi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 460ab71..c9b7069 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1256,7 +1256,7 @@ zero-copy communication to the application level of the guests.  The basic
 syntax is:
 
 @example
-qemu-system-i386 -device ivshmem,size=<size in format accepted by -m>[,shm=<shm name>]
+qemu-system-i386 -device ivshmem,size=@var{size}[,shm=@var{shm-name}]
 @end example
 
 If desired, interrupts can be sent between guest VMs accessing the same shared
@@ -1267,12 +1267,12 @@ memory server is:
 
 @example
 # First start the ivshmem server once and for all
-ivshmem-server -p <pidfile> -S <path> -m <shm name> -l <shm size> -n <vectors n>
+ivshmem-server -p @var{pidfile} -S @var{path} -m @var{shm-name} -l @var{shm-size} -n @var{vectors}
 
 # Then start your qemu instances with matching arguments
-qemu-system-i386 -device ivshmem,size=<shm size>,vectors=<vectors n>,chardev=<id>
+qemu-system-i386 -device ivshmem,size=@var{shm-size},vectors=@var{vectors},chardev=@var{id}
                  [,msi=on][,ioeventfd=on][,role=peer|master]
-                 -chardev socket,path=<path>,id=<id>
+                 -chardev socket,path=@var{path},id=@var{id}
 @end example
 
 When using the server, the guest will be assigned a VM ID (>=0) that allows guests
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 2/5] qemu-doc: Fix ivshmem usage example with shm=...

[Markus Armbruster]

The example suggests you can omit "shm".  This isn't true; you must
specify exactly one of "shm", "chardev", "memdev".  Fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-doc.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index c9b7069..68ca075 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1256,7 +1256,7 @@ zero-copy communication to the application level of the guests.  The basic
 syntax is:
 
 @example
-qemu-system-i386 -device ivshmem,size=@var{size}[,shm=@var{shm-name}]
+qemu-system-i386 -device ivshmem,size=@var{size},shm=@var{shm-name}
 @end example
 
 If desired, interrupts can be sent between guest VMs accessing the same shared
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 3/5] tests/ivshmem-test: Supply missing initializer in get_device()

[Markus Armbruster]

If the device isn't found, the assertion uses dev without
initialization.  Fix that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/ivshmem-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index f1793ba..8f1a849 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -40,6 +40,7 @@ static QPCIDevice *get_device(void)
     QPCIBus *pcibus;
 
     pcibus = qpci_init_pc();
+    dev = NULL;
     qpci_device_foreach(pcibus, 0x1af4, 0x1110, save_fn, &dev);
     g_assert(dev != NULL);
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 83d7bd3..a6bfdde 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
         memory_region_add_subregion(&s->bar, 0, mr);
         pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
     } else if (s->server_chr != NULL) {
+        /* FIXME replace this test, it works basically by chance */
         if (strncmp(s->server_chr->filename, "unix:", 5)) {
             error_setg(errp, "chardev is not a unix client socket");
             return;
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 5/5] ivshmem: Rename property memdev to x-memdev for 2.5

[Markus Armbruster]

The device's guest interface and its QEMU user interface are
flawed^Whotly debated.  We'll resolve that in the next development
cycle, probably by deprecating the device in favour of a cleaned up,
but not quite compatible revision.

To avoid adding more baggage to the soon-to-be-deprecated interface,
mark property "memdev" as experimental, by renaming it to "x-memdev".
It's the only recent user interface change.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem.c    | 8 +++-----
 tests/ivshmem-test.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a6bfdde..e24f9ee 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -60,8 +60,6 @@
 #define IVSHMEM(obj) \
     OBJECT_CHECK(IVShmemState, (obj), TYPE_IVSHMEM)
 
-#define IVSHMEM_MEMDEV_PROP "memdev"
-
 typedef struct Peer {
     int nb_eventfds;
     EventNotifier *eventfds;
@@ -857,8 +855,8 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
         PCI_BASE_ADDRESS_MEM_PREFETCH;
 
     if (!!s->server_chr + !!s->shmobj + !!s->hostmem != 1) {
-        error_setg(errp, "You must specify either a shmobj, a chardev"
-                   " or a hostmem");
+        error_setg(errp,
+                   "You must specify either 'shm', 'chardev' or 'x-memdev'");
         return;
     }
 
@@ -1182,7 +1180,7 @@ static void ivshmem_init(Object *obj)
 {
     IVShmemState *s = IVSHMEM(obj);
 
-    object_property_add_link(obj, IVSHMEM_MEMDEV_PROP, TYPE_MEMORY_BACKEND,
+    object_property_add_link(obj, "x-memdev", TYPE_MEMORY_BACKEND,
                              (Object **)&s->hostmem,
                              ivshmem_check_memdev_is_busy,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 8f1a849..03c7b96 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -393,7 +393,7 @@ static void test_ivshmem_memdev(void)
 
     /* just for the sake of checking memory-backend property */
     setup_vm_cmd(&state, "-object memory-backend-ram,size=1M,id=mb1"
-                 " -device ivshmem,memdev=mb1", false);
+                 " -device ivshmem,x-memdev=mb1", false);
 
     qtest_quit(state.qtest);
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 1/5] qemu-doc: Fix ivshmem example markup

[Marc-André Lureau]

----- Original Message -----
> Use @var{foo} like we do everywhere else, not <foo>.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  qemu-doc.texi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 460ab71..c9b7069 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -1256,7 +1256,7 @@ zero-copy communication to the application level of the
> guests.  The basic
>  syntax is:
>  
>  @example
> -qemu-system-i386 -device ivshmem,size=<size in format accepted by
> -m>[,shm=<shm name>]
> +qemu-system-i386 -device ivshmem,size=@var{size}[,shm=@var{shm-name}]
>  @end example
>  
>  If desired, interrupts can be sent between guest VMs accessing the same
>  shared
> @@ -1267,12 +1267,12 @@ memory server is:
>  
>  @example
>  # First start the ivshmem server once and for all
> -ivshmem-server -p <pidfile> -S <path> -m <shm name> -l <shm size> -n
> <vectors n>
> +ivshmem-server -p @var{pidfile} -S @var{path} -m @var{shm-name} -l
> @var{shm-size} -n @var{vectors}
>  
>  # Then start your qemu instances with matching arguments
> -qemu-system-i386 -device ivshmem,size=<shm size>,vectors=<vectors
> n>,chardev=<id>
> +qemu-system-i386 -device
> ivshmem,size=@var{shm-size},vectors=@var{vectors},chardev=@var{id}
>                   [,msi=on][,ioeventfd=on][,role=peer|master]
> -                 -chardev socket,path=<path>,id=<id>
> +                 -chardev socket,path=@var{path},id=@var{id}
>  @end example
>  
>  When using the server, the guest will be assigned a VM ID (>=0) that allows
>  guests
> --
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.5 2/5] qemu-doc: Fix ivshmem usage example with shm=...

[Marc-André Lureau]

----- Original Message -----
> The example suggests you can omit "shm".  This isn't true; you must
> specify exactly one of "shm", "chardev", "memdev".  Fix it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  qemu-doc.texi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index c9b7069..68ca075 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -1256,7 +1256,7 @@ zero-copy communication to the application level of the
> guests.  The basic
>  syntax is:
>  
>  @example
> -qemu-system-i386 -device ivshmem,size=@var{size}[,shm=@var{shm-name}]
> +qemu-system-i386 -device ivshmem,size=@var{size},shm=@var{shm-name}
>  @end example
>  
>  If desired, interrupts can be sent between guest VMs accessing the same
>  shared
> --
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.5 3/5] tests/ivshmem-test: Supply missing initializer in get_device()

[Marc-André Lureau]

----- Original Message -----
> If the device isn't found, the assertion uses dev without
> initialization.  Fix that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

my bad :)

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  tests/ivshmem-test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index f1793ba..8f1a849 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -40,6 +40,7 @@ static QPCIDevice *get_device(void)
>      QPCIBus *pcibus;
>  
>      pcibus = qpci_init_pc();
> +    dev = NULL;
>      qpci_device_foreach(pcibus, 0x1af4, 0x1110, save_fn, &dev);
>      g_assert(dev != NULL);
>  
> --
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Marc-André Lureau]

----- Original Message -----
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/ivshmem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 83d7bd3..a6bfdde 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
> **errp)
>          memory_region_add_subregion(&s->bar, 0, mr);
>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
>      } else if (s->server_chr != NULL) {
> +        /* FIXME replace this test, it works basically by chance */

I wouldn't say it like that, but ack anyway.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>          if (strncmp(s->server_chr->filename, "unix:", 5)) {
>              error_setg(errp, "chardev is not a unix client socket");
>              return;
> --
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.5 5/5] ivshmem: Rename property memdev to x-memdev for 2.5

[Marc-André Lureau]

----- Original Message -----
> The device's guest interface and its QEMU user interface are
> flawed^Whotly debated.  We'll resolve that in the next development
> cycle, probably by deprecating the device in favour of a cleaned up,
> but not quite compatible revision.
> 
> To avoid adding more baggage to the soon-to-be-deprecated interface,
> mark property "memdev" as experimental, by renaming it to "x-memdev".
> It's the only recent user interface change.

Sounds good to me,

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/ivshmem.c    | 8 +++-----
>  tests/ivshmem-test.c | 2 +-
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a6bfdde..e24f9ee 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -60,8 +60,6 @@
>  #define IVSHMEM(obj) \
>      OBJECT_CHECK(IVShmemState, (obj), TYPE_IVSHMEM)
>  
> -#define IVSHMEM_MEMDEV_PROP "memdev"
> -

I agree the define is unneeded, I probably adapted the code from pc-dimm which declare it in public header (not sure why btw)

>  typedef struct Peer {
>      int nb_eventfds;
>      EventNotifier *eventfds;
> @@ -857,8 +855,8 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
> **errp)
>          PCI_BASE_ADDRESS_MEM_PREFETCH;
>  
>      if (!!s->server_chr + !!s->shmobj + !!s->hostmem != 1) {
> -        error_setg(errp, "You must specify either a shmobj, a chardev"
> -                   " or a hostmem");
> +        error_setg(errp,
> +                   "You must specify either 'shm', 'chardev' or
> 'x-memdev'");

nice improvement

>          return;
>      }
>  
> @@ -1182,7 +1180,7 @@ static void ivshmem_init(Object *obj)
>  {
>      IVShmemState *s = IVSHMEM(obj);
>  
> -    object_property_add_link(obj, IVSHMEM_MEMDEV_PROP, TYPE_MEMORY_BACKEND,
> +    object_property_add_link(obj, "x-memdev", TYPE_MEMORY_BACKEND,
>                               (Object **)&s->hostmem,
>                               ivshmem_check_memdev_is_busy,
>                               OBJ_PROP_LINK_UNREF_ON_RELEASE,
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index 8f1a849..03c7b96 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -393,7 +393,7 @@ static void test_ivshmem_memdev(void)
>  
>      /* just for the sake of checking memory-backend property */
>      setup_vm_cmd(&state, "-object memory-backend-ram,size=1M,id=mb1"
> -                 " -device ivshmem,memdev=mb1", false);
> +                 " -device ivshmem,x-memdev=mb1", false);
>  
>      qtest_quit(state.qtest);
>  }
> --
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.5 5/5] ivshmem: Rename property memdev to x-memdev for 2.5

[Markus Armbruster]

Forgot to include the doc update.  Need to squash in:

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 460ab71..45d37e5 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1300,7 +1300,7 @@ a memory backend that has hugepage support:
 
 @example
 qemu-system-i386 -object memory-backend-file,size=1G,mem-path=/mnt/hugepages/my-shmem-file,id=mb1
-                 -device ivshmem,memdev=mb1
+                 -device ivshmem,x-memdev=mb1
 @end example
 
 ivshmem-server also supports hugepages mount points with the

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Markus Armbruster]

Marc-André Lureau <mlureau@redhat.com> writes:

> ----- Original Message -----
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/misc/ivshmem.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 83d7bd3..a6bfdde 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
>> **errp)
>>          memory_region_add_subregion(&s->bar, 0, mr);
>>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
>>      } else if (s->server_chr != NULL) {
>> +        /* FIXME replace this test, it works basically by chance */
>
> I wouldn't say it like that, but ack anyway.

I'm happy to consider a rephrased comment.

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!

>>          if (strncmp(s->server_chr->filename, "unix:", 5)) {
>>              error_setg(errp, "chardev is not a unix client socket");
>>              return;
>> --
>> 2.4.3
>> 
>> 

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <mlureau@redhat.com> writes:
> 
> > ----- Original Message -----
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  hw/misc/ivshmem.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> index 83d7bd3..a6bfdde 100644
> >> --- a/hw/misc/ivshmem.c
> >> +++ b/hw/misc/ivshmem.c
> >> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
> >> **errp)
> >>          memory_region_add_subregion(&s->bar, 0, mr);
> >>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
> >>      } else if (s->server_chr != NULL) {
> >> +        /* FIXME replace this test, it works basically by chance */
> >
> > I wouldn't say it like that, but ack anyway.
> 
> I'm happy to consider a rephrased comment.

Although Paolo used "chance" too, I think the proper comment would be the following:

FIXME: only pty and socket chardevs set chr->filename, other set to the default---the backend name.

ie, to me it's not just by "chance" :)

> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Thanks!
> 
> >>          if (strncmp(s->server_chr->filename, "unix:", 5)) {
> >>              error_setg(errp, "chardev is not a unix client socket");
> >>              return;
> >> --
> >> 2.4.3
> >> 
> >> 
> 

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Markus Armbruster]

Marc-André Lureau <mlureau@redhat.com> writes:

> Hi
>
> ----- Original Message -----
>> Marc-André Lureau <mlureau@redhat.com> writes:
>> 
>> > ----- Original Message -----
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  hw/misc/ivshmem.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >> 
>> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> >> index 83d7bd3..a6bfdde 100644
>> >> --- a/hw/misc/ivshmem.c
>> >> +++ b/hw/misc/ivshmem.c
>> >> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
>> >> **errp)
>> >>          memory_region_add_subregion(&s->bar, 0, mr);
>> >>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
>> >>      } else if (s->server_chr != NULL) {
>> >> +        /* FIXME replace this test, it works basically by chance */
>> >
>> > I wouldn't say it like that, but ack anyway.
>> 
>> I'm happy to consider a rephrased comment.
>
> Although Paolo used "chance" too, I think the proper comment would be
> the following:
>
> FIXME: only pty and socket chardevs set chr->filename, other set to
> the default---the backend name.

I'm afraid that doesn't convey what needs fixing.

> ie, to me it's not just by "chance" :)

Well, it's certainly not by design!

What about:

+        /* FIXME do not rely on what chr drivers put into filename */

[...]

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Marc-André Lureau]

----- Original Message -----
> Marc-André Lureau <mlureau@redhat.com> writes:
> 
> > Hi
> >
> > ----- Original Message -----
> >> Marc-André Lureau <mlureau@redhat.com> writes:
> >> 
> >> > ----- Original Message -----
> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >> ---
> >> >>  hw/misc/ivshmem.c | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >> 
> >> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> >> index 83d7bd3..a6bfdde 100644
> >> >> --- a/hw/misc/ivshmem.c
> >> >> +++ b/hw/misc/ivshmem.c
> >> >> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev,
> >> >> Error
> >> >> **errp)
> >> >>          memory_region_add_subregion(&s->bar, 0, mr);
> >> >>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
> >> >>      } else if (s->server_chr != NULL) {
> >> >> +        /* FIXME replace this test, it works basically by chance */
> >> >
> >> > I wouldn't say it like that, but ack anyway.
> >> 
> >> I'm happy to consider a rephrased comment.
> >
> > Although Paolo used "chance" too, I think the proper comment would be
> > the following:
> >
> > FIXME: only pty and socket chardevs set chr->filename, other set to
> > the default---the backend name.
> 
> I'm afraid that doesn't convey what needs fixing.
> 
> > ie, to me it's not just by "chance" :)
> 
> Well, it's certainly not by design!
> 
> What about:
> 
> +        /* FIXME do not rely on what chr drivers put into filename */

ack

> 
> [...]
> 

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Markus Armbruster]

Marc-André Lureau <mlureau@redhat.com> writes:

> ----- Original Message -----
>> Marc-André Lureau <mlureau@redhat.com> writes:
>> 
>> > Hi
>> >
>> > ----- Original Message -----
>> >> Marc-André Lureau <mlureau@redhat.com> writes:
>> >> 
>> >> > ----- Original Message -----
>> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> >> ---
>> >> >>  hw/misc/ivshmem.c | 1 +
>> >> >>  1 file changed, 1 insertion(+)
>> >> >> 
>> >> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> >> >> index 83d7bd3..a6bfdde 100644
>> >> >> --- a/hw/misc/ivshmem.c
>> >> >> +++ b/hw/misc/ivshmem.c
>> >> >> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev,
>> >> >> Error
>> >> >> **errp)
>> >> >>          memory_region_add_subregion(&s->bar, 0, mr);
>> >> >>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
>> >> >>      } else if (s->server_chr != NULL) {
>> >> >> +        /* FIXME replace this test, it works basically by chance */
>> >> >
>> >> > I wouldn't say it like that, but ack anyway.
>> >> 
>> >> I'm happy to consider a rephrased comment.
>> >
>> > Although Paolo used "chance" too, I think the proper comment would be
>> > the following:
>> >
>> > FIXME: only pty and socket chardevs set chr->filename, other set to
>> > the default---the backend name.
>> 
>> I'm afraid that doesn't convey what needs fixing.
>> 
>> > ie, to me it's not just by "chance" :)
>> 
>> Well, it's certainly not by design!
>> 
>> What about:
>> 
>> +        /* FIXME do not rely on what chr drivers put into filename */
>
> ack

Will you prepare a pull request, or would you like me to do it?

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <mlureau@redhat.com> writes:
> 
> > ----- Original Message -----
> >> Marc-André Lureau <mlureau@redhat.com> writes:
> >> 
> >> > Hi
> >> >
> >> > ----- Original Message -----
> >> >> Marc-André Lureau <mlureau@redhat.com> writes:
> >> >> 
> >> >> > ----- Original Message -----
> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >> >> ---
> >> >> >>  hw/misc/ivshmem.c | 1 +
> >> >> >>  1 file changed, 1 insertion(+)
> >> >> >> 
> >> >> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> >> >> index 83d7bd3..a6bfdde 100644
> >> >> >> --- a/hw/misc/ivshmem.c
> >> >> >> +++ b/hw/misc/ivshmem.c
> >> >> >> @@ -939,6 +939,7 @@ static void pci_ivshmem_realize(PCIDevice *dev,
> >> >> >> Error
> >> >> >> **errp)
> >> >> >>          memory_region_add_subregion(&s->bar, 0, mr);
> >> >> >>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
> >> >> >>      } else if (s->server_chr != NULL) {
> >> >> >> +        /* FIXME replace this test, it works basically by chance */
> >> >> >
> >> >> > I wouldn't say it like that, but ack anyway.
> >> >> 
> >> >> I'm happy to consider a rephrased comment.
> >> >
> >> > Although Paolo used "chance" too, I think the proper comment would be
> >> > the following:
> >> >
> >> > FIXME: only pty and socket chardevs set chr->filename, other set to
> >> > the default---the backend name.
> >> 
> >> I'm afraid that doesn't convey what needs fixing.
> >> 
> >> > ie, to me it's not just by "chance" :)
> >> 
> >> Well, it's certainly not by design!
> >> 
> >> What about:
> >> 
> >> +        /* FIXME do not rely on what chr drivers put into filename */
> >
> > ack
> 
> Will you prepare a pull request, or would you like me to do it?
> 

Please do it (I am not the ivshmem maintainer ;)

Re: [Qemu-devel] [PATCH for-2.5 4/5] ivshmem: Mark questionable socket type test FIXME

[Markus Armbruster]

Marc-André Lureau <mlureau@redhat.com> writes:

>> Will you prepare a pull request, or would you like me to do it?
>> 
>
> Please do it (I am not the ivshmem maintainer ;)

You touched it, you own it!  ;-P

One pull request coming up.