[v1] usb-mtp: Add fallback definition of NAME_MAX

[Qemu-devel] [PATCH] usb-mtp: Add fallback definition of NAME_MAX

Posted by Kamil Rytarowski 8 years, 2 months ago

This fixes build on SmartOS (Joyent).

Patch cherry-picked from pkgsrc by jperkin (Joyent).

Signed-off-by: Kamil Rytarowski <n54@gmx.com>
---
 hw/usb/dev-mtp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 94c2e94f10..6e8d7b21b5 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -26,6 +26,10 @@
 #include "hw/usb.h"
 #include "hw/usb/desc.h"
 
+#ifndef NAME_MAX
+#define NAME_MAX 255
+#endif
+
 /* ----------------------------------------------------------------------- */
 
 enum mtp_container_type {
-- 
2.14.1

Re: [Qemu-devel] [PATCH] usb-mtp: Add fallback definition of NAME_MAX

Posted by Philippe Mathieu-Daudé 8 years, 2 months ago

Hi Kamil,

On 09/03/2017 01:34 PM, Kamil Rytarowski wrote:
> This fixes build on SmartOS (Joyent).
> 
> Patch cherry-picked from pkgsrc by jperkin (Joyent).
> 
> Signed-off-by: Kamil Rytarowski <n54@gmx.com>
> ---
>   hw/usb/dev-mtp.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 94c2e94f10..6e8d7b21b5 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -26,6 +26,10 @@
>   #include "hw/usb.h"
>   #include "hw/usb/desc.h"
>   
> +#ifndef NAME_MAX
> +#define NAME_MAX 255
> +#endif

this belongs to include/qemu/osdep.h

once moved there:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +
>   /* ----------------------------------------------------------------------- */
>   
>   enum mtp_container_type {
> 

Regards,

Phil.

[Qemu-devel] [PATCH v2] usb-mtp: Add fallback definition of NAME_MAX

Posted by Kamil Rytarowski 8 years, 2 months ago

This fixes build on SmartOS (Joyent).

Patch cherry-picked from pkgsrc by jperkin (Joyent).

Signed-off-by: Kamil Rytarowski <n54@gmx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/osdep.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 6855b94bbf..5d3860f80e 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -306,6 +306,11 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 #endif
 #endif
 
+/* Required by SmartOS (SunOS) */
+#ifndef NAME_MAX
+#define NAME_MAX 255
+#endif
+
 #if defined(__linux__) && \
     (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
    /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
-- 
2.14.1

Re: [Qemu-devel] [PATCH v2] usb-mtp: Add fallback definition of NAME_MAX

Posted by Peter Maydell 8 years, 2 months ago

On 4 September 2017 at 18:25, Kamil Rytarowski <n54@gmx.com> wrote:
> This fixes build on SmartOS (Joyent).
>
> Patch cherry-picked from pkgsrc by jperkin (Joyent).
>
> Signed-off-by: Kamil Rytarowski <n54@gmx.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/osdep.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6855b94bbf..5d3860f80e 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -306,6 +306,11 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #endif
>  #endif
>
> +/* Required by SmartOS (SunOS) */
> +#ifndef NAME_MAX
> +#define NAME_MAX 255
> +#endif

So in hw/usb/dev-mtp.c we're using NAME_MAX in
    char buf[sizeof(struct inotify_event) + NAME_MAX + 1];

because the Linux implementation of inotify documents in inotify(7)
that this is guaranteed to be sufficient to read at least one event:
  http://man7.org/linux/man-pages/man7/inotify.7.html
Looking at the SmartOS manpage
  https://smartos.org/man/5/inotify
there doesn't seem to be any equivalent language.

What is the SmartOS requirement on the buffer size to be
guaranteed to read at least one complete event ?
Defining NAME_MAX to 255 seems like it shuts up the compiler
error but does it give us the correct behaviour?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] usb-mtp: Add fallback definition of NAME_MAX

Posted by Kamil Rytarowski 8 years, 2 months ago

On 04.09.2017 19:50, Peter Maydell wrote:
> On 4 September 2017 at 18:25, Kamil Rytarowski <n54@gmx.com> wrote:
>> This fixes build on SmartOS (Joyent).
>>
>> Patch cherry-picked from pkgsrc by jperkin (Joyent).
>>
>> Signed-off-by: Kamil Rytarowski <n54@gmx.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/qemu/osdep.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 6855b94bbf..5d3860f80e 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -306,6 +306,11 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>>  #endif
>>  #endif
>>
>> +/* Required by SmartOS (SunOS) */
>> +#ifndef NAME_MAX
>> +#define NAME_MAX 255
>> +#endif
> 
> So in hw/usb/dev-mtp.c we're using NAME_MAX in
>     char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
> 
> because the Linux implementation of inotify documents in inotify(7)
> that this is guaranteed to be sufficient to read at least one event:
>   http://man7.org/linux/man-pages/man7/inotify.7.html
> Looking at the SmartOS manpage
>   https://smartos.org/man/5/inotify
> there doesn't seem to be any equivalent language.
> 
> What is the SmartOS requirement on the buffer size to be
> guaranteed to read at least one complete event ?
> Defining NAME_MAX to 255 seems like it shuts up the compiler
> error but does it give us the correct behaviour?
> 
According to my understanding SmartOS has the same logic. It tries to
read at least one element, and NAME_MAX guarantees that it will be
available.

https://github.com/joyent/illumos-joyent/blob/master/usr/src/uts/common/io/inotify.c#L1193

I've gathered some overall details about the patches.

1. The inotify linux-compat interface is only SmartOS specific, it
hasn't been upstreamed so far to other Illumos distributions.

2. Upstream Illumos implemented NAME_MAX
https://github.com/illumos/illumos-gate/commit/9c0752ac0dc05794d2f8a8b4521d55e2b3f63247

It's not available in SmartOS.

3. Proprietary Solaris is out of scope of this work.

Looking at their userland we can assume no qemu support:

https://github.com/oracle/solaris-userland

https://github.com/oracle/solaris-userland/commit/9c78c7b45a5d3dbd64afb455d278d2ca277e2b95#diff-94df904ebca88ee0ff038eaedb063ad6R61

++        if platform.system() == 'SunOS':
++            # No QEMU support on Solaris now.
++            qemu_img = False

We can stop pretending to support it now.

4. SmartOS forked qemu for its hypervisor part and uses qemu-kvm with
the Linux kvm interface (yes, there is Linux kernel kvm port to SmartOS)
- upstreaming the hypervisor's fork is out of scope.

https://github.com/joyent/illumos-kvm-cmd

5. SmartOS ships with virtual machine guests, they host pkgsrc and qemu
from pkgsrc.

6. Jonathan mentioned that preparing a SmartOS tutorial and image will
be tricky, as SmartOS is a hypervisor. I will focus on upstreaming
SmartOS-guest support for qemu, where there is pkgsrc. There is an
option to prepare another Illumos distribution tutorial and testbot,
hopefully it will be fully compatible with SmartOS patches.

> thanks
> -- PMM
>