| Message ID | 07010a2ad79559c412949f0005dbe3cb03d8416e.1498504812.git.ps@pks.im |
|---|---|
| State | New |
| Headers | show |
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 > >
> 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.
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
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
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?
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
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 >
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
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
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
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 >
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
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"
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(+)