Re: [PATC 7/9] Skipping drm build, unsupported
On Wed, 1 Jul 2020 at 16:15, 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. > @@ -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
Re: [PATC 7/9] Skipping drm build, unsupported
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
Yes it does. Regards.
On Wed, 1 Jul 2020 at 16:15, 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);
> +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
> > 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
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
> > > > -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
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 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 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 > > > >>> Date: Mon, 29 Jun 2020 22:20:06 + > > > >>> 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
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 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 > > >>> Date: Mon, 29 Jun 2020 22:20:06 + > > >>> 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
On Tue, 30 Jun 2020 at 09:24, 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 > > > Date: Mon, 29 Jun 2020 22:20:06 + > > > 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
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 > >>> Date: Mon, 29 Jun 2020 22:20:06 + > >>> 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
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 >>> Date: Mon, 29 Jun 2020 22:20:06 + >>> 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
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 > > Date: Mon, 29 Jun 2020 22:20:06 + > > 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
+Gerd On 6/29/20 11:48 PM, David CARLIER wrote: > From 157a0374093371719de42e99364352d64190f52a Mon Sep 17 00:00:00 2001 > From: David Carlier > Date: Mon, 29 Jun 2020 22:20:06 + > Subject: [PATCH 7/9] Skipping drm build, unsupported. > > Signed-off-by: David Carlier Reviewed-by: Philippe Mathieu-Daudé > --- > 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 >
