Re: [Qemu-devel] [PATCH] 9pfs: include for XATTR_SIZE_MAX

[Patrick Steinhardt]

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 . On glibc-based systems, this header is
> indirectly included via , ,
> , 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  indirectly,
> which leads to `XATTR_SIZE_MAX` being undefined.
> 
> Fix this error by directly include . 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 
> ---
>  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 
> +#include 
>  #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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] 9pfs: include for XATTR_SIZE_MAX

[Kamil Rytarowski]

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  wrote:
>>> On 29.07.2017 21:34, Peter Maydell wrote:
 On 29 July 2017 at 14:50, Patrick Steinhardt  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 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
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: include 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  wrote:
> > > On 29.07.2017 21:34, Peter Maydell wrote:
> > >> On 29 July 2017 at 14:50, Patrick Steinhardt  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 for XATTR_SIZE_MAX

[Peter Maydell]

On 31 July 2017 at 15:23, Fam Zheng  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 for XATTR_SIZE_MAX

[Fam Zheng]

On Sun, 07/30 19:23, Peter Maydell wrote:
> On 30 July 2017 at 17:51, Kamil Rytarowski  wrote:
> > On 29.07.2017 21:34, Peter Maydell wrote:
> >> On 29 July 2017 at 14:50, Patrick Steinhardt  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 for XATTR_SIZE_MAX

[Kamil Rytarowski]

On 30.07.2017 20:23, Peter Maydell wrote:
> On 30 July 2017 at 17:51, Kamil Rytarowski  wrote:
>> On 29.07.2017 21:34, Peter Maydell wrote:
>>> On 29 July 2017 at 14:50, Patrick Steinhardt  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
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: include for XATTR_SIZE_MAX

[Peter Maydell]

On 30 July 2017 at 17:51, Kamil Rytarowski  wrote:
> On 29.07.2017 21:34, Peter Maydell wrote:
>> On 29 July 2017 at 14:50, Patrick Steinhardt  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 for XATTR_SIZE_MAX

[Kamil Rytarowski]

On 29.07.2017 21:34, Peter Maydell wrote:
> On 29 July 2017 at 14:50, Patrick Steinhardt  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?



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: include for XATTR_SIZE_MAX

[Peter Maydell]

On 29 July 2017 at 14:50, Patrick Steinhardt  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  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 for XATTR_SIZE_MAX

[Patrick Steinhardt]

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  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 . On glibc-based systems, this header is
> >> indirectly included via , ,
> >> , 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  indirectly,
> >> which leads to `XATTR_SIZE_MAX` being undefined.
> >>
> >> Fix this error by directly include . 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 
> > Reviewed-by: Alistair Francis 
> >> ---
> >>   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 
> 
> 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 
> 
> #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  is included without any ifdef
around. As such, I simply assumed that this code is being built
on Linux systems, only.

Regards
Patrick


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] 9pfs: include for XATTR_SIZE_MAX

[Philippe Mathieu-Daudé]

On Mon, Jun 26, 2017 at 12:20 PM, 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 . On glibc-based systems, this header is
indirectly included via , ,
, 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  indirectly,
which leads to `XATTR_SIZE_MAX` being undefined.

Fix this error by directly include . 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 

Reviewed-by: Alistair Francis 

---
  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 


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 


#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 for XATTR_SIZE_MAX

[Alistair Francis]

On Mon, Jun 26, 2017 at 12:20 PM, 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 . On glibc-based systems, this header is
> indirectly included via , ,
> , 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  indirectly,
> which leads to `XATTR_SIZE_MAX` being undefined.
>
> Fix this error by directly include . 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 

Ping!

Reviewed-by: Alistair Francis 

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 
> +#include 
>  #include "hw/virtio/virtio.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> --
> 2.13.2
>
>

[Qemu-devel] [PATCH] 9pfs: include 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 . On glibc-based systems, this header is
indirectly included via , ,
, 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  indirectly,
which leads to `XATTR_SIZE_MAX` being undefined.

Fix this error by directly include . 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 
---
 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 
+#include 
 #include "hw/virtio/virtio.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
-- 
2.13.2