[Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Patrick Steinhardt]

The function `v9fs_xattrcreate` makes use of the define `XATTR_SIZE_MAX`
to reject attempts of creating xattrs with an invalid size, which is
defined in <linux/limits.h>. On glibc-based systems, this header is
indirectly included via <limits.h>, <bits/posix1_lim.h>,
<bitts/local_lim.h>, but on other platforms this is not guaranteed due
to not being part of the POSIX standard. One examples are systems based
on musl libc, which do not include the <linux/limits.h> indirectly,
which leads to `XATTR_SIZE_MAX` being undefined.

Fix this error by directly include <linux/limits.h>. As the 9P fs code
is being Linux-based either way, we can simply do so without breaking
other platforms. This enables building 9pfs on musl-based systems.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 hw/9pfs/9p.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 96d2683348..48cd558e96 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include <glib/gprintf.h>
+#include <linux/limits.h>
 #include "hw/virtio/virtio.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
-- 
2.13.2

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Alistair Francis]

On Mon, Jun 26, 2017 at 12:20 PM, Patrick Steinhardt <ps@pks.im> wrote:
> The function `v9fs_xattrcreate` makes use of the define `XATTR_SIZE_MAX`
> to reject attempts of creating xattrs with an invalid size, which is
> defined in <linux/limits.h>. On glibc-based systems, this header is
> indirectly included via <limits.h>, <bits/posix1_lim.h>,
> <bitts/local_lim.h>, but on other platforms this is not guaranteed due
> to not being part of the POSIX standard. One examples are systems based
> on musl libc, which do not include the <linux/limits.h> indirectly,
> which leads to `XATTR_SIZE_MAX` being undefined.
>
> Fix this error by directly include <linux/limits.h>. As the 9P fs code
> is being Linux-based either way, we can simply do so without breaking
> other platforms. This enables building 9pfs on musl-based systems.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Ping!

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair


> ---
>  hw/9pfs/9p.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 96d2683348..48cd558e96 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -13,6 +13,7 @@
>
>  #include "qemu/osdep.h"
>  #include <glib/gprintf.h>
> +#include <linux/limits.h>
>  #include "hw/virtio/virtio.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> --
> 2.13.2
>
>

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Philippe Mathieu-Daudé]

> On Mon, Jun 26, 2017 at 12:20 PM, Patrick Steinhardt <ps@pks.im> wrote:
>> The function `v9fs_xattrcreate` makes use of the define `XATTR_SIZE_MAX`
>> to reject attempts of creating xattrs with an invalid size, which is
>> defined in <linux/limits.h>. On glibc-based systems, this header is
>> indirectly included via <limits.h>, <bits/posix1_lim.h>,
>> <bitts/local_lim.h>, but on other platforms this is not guaranteed due
>> to not being part of the POSIX standard. One examples are systems based
>> on musl libc, which do not include the <linux/limits.h> indirectly,
>> which leads to `XATTR_SIZE_MAX` being undefined.
>>
>> Fix this error by directly include <linux/limits.h>. As the 9P fs code
>> is being Linux-based either way, we can simply do so without breaking
>> other platforms. This enables building 9pfs on musl-based systems.
>>
>> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>   hw/9pfs/9p.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 96d2683348..48cd558e96 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -13,6 +13,7 @@
>>
>>   #include "qemu/osdep.h"
>>   #include <glib/gprintf.h>

This is likely to break on BSD, but now than patchew has a NetBSD job 
you can trigger a build RESENDing this patch.

This should probably work:

#ifdef __linux__

>> +#include <linux/limits.h>

#endif

>>   #include "hw/virtio/virtio.h"
>>   #include "qapi/error.h"
>>   #include "qemu/error-report.h"
>> --
>> 2.13.2

Regards,

Phil.

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
> > On Mon, Jun 26, 2017 at 12:20 PM, Patrick Steinhardt <ps@pks.im> wrote:
> >> The function `v9fs_xattrcreate` makes use of the define `XATTR_SIZE_MAX`
> >> to reject attempts of creating xattrs with an invalid size, which is
> >> defined in <linux/limits.h>. On glibc-based systems, this header is
> >> indirectly included via <limits.h>, <bits/posix1_lim.h>,
> >> <bitts/local_lim.h>, but on other platforms this is not guaranteed due
> >> to not being part of the POSIX standard. One examples are systems based
> >> on musl libc, which do not include the <linux/limits.h> indirectly,
> >> which leads to `XATTR_SIZE_MAX` being undefined.
> >>
> >> Fix this error by directly include <linux/limits.h>. As the 9P fs code
> >> is being Linux-based either way, we can simply do so without breaking
> >> other platforms. This enables building 9pfs on musl-based systems.
> >>
> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> >> ---
> >>   hw/9pfs/9p.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> >> index 96d2683348..48cd558e96 100644
> >> --- a/hw/9pfs/9p.c
> >> +++ b/hw/9pfs/9p.c
> >> @@ -13,6 +13,7 @@
> >>
> >>   #include "qemu/osdep.h"
> >>   #include <glib/gprintf.h>
> 
> This is likely to break on BSD, but now than patchew has a NetBSD job 
> you can trigger a build RESENDing this patch.
> 
> This should probably work:
> 
> #ifdef __linux__
> 
> >> +#include <linux/limits.h>
> 
> #endif
> 
> >>   #include "hw/virtio/virtio.h"
> >>   #include "qapi/error.h"
> >>   #include "qemu/error-report.h"
> >> --
> >> 2.13.2
> 
> Regards,
> 
> Phil.

Thanks for the feedback! Is this really relevant in this context,
though? Both 9p-local.c and 9p-handle.c already include linux
headers, especially <linux/fs.h> is included without any ifdef
around. As such, I simply assumed that this code is being built
on Linux systems, only.

Regards
Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Peter Maydell]

On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
>> This is likely to break on BSD, but now than patchew has a NetBSD job
>> you can trigger a build RESENDing this patch.

> Thanks for the feedback! Is this really relevant in this context,
> though? Both 9p-local.c and 9p-handle.c already include linux
> headers, especially <linux/fs.h> is included without any ifdef
> around. As such, I simply assumed that this code is being built
> on Linux systems, only.

Yes; hw/Makefile.objs only includes the 9pfs directory
if CONFIG_VIRTFS is defined, and configure only sets that
on Linux hosts which have libpcap and libattr available.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Kamil Rytarowski]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On 29.07.2017 21:34, Peter Maydell wrote:
> On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
>> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
>>> This is likely to break on BSD, but now than patchew has a NetBSD job
>>> you can trigger a build RESENDing this patch.
> 

I just checked patchew, and there is FreeBSD job. How far are we from
adding more BSDs?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Peter Maydell]

On 30 July 2017 at 17:51, Kamil Rytarowski <n54@gmx.com> wrote:
> On 29.07.2017 21:34, Peter Maydell wrote:
>> On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
>>> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
>>>> This is likely to break on BSD, but now than patchew has a NetBSD job
>>>> you can trigger a build RESENDing this patch.
>>
>
> I just checked patchew, and there is FreeBSD job. How far are we from
> adding more BSDs?

I now test OpenBSD and NetBSD as well in my pre-merge
test setup. Patchew could add them as well if desired.
(vm setup instructions at http://wiki.qemu.org/Hosts/BSD)

(I haven't bothered to send a patch marking OpenBSD
as 'supported' since we've had zero contact from
anybody in the OpenBSD community AFAIK.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Kamil Rytarowski]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On 30.07.2017 20:23, Peter Maydell wrote:
> On 30 July 2017 at 17:51, Kamil Rytarowski <n54@gmx.com> wrote:
>> On 29.07.2017 21:34, Peter Maydell wrote:
>>> On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
>>>> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
>>>>> This is likely to break on BSD, but now than patchew has a NetBSD job
>>>>> you can trigger a build RESENDing this patch.
>>>
>>
>> I just checked patchew, and there is FreeBSD job. How far are we from
>> adding more BSDs?
> 
> I now test OpenBSD and NetBSD as well in my pre-merge
> test setup. Patchew could add them as well if desired.
> (vm setup instructions at http://wiki.qemu.org/Hosts/BSD)
> 

Please do.

> (I haven't bothered to send a patch marking OpenBSD
> as 'supported' since we've had zero contact from
> anybody in the OpenBSD community AFAIK.)
> 

There is one maintainer in OpenBSD ports Brad Smith, but he's not an
OpenBSD developer as far as I can tell.

Adding him to CC.

> thanks
> -- PMM
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Fam Zheng]

On Sun, 07/30 19:23, Peter Maydell wrote:
> On 30 July 2017 at 17:51, Kamil Rytarowski <n54@gmx.com> wrote:
> > On 29.07.2017 21:34, Peter Maydell wrote:
> >> On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
> >>> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
> >>>> This is likely to break on BSD, but now than patchew has a NetBSD job
> >>>> you can trigger a build RESENDing this patch.
> >>
> >
> > I just checked patchew, and there is FreeBSD job. How far are we from
> > adding more BSDs?
> 
> I now test OpenBSD and NetBSD as well in my pre-merge
> test setup. Patchew could add them as well if desired.
> (vm setup instructions at http://wiki.qemu.org/Hosts/BSD)

No objection to adding more BSDs to patchew as long as I can find a few more
gigabytes RAM to run the VM (BTW I'm also thinking about converting long running
VMs to boot/shutdown on demand, to support more types of guests). But still want
to ask this: how likely it is for a patch to compile on one BSD flavor but fail
on the other?

Fam

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Peter Maydell]

On 31 July 2017 at 15:23, Fam Zheng <famz@redhat.com> wrote:
> No objection to adding more BSDs to patchew as long as I can find a few more
> gigabytes RAM to run the VM (BTW I'm also thinking about converting long running
> VMs to boot/shutdown on demand, to support more types of guests). But still want
> to ask this: how likely it is for a patch to compile on one BSD flavor but fail
> on the other?

I dunno about any particular patch, but when I came to trying
to get the BSDs into my test machine set I found that the
three different OSes were all failing for different reasons.
The bulk of "BSD fails" probably fail on all 3 though, so
you could probably get away with only testing 1 in patchew
and letting my tests catch the rest.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Daniel P. Berrange]

On Mon, Jul 31, 2017 at 10:23:08PM +0800, Fam Zheng wrote:
> On Sun, 07/30 19:23, Peter Maydell wrote:
> > On 30 July 2017 at 17:51, Kamil Rytarowski <n54@gmx.com> wrote:
> > > On 29.07.2017 21:34, Peter Maydell wrote:
> > >> On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
> > >>> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
> > >>>> This is likely to break on BSD, but now than patchew has a NetBSD job
> > >>>> you can trigger a build RESENDing this patch.
> > >>
> > >
> > > I just checked patchew, and there is FreeBSD job. How far are we from
> > > adding more BSDs?
> > 
> > I now test OpenBSD and NetBSD as well in my pre-merge
> > test setup. Patchew could add them as well if desired.
> > (vm setup instructions at http://wiki.qemu.org/Hosts/BSD)
> 
> No objection to adding more BSDs to patchew as long as I can find a few more
> gigabytes RAM to run the VM (BTW I'm also thinking about converting long running
> VMs to boot/shutdown on demand, to support more types of guests). But still want
> to ask this: how likely it is for a patch to compile on one BSD flavor but fail
> on the other?

While they share common ancestry, they are largely independant projects,
so each has its own quirks & potentially differing features. IOW it isn't
like Linux distros, where there's a common kernel & userspace in every
distros, and you're largely just dealing with software version differences.
So if you have the resources, I think it'd be worth running patchew across
the different BSDs that QEMU claims support for. I'd far rather see the
failures upfront, than when Peter tries to merge my pull request.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Kamil Rytarowski]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

On 31.07.2017 16:23, Fam Zheng wrote:
> On Sun, 07/30 19:23, Peter Maydell wrote:
>> On 30 July 2017 at 17:51, Kamil Rytarowski <n54@gmx.com> wrote:
>>> On 29.07.2017 21:34, Peter Maydell wrote:
>>>> On 29 July 2017 at 14:50, Patrick Steinhardt <ps@pks.im> wrote:
>>>>> On Fri, Jul 28, 2017 at 02:20:49PM -0300, Philippe Mathieu-Daudé wrote:
>>>>>> This is likely to break on BSD, but now than patchew has a NetBSD job
>>>>>> you can trigger a build RESENDing this patch.
>>>>
>>>
>>> I just checked patchew, and there is FreeBSD job. How far are we from
>>> adding more BSDs?
>>
>> I now test OpenBSD and NetBSD as well in my pre-merge
>> test setup. Patchew could add them as well if desired.
>> (vm setup instructions at http://wiki.qemu.org/Hosts/BSD)
> 
> No objection to adding more BSDs to patchew as long as I can find a few more
> gigabytes RAM to run the VM (BTW I'm also thinking about converting long running
> VMs to boot/shutdown on demand, to support more types of guests). But still want
> to ask this: how likely it is for a patch to compile on one BSD flavor but fail
> on the other?
> 

High probability.

These systems (FreeBSD, NetBSD, OpenBSD) diverged over 20 years ago and
are developed by different teams and for different use-cases.

We should assume that these systems are completely different POSIX-like
systems, with shared ancestors (AT&T UNIX -> BSD UNIX).

Thought, DragonflyBSD & FreeBSD are relatively similar, FreeBSD fixes
should work for DragonflyBSD in most cases.

Right now there are no other modern BSDs with a vibrant community,
everything else is retro-computing, remix with preconfigured desktop,
hobby, 1-man-show etc.

> Fam
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: include <linux/limits.h> for XATTR_SIZE_MAX

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Mon, Jun 26, 2017 at 09:20:45PM +0200, Patrick Steinhardt wrote:
> The function `v9fs_xattrcreate` makes use of the define `XATTR_SIZE_MAX`
> to reject attempts of creating xattrs with an invalid size, which is
> defined in <linux/limits.h>. On glibc-based systems, this header is
> indirectly included via <limits.h>, <bits/posix1_lim.h>,
> <bitts/local_lim.h>, but on other platforms this is not guaranteed due
> to not being part of the POSIX standard. One examples are systems based
> on musl libc, which do not include the <linux/limits.h> indirectly,
> which leads to `XATTR_SIZE_MAX` being undefined.
> 
> Fix this error by directly include <linux/limits.h>. As the 9P fs code
> is being Linux-based either way, we can simply do so without breaking
> other platforms. This enables building 9pfs on musl-based systems.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  hw/9pfs/9p.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 96d2683348..48cd558e96 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  #include <glib/gprintf.h>
> +#include <linux/limits.h>
>  #include "hw/virtio/virtio.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> -- 
> 2.13.2
> 

Another ping on this patch. I think in the end all agreed this
patch is sane, but the thread has been derailed by the
patchew-discussion. So any further comments regarding this patch
or can it be submitted as is?

Regards
Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]