[PATC 7/9] Skipping drm build, unsupported

[PATC 7/9] Skipping drm build, unsupported

[David CARLIER]

From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen@gmail.com>
Date: Mon, 29 Jun 2020 22:20:06 +0000
Subject: [PATCH 7/9] Skipping drm build, unsupported.

Signed-off-by: David Carlier <devnexen@gmail.com>
---
 util/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/Makefile.objs b/util/Makefile.objs
index cc5e37177a..faebc13fac 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -39,7 +39,7 @@ util-obj-y += qsp.o
 util-obj-y += range.o
 util-obj-y += stats64.o
 util-obj-y += systemd.o
-util-obj-$(CONFIG_POSIX) += drm.o
+util-obj-$(CONFIG_LINUX) += drm.o
 util-obj-y += guest-random.o
 util-obj-$(CONFIG_GIO) += dbus.o
 dbus.o-cflags = $(GIO_CFLAGS)
--
2.26.0

Re: [PATC 7/9] Skipping drm build, unsupported

[Philippe Mathieu-Daudé]

+Gerd

On 6/29/20 11:48 PM, David CARLIER wrote:
> From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
> From: David Carlier <devnexen@gmail.com>
> Date: Mon, 29 Jun 2020 22:20:06 +0000
> Subject: [PATCH 7/9] Skipping drm build, unsupported.
> 
> Signed-off-by: David Carlier <devnexen@gmail.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  util/Makefile.objs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index cc5e37177a..faebc13fac 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -39,7 +39,7 @@ util-obj-y += qsp.o
>  util-obj-y += range.o
>  util-obj-y += stats64.o
>  util-obj-y += systemd.o
> -util-obj-$(CONFIG_POSIX) += drm.o
> +util-obj-$(CONFIG_LINUX) += drm.o
>  util-obj-y += guest-random.o
>  util-obj-$(CONFIG_GIO) += dbus.o
>  dbus.o-cflags = $(GIO_CFLAGS)
> --
> 2.26.0
> 

Re: [PATC 7/9] Skipping drm build, unsupported

[Gerd Hoffmann]

On Tue, Jun 30, 2020 at 08:44:24AM +0200, Philippe Mathieu-Daudé wrote:
> +Gerd
> 
> On 6/29/20 11:48 PM, David CARLIER wrote:
> > From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
> > From: David Carlier <devnexen@gmail.com>
> > Date: Mon, 29 Jun 2020 22:20:06 +0000
> > Subject: [PATCH 7/9] Skipping drm build, unsupported.

--verbose please.

> > -util-obj-$(CONFIG_POSIX) += drm.o
> > +util-obj-$(CONFIG_LINUX) += drm.o

Can't see anything linux-specific there.  Also note that FreeBSD (and
possibly other *BSDs too) have drm support.  So CONFIG_POSIX looks
correct to me.

take care,
  Gerd

Re: [PATC 7/9] Skipping drm build, unsupported

[Philippe Mathieu-Daudé]

On 6/30/20 10:23 AM, Gerd Hoffmann wrote:
> On Tue, Jun 30, 2020 at 08:44:24AM +0200, Philippe Mathieu-Daudé wrote:
>> +Gerd
>>
>> On 6/29/20 11:48 PM, David CARLIER wrote:
>>> From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
>>> From: David Carlier <devnexen@gmail.com>
>>> Date: Mon, 29 Jun 2020 22:20:06 +0000
>>> Subject: [PATCH 7/9] Skipping drm build, unsupported.
> 
> --verbose please.

David has difficulties understanding how to send patches,
so you missed the cover/context. This is for the Haiku OS
which apparently is POSIX.1-2001 compatible.

I don't know about DRI-1.0, maybe it is POSIX.1-2008?

>>> -util-obj-$(CONFIG_POSIX) += drm.o
>>> +util-obj-$(CONFIG_LINUX) += drm.o
> 
> Can't see anything linux-specific there.  Also note that FreeBSD (and
> possibly other *BSDs too) have drm support.  So CONFIG_POSIX looks
> correct to me.
> 
> take care,
>   Gerd
> 

Re: [PATC 7/9] Skipping drm build, unsupported

[Gerd Hoffmann]

On Tue, Jun 30, 2020 at 10:46:59AM +0200, Philippe Mathieu-Daudé wrote:
> On 6/30/20 10:23 AM, Gerd Hoffmann wrote:
> > On Tue, Jun 30, 2020 at 08:44:24AM +0200, Philippe Mathieu-Daudé wrote:
> >> +Gerd
> >>
> >> On 6/29/20 11:48 PM, David CARLIER wrote:
> >>> From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
> >>> From: David Carlier <devnexen@gmail.com>
> >>> Date: Mon, 29 Jun 2020 22:20:06 +0000
> >>> Subject: [PATCH 7/9] Skipping drm build, unsupported.
> > 
> > --verbose please.
> 
> David has difficulties understanding how to send patches,
> so you missed the cover/context. This is for the Haiku OS
> which apparently is POSIX.1-2001 compatible.

That doesn't explain why he thinks this patch is needed.
It should build just fine on Haiku ...

take care,
  Gerd

Re: [PATC 7/9] Skipping drm build, unsupported

[Peter Maydell]

On Tue, 30 Jun 2020 at 09:24, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Tue, Jun 30, 2020 at 08:44:24AM +0200, Philippe Mathieu-Daudé wrote:
> > +Gerd
> >
> > On 6/29/20 11:48 PM, David CARLIER wrote:
> > > From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
> > > From: David Carlier <devnexen@gmail.com>
> > > Date: Mon, 29 Jun 2020 22:20:06 +0000
> > > Subject: [PATCH 7/9] Skipping drm build, unsupported.
>
> --verbose please.
>
> > > -util-obj-$(CONFIG_POSIX) += drm.o
> > > +util-obj-$(CONFIG_LINUX) += drm.o
>
> Can't see anything linux-specific there.  Also note that FreeBSD (and
> possibly other *BSDs too) have drm support.  So CONFIG_POSIX looks
> correct to me.

This change was my suggestion; I assumed that "open /dev/dri/whatever"
was Linux-specific. The specific thing that doesn't work on
Haiku, or on Solaris for that matter, is that the code uses
DT_CHR, which isn't in POSIX but is a Linux-and-BSDism.

thanks
-- PMM

Re: [PATC 7/9] Skipping drm build, unsupported

[David CARLIER]

1/ It does not compile on Haiku has dirent does not contain d_type
field (among other things).
2/ does not support drm anyway.
3/ Haiku is less portable than a illumos or NetBSD system, even with
the BSD compatibility layer.

On Tue, 30 Jun 2020 at 16:48, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Tue, Jun 30, 2020 at 10:46:59AM +0200, Philippe Mathieu-Daudé wrote:
> > On 6/30/20 10:23 AM, Gerd Hoffmann wrote:
> > > On Tue, Jun 30, 2020 at 08:44:24AM +0200, Philippe Mathieu-Daudé wrote:
> > >> +Gerd
> > >>
> > >> On 6/29/20 11:48 PM, David CARLIER wrote:
> > >>> From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
> > >>> From: David Carlier <devnexen@gmail.com>
> > >>> Date: Mon, 29 Jun 2020 22:20:06 +0000
> > >>> Subject: [PATCH 7/9] Skipping drm build, unsupported.
> > >
> > > --verbose please.
> >
> > David has difficulties understanding how to send patches,
> > so you missed the cover/context. This is for the Haiku OS
> > which apparently is POSIX.1-2001 compatible.
>
> That doesn't explain why he thinks this patch is needed.
> It should build just fine on Haiku ...
>
> take care,
>   Gerd
>

Re: [PATC 7/9] Skipping drm build, unsupported

[David CARLIER]

Otherwise, if it is ok if not all patches are accepted (like this one)
but at least most of them would be nice then Haikuport can decrease
needed local patches.

Regards.

On Tue, 30 Jun 2020 at 16:55, David CARLIER <devnexen@gmail.com> wrote:
>
> 1/ It does not compile on Haiku has dirent does not contain d_type
> field (among other things).
> 2/ does not support drm anyway.
> 3/ Haiku is less portable than a illumos or NetBSD system, even with
> the BSD compatibility layer.
>
> On Tue, 30 Jun 2020 at 16:48, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 10:46:59AM +0200, Philippe Mathieu-Daudé wrote:
> > > On 6/30/20 10:23 AM, Gerd Hoffmann wrote:
> > > > On Tue, Jun 30, 2020 at 08:44:24AM +0200, Philippe Mathieu-Daudé wrote:
> > > >> +Gerd
> > > >>
> > > >> On 6/29/20 11:48 PM, David CARLIER wrote:
> > > >>> From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001
> > > >>> From: David Carlier <devnexen@gmail.com>
> > > >>> Date: Mon, 29 Jun 2020 22:20:06 +0000
> > > >>> Subject: [PATCH 7/9] Skipping drm build, unsupported.
> > > >
> > > > --verbose please.
> > >
> > > David has difficulties understanding how to send patches,
> > > so you missed the cover/context. This is for the Haiku OS
> > > which apparently is POSIX.1-2001 compatible.
> >
> > That doesn't explain why he thinks this patch is needed.
> > It should build just fine on Haiku ...
> >
> > take care,
> >   Gerd
> >

Re: [PATC 7/9] Skipping drm build, unsupported

[Gerd Hoffmann]

> > > > -util-obj-$(CONFIG_POSIX) += drm.o
> > > > +util-obj-$(CONFIG_LINUX) += drm.o
> >
> > Can't see anything linux-specific there.  Also note that FreeBSD (and
> > possibly other *BSDs too) have drm support.  So CONFIG_POSIX looks
> > correct to me.
> 
> This change was my suggestion; I assumed that "open /dev/dri/whatever"
> was Linux-specific. The specific thing that doesn't work on
> Haiku, or on Solaris for that matter, is that the code uses
> DT_CHR, which isn't in POSIX but is a Linux-and-BSDism.

Ah, that is the problem.  Yes, DT_CHR is an non-posix optimization which
allows to get the file type directly, without another round-trip to the
kernel.  If that isn't available you can stat() the file and check
((st_mode & S_IFMT) == S_IFCHR) instead.

take care,
  Gerd

Re: [PATC 7/9] Skipping drm build, unsupported

[Eric Blake]

On 6/30/20 11:53 AM, Gerd Hoffmann wrote:
>>>>> -util-obj-$(CONFIG_POSIX) += drm.o
>>>>> +util-obj-$(CONFIG_LINUX) += drm.o
>>>
>>> Can't see anything linux-specific there.  Also note that FreeBSD (and
>>> possibly other *BSDs too) have drm support.  So CONFIG_POSIX looks
>>> correct to me.
>>
>> This change was my suggestion; I assumed that "open /dev/dri/whatever"
>> was Linux-specific. The specific thing that doesn't work on
>> Haiku, or on Solaris for that matter, is that the code uses
>> DT_CHR, which isn't in POSIX but is a Linux-and-BSDism.
> 
> Ah, that is the problem.  Yes, DT_CHR is an non-posix optimization which
> allows to get the file type directly, without another round-trip to the
> kernel.  If that isn't available you can stat() the file and check
> ((st_mode & S_IFMT) == S_IFCHR) instead.

Even when d_type and DT_CHR is available, there are filesystems where 
the Linux kernel reports d_type of DT_UNKNOWN, and where you are best 
having that code also falling back to an fstat().  In short, any 
portable code that uses d_type should have fallback code for DT_UNKNOWN, 
at which point porting to systems without d_type is as easy as writing 
an accessor macro that returns d_type when it exists and DT_UNKNOWN 
where it doesn't.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATC 7/9] Skipping drm build, unsupported

[Gerd Hoffmann]

> > Ah, that is the problem.  Yes, DT_CHR is an non-posix optimization which
> > allows to get the file type directly, without another round-trip to the
> > kernel.  If that isn't available you can stat() the file and check
> > ((st_mode & S_IFMT) == S_IFCHR) instead.
> 
> Even when d_type and DT_CHR is available, there are filesystems where the
> Linux kernel reports d_type of DT_UNKNOWN, and where you are best having
> that code also falling back to an fstat().

Given this isn't perforance critical at all it is probably simplest to
avoid non-portable d_type altogether and just to the fstat
unconditionally.

David, does that work for haiku?

take care,
  Gerd

diff --git a/util/drm.c b/util/drm.c
index a23ff2453826..a1d3520d00f2 100644
--- a/util/drm.c
+++ b/util/drm.c
@@ -24,6 +24,7 @@ int qemu_drm_rendernode_open(const char *rendernode)
 {
     DIR *dir;
     struct dirent *e;
+    struct stat st;
     int r, fd;
     char *p;
 
@@ -38,10 +39,6 @@ int qemu_drm_rendernode_open(const char *rendernode)
 
     fd = -1;
     while ((e = readdir(dir))) {
-        if (e->d_type != DT_CHR) {
-            continue;
-        }
-
         if (strncmp(e->d_name, "renderD", 7)) {
             continue;
         }
@@ -53,6 +50,12 @@ int qemu_drm_rendernode_open(const char *rendernode)
             g_free(p);
             continue;
         }
+        fstat(r, &st);
+        if ((st.st_mode & S_IFMT) != S_IFCHR) {
+            close(r);
+            g_free(p);
+            continue;
+        }
         fd = r;
         g_free(p);
         break;

Re: [PATC 7/9] Skipping drm build, unsupported

[David CARLIER]

Yes it does. Regards.

On Wed, 1 Jul 2020 at 16:15, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > Ah, that is the problem.  Yes, DT_CHR is an non-posix optimization which
> > > allows to get the file type directly, without another round-trip to the
> > > kernel.  If that isn't available you can stat() the file and check
> > > ((st_mode & S_IFMT) == S_IFCHR) instead.
> >
> > Even when d_type and DT_CHR is available, there are filesystems where the
> > Linux kernel reports d_type of DT_UNKNOWN, and where you are best having
> > that code also falling back to an fstat().
>
> Given this isn't perforance critical at all it is probably simplest to
> avoid non-portable d_type altogether and just to the fstat
> unconditionally.
>
> David, does that work for haiku?
>
> take care,
>   Gerd
>
> diff --git a/util/drm.c b/util/drm.c
> index a23ff2453826..a1d3520d00f2 100644
> --- a/util/drm.c
> +++ b/util/drm.c
> @@ -24,6 +24,7 @@ int qemu_drm_rendernode_open(const char *rendernode)
>  {
>      DIR *dir;
>      struct dirent *e;
> +    struct stat st;
>      int r, fd;
>      char *p;
>
> @@ -38,10 +39,6 @@ int qemu_drm_rendernode_open(const char *rendernode)
>
>      fd = -1;
>      while ((e = readdir(dir))) {
> -        if (e->d_type != DT_CHR) {
> -            continue;
> -        }
> -
>          if (strncmp(e->d_name, "renderD", 7)) {
>              continue;
>          }
> @@ -53,6 +50,12 @@ int qemu_drm_rendernode_open(const char *rendernode)
>              g_free(p);
>              continue;
>          }
> +        fstat(r, &st);
> +        if ((st.st_mode & S_IFMT) != S_IFCHR) {
> +            close(r);
> +            g_free(p);
> +            continue;
> +        }
>          fd = r;
>          g_free(p);
>          break;
>

Re: [PATC 7/9] Skipping drm build, unsupported

[Philippe Mathieu-Daudé]

Hi Gerd,

On 7/1/20 5:15 PM, Gerd Hoffmann wrote:
>>> Ah, that is the problem.  Yes, DT_CHR is an non-posix optimization which
>>> allows to get the file type directly, without another round-trip to the
>>> kernel.  If that isn't available you can stat() the file and check
>>> ((st_mode & S_IFMT) == S_IFCHR) instead.
>>
>> Even when d_type and DT_CHR is available, there are filesystems where the
>> Linux kernel reports d_type of DT_UNKNOWN, and where you are best having
>> that code also falling back to an fstat().
> 
> Given this isn't perforance critical at all it is probably simplest to
> avoid non-portable d_type altogether and just to the fstat
> unconditionally.
> 
> David, does that work for haiku?
> 
> take care,
>   Gerd
> 
> diff --git a/util/drm.c b/util/drm.c
> index a23ff2453826..a1d3520d00f2 100644
> --- a/util/drm.c
> +++ b/util/drm.c
> @@ -24,6 +24,7 @@ int qemu_drm_rendernode_open(const char *rendernode)
>  {
>      DIR *dir;
>      struct dirent *e;
> +    struct stat st;
>      int r, fd;
>      char *p;
>  
> @@ -38,10 +39,6 @@ int qemu_drm_rendernode_open(const char *rendernode)
>  
>      fd = -1;
>      while ((e = readdir(dir))) {
> -        if (e->d_type != DT_CHR) {
> -            continue;
> -        }
> -
>          if (strncmp(e->d_name, "renderD", 7)) {
>              continue;
>          }
> @@ -53,6 +50,12 @@ int qemu_drm_rendernode_open(const char *rendernode)
>              g_free(p);
>              continue;
>          }
> +        fstat(r, &st);

While preparing the formal patch, can you add a comment here explaining
we deliberately use this way for portability (not checking DT_CHR /
DT_UNKNOWN ...)?

Thanks!

> +        if ((st.st_mode & S_IFMT) != S_IFCHR) {
> +            close(r);
> +            g_free(p);
> +            continue;
> +        }
>          fd = r;
>          g_free(p);
>          break;
> 

Re: [PATC 7/9] Skipping drm build, unsupported

[Peter Maydell]

On Wed, 1 Jul 2020 at 16:15, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > Ah, that is the problem.  Yes, DT_CHR is an non-posix optimization which
> > > allows to get the file type directly, without another round-trip to the
> > > kernel.  If that isn't available you can stat() the file and check
> > > ((st_mode & S_IFMT) == S_IFCHR) instead.
> >
> > Even when d_type and DT_CHR is available, there are filesystems where the
> > Linux kernel reports d_type of DT_UNKNOWN, and where you are best having
> > that code also falling back to an fstat().
>
> Given this isn't perforance critical at all it is probably simplest to
> avoid non-portable d_type altogether and just to the fstat
> unconditionally.

> @@ -53,6 +50,12 @@ int qemu_drm_rendernode_open(const char *rendernode)
>              g_free(p);
>              continue;
>          }
> +        fstat(r, &st);

Don't forget to check the fstat return code in the final
version of this patch :-)

thanks
-- PMM