AW: "Modern C" and the X.Org software

[Walter Harms]

Hi Alan,
just for my understanding.
You compiled everything and only these few thinks showed up.
Or is more like an iceberg only the tip is there and we should do something ?

CU

Von: xorg-devel  im Auftrag von Alan 
Coopersmith 
Gesendet: Montag, 11. Dezember 2023 23:29
An: X.Org Development
Betreff: "Modern C" and the X.Org software

As folks may have seen from https://lwn.net/Articles/954018/ or elsewhere,
there's efforts underway to make more warnings into errors in gcc & clang,
and as part of those, people are compiling their distros/packages with
flags to turn warnings like this into errors:

   -Wimplicit-function-declaration
   -Wimplicit-int
   -Wint-conversion
   -Wreturn-mismatch (new, previously part of -Wreturn-types)
   -Wdeclaration-missing-parameter-type (new, previously unnamed)
   -Wincompatible-pointer-types

Fortunately, we've had some of these in our default compiler flags from
xorg-macros for over a decade, as long as the compiler seems to support
them - both -Werror=implicit & -Werror=return-type.

Sam James from Gentoo filed bugs for 4 issues found in their testing
over the past week.  I've added the flags and did my own test builds
on Solaris 11.4 using the autoconf-2.72d beta that makes autoconf tests
work with these flags, and found some more myself.  I've also looked
through the Gentoo & Fedora lists at
https://lists.fedoraproject.org/archives/list/de...@lists.fedoraproject.org/thread/GKCRXZESHSCCEKED2N5GNQ7GH32VSK2X/
https://bugs.gentoo.org/showdependencytree.cgi?id=870412_resolved=1
to see if they'd found any more, and I think we've found all the ones
in the packages we maintain via X.Org.


The fixes I've proposed for these are:

xkbwatch: Fix -Wincompatible-pointer-types warning (issue #2)
https://gitlab.freedesktop.org/xorg/app/xkbutils/-/merge_requests/4

Fix -Wincompatible-pointer-types warning from gcc (issue #1)
https://gitlab.freedesktop.org/xorg/app/xlsfonts/-/merge_requests/6

Fix -Wincompatible-pointer-types warning from gcc (issue #15)
https://gitlab.freedesktop.org/xorg/app/xdm/-/merge_requests/18

Fix -Wincompatible-pointer-types warning (Issue #3)
https://gitlab.freedesktop.org/xorg/util/imake/-/merge_requests/8

Multisink.c: Clear -Werror=incompatible-pointer-types error
https://gitlab.freedesktop.org/xorg/lib/libxaw3d/-/merge_requests/10


I also added -std=gnu23 to my build flags, which found one more issue
due to C23 defining "true" as an rvalue, not an lvalue:

Fix C23 build by renaming variable 'true'
https://gitlab.freedesktop.org/xorg/util/imake/-/merge_requests/8


There were two that I've "fixed" by making the drivers simply not
call xf86DisableRandR in ABI_VIDEODRV_VERSION 24 & later - which may
mean that in the old-style manual rotation configurations attempts
to use RandR will do awful things - but since there were no bugs filed
about use of those config options making the drivers crash trying to
call an unresolvable symbol, I'm guessing no one cares about that:

https://gitlab.freedesktop.org/xorg/driver/xf86-video-nv/-/merge_requests/9
https://gitlab.freedesktop.org/xorg/driver/xf86-video-savage/-/merge_requests/6

(If I'm wrong, someone who does care can submit patches to fix it.)


While I'll probably merge all of the above MR's into git before the holidays,
I don't plan to make any new releases including them until after, so that they
can be released with the final release of autoconf 2.72 (which is planned for
release sometime in the week of Dec. 18 - 22), which has both the fixes for
bugs in autoconf tests caused by these flags and the addition of support for
64-bit time_t in 32-bit programs which some distros/packagers may desire.

--
 -Alan Coopersmith- alan.coopersm...@oracle.com
  Oracle Solaris Engineering - https://blogs.oracle.com/solaris

AW: Hitting X limit in launching maximum number of vkcube instances

[Walter Harms]

Hi Anuj,
i did not follow the diskussion in detail, so maybe this was already purposed:
Some years ago in an unrelated project i also hit the max file pointer limit. 
The reason seemed obvious but in the end it turned out the be something 
different.
Did you check you hit the expected limit when reducing the max number of file 
pointers ?
(This helps also to locate the culprit by checking with lsof).

jm2c.
 wh

Von: xorg-devel  im Auftrag von Anuj Phogat 

Gesendet: Donnerstag, 17. August 2023 00:01
An: Olivier Fourdan
Cc: xorg-devel@lists.x.org; Adam Jackson
Betreff: Re: Hitting X limit in launching maximum number of vkcube instances

Hi Olivier,

Thanks for experimenting on your end to rule out any X server issues.
I was able to match your results and launch 512 instances of vkcube
after making sure that Xserver (1.21.1.8) has inherited the higher
"max open files" limit. I really appreciate help from you, Adam and Alan.

Anuj

On Wed, Aug 16, 2023 at 12:46 AM Olivier Fourdan  wrote:
>
> Hi Anuj,
>
> On Mon, Aug 14, 2023 at 8:27 PM Anuj Phogat  wrote:
>>
>> - I'm still getting the "_XSERVTransSocketUNIXAccept: accept() failed" error
>> after launching ~ 256 instances of vkcube. I was able to find the root
>> cause of error after patching libxtrans based on Adam's suggestion:
>> "_XSERVTransSocketUNIXAccept: accept() failed (Too many open files)"
>> - Based on the hint from the error string, I changed the system wide limit to
>> increase the maximum number of open files allowed from default 1024 to
>> 'ulimit -n' confirmed the new limit. But, this change doesn't change
>> the error past launching ~ 256 instances of vkcube.
>> - I hit the same error but at ~330 instances when I try to run glxgears.
>>
>> Questions:
>> What am I missing when changing the open files limit ?
>
>
> Make sure the X process has inherited that limit, depending on how/where you 
> bumped the limit, the X server process may not have that applied.
>
> You can check using the /proc filesystem on Linux:
>
> $ cat /proc/$(pidof Xorg)/limits
>
>> What else can I try to get past this error ?
>
>
> If you can get 330 instances of glxgears, it means that you are already past 
> the 256 limit, hence the maxclients worked.
>
> It is worth noting that the limit applies to all X11 clients, so if you run a 
> window manager, an xterm, etc, everything counts.
>
> Also if a client opens more than one connection to the X server, that also 
> counts. So you can expect that a limit of 512 might give you a lower actual 
> limit.
>
>>
>> Should I reopen xserver issue [1] or create a new issue to track it ?
>
>
> I doubt that this is an X server issue.
>
> FWIW, I just tried here with Xwayland rootful and was able to run 474 
> instances of vkcube on a rootful stanlone Xwayland instance:
>
> $ Xwayland -maxclients 512 -decorate :12 7
> $ for i in $(seq 1 512); do DISPLAY=:12 vkcube& done
> $ ps aux | grep vkcube | wc -l
> 474
>
> I have seen some "Maximum number of clients reached" errors while the vkcube 
> instances were spawning en masse, so I suspect maybe the vulkan 
> implementation is opening a connection to the display for some reason 
> temporarily, so having those start all at once may lead to a lower actual 
> limit of clients..
>
> To confirm that theory, I dedid the same test waiting a bit between each 
> instance of vkcube.
>
> And nowI can reach the limit of 512:
>
> $ for i in $(seq 1 512); do DISPLAY=:12 vkcube; sleep .2 & done
> $ ps aux | grep vkcube | wc -l
> 512
>
> So yeah, no bug in the Xserver AFAICS.
>
> This is with:
>
> $ cat /proc/$(pidof Xwayland)/limits
> Limit Soft Limit   Hard Limit   Units
> Max cpu time  unlimitedunlimitedseconds
> Max file size unlimitedunlimitedbytes
> Max data size unlimitedunlimitedbytes
> Max stack size8388608  unlimitedbytes
> Max core file sizeunlimitedunlimitedbytes
> Max resident set  unlimitedunlimitedbytes
> Max processes 6231962319processes
> Max open files16777216 16777216 files
> Max locked memory 8388608  8388608  bytes
> Max address space unlimitedunlimitedbytes
> Max file locksunlimitedunlimitedlocks
> Max pending signals   6231962319signals
> Max msgqueue size 819200   819200   bytes
> Max nice priority 00
> Max realtime priority 00
> Max realtime timeout  unlimitedunlimitedus
>
>> [1] : https://gitlab.freedesktop.org/xorg/xserver/-/issues/1310
>
>
> Cheers
> Olivier
>
>

AW: PATCH libX11 2/2] ChkIfEv.c: fix wrong handling of dpy->in_ifevent

[Walter Harms]

maybe i missed a point,
is it possible that  dpy->in_ifevent will underflow ?

re,
 wh

Von: xorg-devel  im Auftrag von Ulrich Sibiller 

Gesendet: Mittwoch, 30. November 2022 23:47
An: xorg-devel@lists.x.org
Betreff: PATCH libX11 2/2] ChkIfEv.c: fix wrong handling of dpy->in_ifevent

>From 9d6bb3d179427d832d811e61fb6e4ebb9d004283 Mon Sep 17 00:00:00 2001
From: Ulrich Sibiller 
Date: Wed, 30 Nov 2022 22:19:15 +0100
Subject: [PATCH libX11 2/2] ChkIfEv.c: fix wrong handling of dpy->in_ifevent

Is no longer a bool but a counter.

Signed-off-by: Ulrich Sibiller 
---
 src/ChkIfEv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ChkIfEv.c b/src/ChkIfEv.c
index eba36941..b32c2d3e 100644
--- a/src/ChkIfEv.c
+++ b/src/ChkIfEv.c
@@ -61,7 +61,7 @@ Bool XCheckIfEvent (
*event = qelt->event;
_XDeq(dpy, prev, qelt);
_XStoreEventCookie(dpy, event);
-dpy->in_ifevent = False;
+   dpy->in_ifevent--;
UnlockDisplay(dpy);
return True;
}
--
2.20.1

AW: Yet another leak in Xlib

[Walter Harms]

will sombody close the hole in the documentation ?

Von: xorg-devel  im Auftrag von Po Lu 

Gesendet: Dienstag, 4. Oktober 2022 04:38
An: Thomas Dickey
Cc: xorg-devel@lists.x.org
Betreff: Re: Yet another leak in Xlib

Thomas Dickey  writes:

> looks okay reading the library code (src/xlibi18n/XDefaultIMIF.c, _CloseIM).
>
> xterm doesn't free that 'xim' value (and the XCloseIM manual page doesn't
> say who's responsible for that -- though it's possible that some other
> application developer read the library source code and is freeing it).

XCloseIM (in IMWrap.c) frees the XIM value itself after calling close to
deinitialize the input method.

So I think the patch should be fine, and I've been running it for a
while with no ill effect.  Could it be installed?

Thanks.

AW: [PATCH] xrdb: Add actual querying capabilities to -query

[Walter Harms]

Hi,
thx for the patch.

NTL it is better for the general look if you use a name
for sign-off that is more easy to pronounce.

for a starter: 
yes, i think we can drop support for solaris 4 :)

IMHO, the idea is good. I am struggling a bit with
reusing the query option but thats not a showstopper.
Lets see if a discussion comes up and if no one objects
ping me next WE.

re,
 wh


Von: xorg-devel  im Auftrag von rnhmjoj 

Gesendet: Sonntag, 20. Juni 2021 13:28
An: xorg-devel@lists.x.org
Betreff: [PATCH] xrdb: Add actual querying capabilities to -query

Hi all,

I've opened a pull request on gitlab[1] months ago, but apparently
it hasn't been noticed, so I'm sending the patch here too. It's a
pretty simple change to xrdb but I think it's very useful: I've been
using it for a while and it simplified my shell scripts a lot.

[1]: https://gitlab.freedesktop.org/xorg/app/xrdb/-/merge_requests/2

--- ORIGINAL PATCH HERE ---

The world is littered with broken grep commands because `xrdb -query`
can only dump the database and doesn't implement this simple search feature.

Things I tested:

  - `xrdb -query` without arguments works the same as before

  - `xrdb -query prop` prints the value of `prop` when it exists in the
resource database.

  - `xrdb -query prop` doesn't print anything when `prop` doesn't exist
in the resource database.

  - `xrdb -query prop` doesn't leak any memory. I run it in valgrind
with the `--leak-check=full` option.

PS: I think Sun took care of the fputs thing by now.

Signed-off-by: rnhmjoj 
---
 xrdb.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/xrdb.c b/xrdb.c
index 3f6e533..4b69363 100644
--- a/xrdb.c
+++ b/xrdb.c
@@ -48,6 +48,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -126,6 +127,7 @@ static char *editFile = NULL;
 static const char *cpp_program = NULL;
 static const char * const cpp_locations[] = { CPP };
 static const char *backup_suffix = BACKUP_SUFFIX;
+static const char *query_property = NULL;
 static Bool dont_execute = False;
 static Bool show_cpp = False;
 static String defines;
@@ -785,7 +787,7 @@ Syntax(const char *errmsg)
 " -cpp filename   preprocessor to use [%s]\n"
 " -nocpp  do not use a preprocessor\n"
 " -E  show preprocessor command & processed input 
file\n"
-" -query  query resources\n"
+" -query [property]   query resources\n"
 " -load   load resources from file [default]\n"
 " -override   add in resources from file\n"
 " -merge  merge resources from file & sort\n"
@@ -982,6 +984,8 @@ main(int argc, char *argv[])
 }
 else if (isabbreviation("-query", arg, 2)) {
 oper = OPQUERY;
+if (i+1 < argc && argv[i+1][0] != '-')
+query_property = argv[++i];
 continue;
 }
 else if (isabbreviation("-load", arg, 2)) {
@@ -1283,8 +1287,18 @@ Process(int scrno, Bool doScreen, Bool execute)
 printf("%s\n", defines.val);
 }
 else if (oper == OPQUERY) {
-if (xdefs)
-printf("%s", xdefs);/* fputs broken in SunOS 4.0 */
+if (xdefs && query_property != NULL) {
+char *type = NULL;
+XrmValue value;
+XrmDatabase xrdb = XrmGetStringDatabase(xdefs);
+Bool found = XrmGetResource(xrdb, query_property,
+query_property, , );
+if (found == True && value.addr != NULL)
+printf("%s\n", value.addr);
+XrmDestroyDatabase(xrdb);
+}
+else if (xdefs)
+fputs(xdefs, stdout);
 }
 else if (oper == OPREMOVE) {
 if (xdefs)
--
2.31.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: [Xlib] Ignoring BadAccess errors

[Walter Harms]

since no one feels the need to answer is will try:

Having those exceptions is obviously a hack to allow clients
to continue on "minor" errors. e.g. BadAccess can be generated
by xgrab*() and may work in a second attempt. 

NTL this should be documented in the doc.

regarding you original question:
 I need a function which robustly returns an
error when we in untrusted context, which returned exactly when client is 
untrusted and not for any other reasons.

this is something i can not answer, sorry.

re,
 wh

Von: xorg-devel  im Auftrag von Nekun 

Gesendet: Dienstag, 20. April 2021 13:19
An: xorg-devel@lists.x.org
Betreff: [Xlib] Ignoring BadAccess errors

WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf 
Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die 
Absender*in und wissen, dass der Inhalt sicher ist.


Hi all.

Can somebody explain me why BadAlloc and BadAccess errors are ignored in
Xlib reply processing, but only in certain conditions? As I can see
(maybe I misunderstood something, please correct), in _Xreply:
https://cgit.freedesktop.org/xorg/lib/libX11/tree/src/xcb_io.c#n655 ,
after sending a request, at first we process pending responses in input
buffer and handle it without any exceptions and then we process the last
response and ignore some errors: BadName and BadFont in some conditions
described in the Xlib spec:
https://www.x.org/releases/current/doc/libX11/libX11/libX11.html#Using_the_Default_Error_Handlers
, and, suddenly, completely ignore BadAccess and BadAlloc. I don't found
any references for that behavior. In result, I can't trap these errors
with my error handler if an Xlib request wrapper calls _Xreply
explicitly. Some wrappers, such as XBell, writes a request to output
queue, calls the sync handler and then errors are handled without
exception because XSync adds InputSetFocus to queue, so the target
request is processed with no-exception logic described above. This issue
arised when I worked on fixing the broken untrusted context check in GTK
(GDK) at display opening. I need a function which robustly returns an
error when we in untrusted context, which returned exactly when client
is untrusted and not for any other reasons, and throw in the gdk_x_error
trap. Tried XListHosts and failed for reasons described above. Maybe I
can use a return value of some function instead of error handling? Seems
like X11 security extension more often returns fake values for untrusted
clients rather than errors, but not sure that is possible to distinguish
between a fake value and a real in any response. Stucked here.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: New libxcvt library

[Walter Harms]

Hi,
i think it is a good idea.
Can you give a hint how big that lib is ?

Even when the idea is good it is not really helpful to have to many
different libs hanging around. I read Keith Packard was already considering 
putting that into xrandr, if its not to big i would say it would
be a good idea to collect the monitor related things into one lib (EDID ?). 

re,
 wh


Von: xorg-devel  im Auftrag von Olivier Fourdan 

Gesendet: Freitag, 26. März 2021 16:58
An: xorg-devel
Betreff: RFC: New libxcvt library

WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf 
Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die 
Absender*in und wissen, dass der Inhalt sicher ist.


Hi all,

Currently. the Xorg Xserver has its own implementation of the VESA CVT standard 
timing modelines generator in `hw/xfree86/modes/xf86cvt.c`.

That code is placed in its own source file alone because it is also being used 
by the `cvt` utility.

Because it's part of the Xorg DDX, Xwayland, which is another DDX also has a 
copy of the same code in `hw/xwayland/xwayland-cvt.c`.

Now with Xwayland being a standalone package, mutter which uses the cvt utility 
at build time still needs to install `cvt` from Xorg to generate the modes used 
in Wayland.

Some time ago, discussing with Jonas Adahl on irc, we thought it would be a 
good idea to have that code and utility part of its own standalone project, to 
avoid the code duplication and possibly ease contributions.

That was https://gitlab.freedesktop.org/xorg/xserver/-/issues/1142

So I went ahead and did just that, called it “libxcvt” and placed in 
https://gitlab.freedesktop.org/ofourdan/libxcvt

I also made a merge request to use that code in the Xserver: 
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/637

Can we consider moving “libxcvt” to the xorg/lib namespace in gitlab?

Cheers
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: [PATCH] mi: sprite: SaveUnderCursor2

[Walter Harms]

hi Madhurkiran Harikrishnan,
my experience is that readability reduced rapidly with indent-level.
I would suggest:
 
 if ( ! DevHasCursor(pDev) continue;

pCursorInfo = GetSprite(pDev);

if (!pCursorInfo) continue; // just to shrink the if ()


// no braces needed
 if ( pCursorInfo->isUp && 
   pCursorInfo->pScreen == pScreen) 
 miSpriteSaveUnderCursor(pDev, pScreen);

jm2c
re,
 wh

Von: xorg-devel  im Auftrag von Madhurkiran 
Harikrishnan 
Gesendet: Montag, 8. März 2021 23:09
An: xorg-devel@lists.x.org
Cc: Madhurkiran Harikrishnan; Hyun Kwon
Betreff: [PATCH] mi: sprite: SaveUnderCursor2

From: Hyun Kwon 

Signed-off-by: Hyun Kwon 
Signed-off-by: Madhurkiran Harikrishnan 
---
 mi/mipointer.h |  2 ++
 mi/misprite.c  | 17 +
 2 files changed, 19 insertions(+)

diff --git a/mi/mipointer.h b/mi/mipointer.h
index 7ce6409..107b24f 100644
--- a/mi/mipointer.h
+++ b/mi/mipointer.h
@@ -127,4 +127,6 @@ extern _X_EXPORT DevPrivateKeyRec miPointerScreenKeyRec;

 #define miPointerScreenKey ()

+extern _X_EXPORT void miDCSaveUnderCursor2(ScreenPtr pScreen);
+
 #endif  /* MIPOINTER_H */
diff --git a/mi/misprite.c b/mi/misprite.c
index add2c55..46ce9f0 100644
--- a/mi/misprite.c
+++ b/mi/misprite.c
@@ -955,3 +955,20 @@ miSpriteComputeSaved(DeviceIntPtr pDev, ScreenPtr pScreen)
 pCursorInfo->saved.x2 = pCursorInfo->saved.x1 + w + wpad * 2;
 pCursorInfo->saved.y2 = pCursorInfo->saved.y1 + h + hpad * 2;
 }
+
+void
+miDCSaveUnderCursor2(ScreenPtr pScreen)
+{
+   DeviceIntPtr pDev;
+   miCursorInfoPtr pCursorInfo;
+
+   for (pDev = inputInfo.devices; pDev; pDev = pDev->next) {
+   if (DevHasCursor(pDev)) {
+   pCursorInfo = GetSprite(pDev);
+   if (pCursorInfo && pCursorInfo->isUp
+   && pCursorInfo->pScreen == pScreen) {
+   miSpriteSaveUnderCursor(pDev, pScreen);
+   }
+   }
+   }
+}
--
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: AW: AW: Preparing for libX11 1.7.0

[Walter Harms]

nobody expects this to become bug free. The point was to raise awareness that 
the
same class (heap-use-after-free) are still reported.


Von: Alan Coopersmith 
Gesendet: Donnerstag, 19. November 2020 18:07
An: Walter Harms; Keith Packard; Matthieu Herrb; 
xorg-de...@lists.freedesktop.org
Cc: Vittorio Zecca
Betreff: Re: AW: AW: Preparing for libX11 1.7.0

The original issue should be fixed by Keith's commit yesterday:
https://gitlab.freedesktop.org/xorg/lib/libx11/-/commit/a3c0b5dbd6b

I also put in a commit yesterday to prevent some potential use-after-free
issues found by our static analyzer:
https://gitlab.freedesktop.org/xorg/lib/libx11/-/commit/103e2e11519

If we wait until libX11 is completely bug free, we'll never ship a release.
Shipping 1.7.0 doesn't mean we stop work - these could continue to be
investigated for 1.7.1 while users get a significant set of bug fixes and
improvements in 1.7.0.

-alan-

On 11/19/20 8:32 AM, Walter Harms wrote:
> I would ask to wait before releasing a new version.
> Actually i had no time the check that, maybe they are all fixed now.
> NTL we should investigate and fix.
>
> btw:gcc has some warnings for xts also
>
> Vittorio Zecca reportet that xts5 finds some more issues.
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/vitti/rpmbuil/SOURCES/libX11-1.6.12/src/DrPoint.c:47 in XDrawPoint
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/SetClMask.c:40 in XSetClipMask
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/CrGC.c:339 in XFlushGC
> SUMMARY: AddressSanitizer: heap-buffer-overflow 
> (/home/vitti/libasan.so+0x39dd2) in __interceptor_memcpy
> SUMMARY: AddressSanitizer: double-free (/home/vitti/libasan.so+0xab0c7) in 
> __interceptor_free
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/DrLine.c:50 in XDrawLine
> SUMMARY: AddressSanitizer: heap-buffer-overflow 
> (/home/vitti/libasan.so+0x589c2) in __interceptor_strncpy
> SUMMARY: AddressSanitizer: heap-buffer-overflow 
> (/home/vitti/libasan.so+0x39dd2) in __interceptor_memcpy
> SUMMARY: AddressSanitizer: heap-buffer-overflow 
> (/home/vitti/libasan.so+0x39dd2) in __interceptor_memcpy
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/DrLine.c:50 in XDrawLine
>
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/QuExt.c:43 in XQueryExtension
>
> ____
> Von: Keith Packard 
> Gesendet: Dienstag, 17. November 2020 03:11
> An: Alan Coopersmith; Walter Harms; Matthieu Herrb; 
> xorg-de...@lists.freedesktop.org
> Cc: Vittorio Zecca
> Betreff: Re: AW: Preparing for libX11 1.7.0
>
> Alan Coopersmith  writes:
>
>> https://lists.x.org/archives/xorg/2020-November/060510.html
>
> I've reviewed this message and believe that this issue has already been
> fixed on Xlib master -- Jacek Caban provided a set of fixes over three
> years ago which have been merged along with some small additional work I
> did as well:
>
>  https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/56
>
> This series gives up on ever freeing locale information due to Xlib API
> design issues, so it's likely to avoid accessing something after it has
> been freed.
>
> --
> -keith
>


--
-Alan Coopersmith-   alan.coopersm...@oracle.com
 Oracle Solaris Engineering - https://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: AW: Preparing for libX11 1.7.0

[Walter Harms]

I would ask to wait before releasing a new version.
Actually i had no time the check that, maybe they are all fixed now.
NTL we should investigate and fix.

btw:gcc has some warnings for xts also

Vittorio Zecca reportet that xts5 finds some more issues.
SUMMARY: AddressSanitizer: heap-use-after-free 
/home/vitti/rpmbuil/SOURCES/libX11-1.6.12/src/DrPoint.c:47 in XDrawPoint
SUMMARY: AddressSanitizer: heap-use-after-free 
/home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/SetClMask.c:40 in XSetClipMask
SUMMARY: AddressSanitizer: heap-use-after-free 
/home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/CrGC.c:339 in XFlushGC
SUMMARY: AddressSanitizer: heap-buffer-overflow 
(/home/vitti/libasan.so+0x39dd2) in __interceptor_memcpy
SUMMARY: AddressSanitizer: double-free (/home/vitti/libasan.so+0xab0c7) in 
__interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free 
/home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/DrLine.c:50 in XDrawLine
SUMMARY: AddressSanitizer: heap-buffer-overflow 
(/home/vitti/libasan.so+0x589c2) in __interceptor_strncpy
SUMMARY: AddressSanitizer: heap-buffer-overflow 
(/home/vitti/libasan.so+0x39dd2) in __interceptor_memcpy
SUMMARY: AddressSanitizer: heap-buffer-overflow 
(/home/vitti/libasan.so+0x39dd2) in __interceptor_memcpy
SUMMARY: AddressSanitizer: heap-use-after-free 
/home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/DrLine.c:50 in XDrawLine

SUMMARY: AddressSanitizer: heap-use-after-free 
/home/vitti/rpmbuild/SOURCES/libX11-1.6.12/src/QuExt.c:43 in XQueryExtension


Von: Keith Packard 
Gesendet: Dienstag, 17. November 2020 03:11
An: Alan Coopersmith; Walter Harms; Matthieu Herrb; 
xorg-de...@lists.freedesktop.org
Cc: Vittorio Zecca
Betreff: Re: AW: Preparing for libX11 1.7.0

Alan Coopersmith  writes:

> https://lists.x.org/archives/xorg/2020-November/060510.html

I've reviewed this message and believe that this issue has already been
fixed on Xlib master -- Jacek Caban provided a set of fixes over three
years ago which have been merged along with some small additional work I
did as well:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/56

This series gives up on ever freeing locale information due to Xlib API
design issues, so it's likely to avoid accessing something after it has
been freed.

--
-keith
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: AW: Preparing for libX11 1.7.0

[Walter Harms]

to close this thread:
the problem reported by Vittorio Zecca seems to have vanished with the git 
version. It does no show up anymore. Vittorio can not reproduce the problem, 
also.

re,
 wh
 




Von: xorg-devel  im Auftrag von Walter Harms 

Gesendet: Dienstag, 17. November 2020 09:35
An: xorg-de...@lists.freedesktop.org
Cc: Vittorio Zecca
Betreff: AW: AW: Preparing for libX11 1.7.0

I was able to reproduce the the problem with an older version of libX11 this 
way:

LD_PRELOAD=/usr/lib64/libasan.so.4 /usr/bin/wish

Since i have not installed the lastest version yet, can someome
test this ?
I guess a yes/no would help here.


Von: xorg-devel  im Auftrag von Keith Packard 

Gesendet: Dienstag, 17. November 2020 08:33
An: Vittorio Zecca
Cc: xorg-de...@lists.freedesktop.org; Walter Harms
Betreff: Re: AW: Preparing for libX11 1.7.0

Vittorio Zecca  writes:

> Even easier to reproduce running /usr/bin/wish if you have tk installed.

I run wish (for 'gitk') many times a day and haven't seen any thing
amiss, so it could be something in your environment causing trouble.

> If there is a fix to this issue plese let me know it so I can try it
> here.

The current master branch of libX11 has a likely fix for this issue.

https://gitlab.freedesktop.org/xorg/lib/libx11

--
-keith
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: AW: Preparing for libX11 1.7.0

[Walter Harms]

I was able to reproduce the the problem with an older version of libX11 this 
way:

LD_PRELOAD=/usr/lib64/libasan.so.4 /usr/bin/wish

Since i have not installed the lastest version yet, can someome
test this ? 
I guess a yes/no would help here.


Von: xorg-devel  im Auftrag von Keith Packard 

Gesendet: Dienstag, 17. November 2020 08:33
An: Vittorio Zecca
Cc: xorg-de...@lists.freedesktop.org; Walter Harms
Betreff: Re: AW: Preparing for libX11 1.7.0

Vittorio Zecca  writes:

> Even easier to reproduce running /usr/bin/wish if you have tk installed.

I run wish (for 'gitk') many times a day and haven't seen any thing
amiss, so it could be something in your environment causing trouble.

> If there is a fix to this issue plese let me know it so I can try it
> here.

The current master branch of libX11 has a likely fix for this issue.

https://gitlab.freedesktop.org/xorg/lib/libx11

--
-keith
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: Preparing for libX11 1.7.0

[Walter Harms]

before the actions start,
i would like to point out that Vittorio Zecca reported a use-after-free in 
LibX11. It is reproduceable and it is found with libtk.

jm2c
 wh

Von: xorg-devel  im Auftrag von Matthieu Herrb 

Gesendet: Montag, 16. November 2020 07:30:59
An: xorg-de...@lists.freedesktop.org
Betreff: Re: Preparing for libX11 1.7.0

On Sun, Nov 15, 2020 at 02:01:15PM -0800, Keith Packard wrote:
> Alan Coopersmith  writes:
>
> > Since https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/15
> > was merged, I was thinking the next release should be 1.7.0 since it has
> > a new API people may want to check for with pkg-config version checks,
> > but yes, a new release once things finish getting merged seems like a
> > good idea.
>
> Thanks for thinking about what the version number should be. I've
> updated the subject line accordingly.
>

Hi,

Since a new API was added, the shared lib version number in
src/Makefie.am probably needs to be adjusted too.

--
Matthieu Herrb
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

test , please ignore

[Walter Harms]

test
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: [PATCH xserver v2] xfree86: Refine drm modes on continuous freq panel

[Walter Harms]

Von: xorg-devel  im Auftrag von Aaron Ma 

Gesendet: Dienstag, 14. April 2020 11:28
An: xorg-devel@lists.x.org; aaron...@canonical.com
Betreff: [PATCH xserver v2] xfree86: Refine drm modes on continuous freq panel

EDID1.4 replaced GTF Bit with Continuous or Non-Continuous Frequency Display.

Check the "Display Range Limits Descriptor" for GTF support.
If panel doesn't support GTF, then add gtf modes.

Otherwise X will only show the modes in "Detailed Timing Descriptor".

V2: Coding style changes.

BugLink: https://gitlab.freedesktop.org/drm/intel/issues/313
Signed-off-by: Aaron Ma 
---
 hw/xfree86/ddc/edid.h |  6 
 hw/xfree86/ddc/interpret_edid.c   | 35 +++
 hw/xfree86/ddc/xf86DDC.h  |  3 ++
 .../drivers/modesetting/drmmode_display.c |  2 +-
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/ddc/edid.h b/hw/xfree86/ddc/edid.h
index 750e4270b..dffcb3749 100644
--- a/hw/xfree86/ddc/edid.h
+++ b/hw/xfree86/ddc/edid.h
@@ -262,6 +262,10 @@
 #define MAX_H (_MAX_H(c) + _MAX_H_OFFSET(c))
 #define _MAX_CLOCK(x) x[9]
 #define MAX_CLOCK _MAX_CLOCK(c)
+#define _DEFAULT_GTF(x) (x[10] == 0x00)
+#define DEFAULT_GTF _DEFAULT_GTF(c)
+#define _RANGE_LIMITS_ONLY(x) (x[10] == 0x01)
+#define RANGE_LIMITS_ONLY _RANGE_LIMITS_ONLY(c)
 #define _HAVE_2ND_GTF(x) (x[10] == 0x02)
 #define HAVE_2ND_GTF _HAVE_2ND_GTF(c)
 #define _F_2ND_GTF(x) (x[12] * 2)
@@ -490,6 +494,8 @@ struct monitor_ranges {
 int gtf_2nd_j;
 int max_clock_khz;
 int maxwidth;   /* in pixels */
+BOOL supported_default_gtf;
+BOOL range_limits_only;
 char supported_aspect;
 char preferred_aspect;
 char supported_blanking;
diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
index 17a8f81c0..c9f0b0ec3 100644
--- a/hw/xfree86/ddc/interpret_edid.c
+++ b/hw/xfree86/ddc/interpret_edid.c
@@ -672,7 +672,19 @@ get_monitor_ranges(Uchar * c, struct monitor_ranges *r)
 r->max_clock = 0;
 if (MAX_CLOCK != 0xff)  /* is specified? */
 r->max_clock = MAX_CLOCK * 10 + 5;
+
+if (DEFAULT_GTF) {
+   r->supported_default_gtf = TRUE;
+} else
+   r->supported_default_gtf = FALSE;
+
+if (RANGE_LIMITS_ONLY) {
+   r->range_limits_only = TRUE;
+} else
+   r->range_limits_only = FALSE;
+
 if (HAVE_2ND_GTF) {
+r->supported_default_gtf = TRUE;
 r->gtf_2nd_f = F_2ND_GTF;
 r->gtf_2nd_c = C_2ND_GTF;
 r->gtf_2nd_m = M_2ND_GTF;
@@ -751,6 +763,29 @@ validate_version(int scrnIndex, struct edid_version *r)
 return TRUE;
 }

+Bool
+gtf_supported(int scrnIndex, xf86MonPtr mon)
+{
+int i;
+
+if (!mon)
+return FALSE;
+
+if (mon->ver.revision < 4) {
+if (GTF_SUPPORTED(mon->features.msc))
+   return TRUE;
+} else {
+for (i = 0; i < DET_TIMINGS; i++) {
+struct detailed_monitor_section *det_timing_des = 
&(mon->det_mon[i]);
+if (det_timing_des && (det_timing_des->type == DS_RANGES) &&
+det_timing_des->section.ranges.supported_default_gtf)
+   return TRUE;
+   }
+}
+
+return FALSE;
+}
+
 /*
  * Returns true if HDMI, false if definitely not or unknown.
  */
diff --git a/hw/xfree86/ddc/xf86DDC.h b/hw/xfree86/ddc/xf86DDC.h
index 7d81ab911..2fa4ba55a 100644
--- a/hw/xfree86/ddc/xf86DDC.h
+++ b/hw/xfree86/ddc/xf86DDC.h
@@ -48,6 +48,9 @@ extern _X_EXPORT Bool xf86SetDDCproperties(ScrnInfoPtr 
pScreen, xf86MonPtr DDC);
 extern _X_EXPORT Bool
  xf86MonitorIsHDMI(xf86MonPtr mon);

+extern _X_EXPORT Bool
+gtf_supported(int scrnIndex, xf86MonPtr mon);
+
 extern _X_EXPORT DisplayModePtr
 FindDMTMode(int hsize, int vsize, int refresh, Bool rb);

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 8e6b697c4..031e5efea 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -2459,7 +2459,7 @@ drmmode_output_add_gtf_modes(xf86OutputPtr output, 
DisplayModePtr Modes)
 int max_x = 0, max_y = 0;
 float max_vrefresh = 0.0;

-if (mon && GTF_SUPPORTED(mon->features.msc))
+if (mon && gtf_supported(output->scrn->scrnIndex, mon))
 return Modes;

nit picking:
no need to check mon gtf_supported() will return FALSE if mon==NULL

re,
 wh

 if (!has_panel_fitter(output))
--
2.26.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

AW: AW V2: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

[Walter Harms]

Von: Aaron Ma 
Gesendet: Dienstag, 14. April 2020 10:30
An: Walter Harms; xorg-devel@lists.x.org
Betreff: Re: AW V2: [PATCH xserver] xfree86: Refine drm modes on continuous 
freq panel

On 4/13/20 5:11 PM, Walter Harms wrote:
>
>
> 
> Von: xorg-devel  im Auftrag von Aaron Ma 
> 
> Gesendet: Samstag, 11. April 2020 12:14
> An: xorg-devel@lists.x.org; aaron...@canonical.com
> Betreff: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel
>
> EDID1.4 replaced GTF Bit with Continuous or Non-Continuous Frequency Display.
>
> Check the "Display Range Limits Descriptor" for GTF support.
> If panel doesn't support GTF, then add gtf modes.
>
> Otherwise X will only show the modes in "Detailed Timing Descriptor".
>
> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/313
> Signed-off-by: Aaron Ma 
> ---
>  hw/xfree86/ddc/edid.h |  6 
>  hw/xfree86/ddc/interpret_edid.c   | 32 +++
>  hw/xfree86/ddc/xf86DDC.h  |  3 ++
>  .../drivers/modesetting/drmmode_display.c |  2 +-
>  4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xfree86/ddc/edid.h b/hw/xfree86/ddc/edid.h
> index 750e4270b..dffcb3749 100644
> --- a/hw/xfree86/ddc/edid.h
> +++ b/hw/xfree86/ddc/edid.h
> @@ -262,6 +262,10 @@
>  #define MAX_H (_MAX_H(c) + _MAX_H_OFFSET(c))
>  #define _MAX_CLOCK(x) x[9]
>  #define MAX_CLOCK _MAX_CLOCK(c)
> +#define _DEFAULT_GTF(x) (x[10] == 0x00)
> +#define DEFAULT_GTF _DEFAULT_GTF(c)
> +#define _RANGE_LIMITS_ONLY(x) (x[10] == 0x01)
> +#define RANGE_LIMITS_ONLY _RANGE_LIMITS_ONLY(c)
>  #define _HAVE_2ND_GTF(x) (x[10] == 0x02)
>  #define HAVE_2ND_GTF _HAVE_2ND_GTF(c)
>  #define _F_2ND_GTF(x) (x[12] * 2)
> @@ -490,6 +494,8 @@ struct monitor_ranges {
>  int gtf_2nd_j;
>  int max_clock_khz;
>  int maxwidth;   /* in pixels */
> +BOOL supported_default_gtf;
> +BOOL range_limits_only;
>  char supported_aspect;
>  char preferred_aspect;
>  char supported_blanking;
> diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
> index 17a8f81c0..4f3cbc587 100644
> --- a/hw/xfree86/ddc/interpret_edid.c
> +++ b/hw/xfree86/ddc/interpret_edid.c
> @@ -672,7 +672,19 @@ get_monitor_ranges(Uchar * c, struct monitor_ranges *r)
>  r->max_clock = 0;
>  if (MAX_CLOCK != 0xff)  /* is specified? */
>  r->max_clock = MAX_CLOCK * 10 + 5;
> +
> +if (DEFAULT_GTF) {
> +   r->supported_default_gtf = TRUE;
> +} else
> +   r->supported_default_gtf = FALSE;
> +
> +if (RANGE_LIMITS_ONLY) {
> +   r->range_limits_only = TRUE;
> +} else
> +   r->range_limits_only = FALSE;
> +
>  if (HAVE_2ND_GTF) {
> +r->supported_default_gtf = TRUE;
>  r->gtf_2nd_f = F_2ND_GTF;
>  r->gtf_2nd_c = C_2ND_GTF;
>  r->gtf_2nd_m = M_2ND_GTF;
> @@ -751,6 +763,26 @@ validate_version(int scrnIndex, struct edid_version *r)
>  return TRUE;
>  }
>
> +Bool
> +gtf_supported(int scrnIndex, xf86MonPtr mon)
> +{
>
> +struct detailed_monitor_section *det_timing_des = mon->det_mon;
> +int i;
> +
> +if (mon && (mon->ver.revision < 4)) {
>
> can mon be NULL ? if yes det_timing_des = mon->det_mon; will crash.
> a simple
>if (!mon)
>  return FALSE;
>
> clears the fog.

Will change in V2.


>
> +if (GTF_SUPPORTED(mon->features.msc))
> +   return TRUE;
> +} else {
>
> maybe you can improve readability this way:
>
>  if ( mon->ver.revision < 4 ) {
>  if (GTF_SUPPORTED(mon->features.msc))
>return TRUE;
>   else
> return FALSE;
>}

Only return TRUE in if condition.
Default return FALSE at the function end.


so far i understand there is no need to loop if you have Version < 4
so you can return early.

happy hacking
re,
 wh

>
>
>
>
>
> +for (i = 0; i < DET_TIMINGS; det_timing_des = &(mon->det_mon[++i])) {
> +   if ((det_timing_des) && (det_timing_des->type == DS_RANGES))
>
> using a simple loop makes it more readable, and avoids the init at start
> (case mon==NULL). The scope of det_timing_des is small you could call it dms 
> or so.
>
>  for(i=0;i < DET_TIMINGS;i++)
>   {
>   struct detailed_monitor_section *det_timing_des = 
> &(mon->det_mon[i]))

Change in V2.

>if (! det_timing_des)

AW V2: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

[Walter Harms]

Von: xorg-devel  im Auftrag von Aaron Ma 

Gesendet: Samstag, 11. April 2020 12:14
An: xorg-devel@lists.x.org; aaron...@canonical.com
Betreff: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

EDID1.4 replaced GTF Bit with Continuous or Non-Continuous Frequency Display.

Check the "Display Range Limits Descriptor" for GTF support.
If panel doesn't support GTF, then add gtf modes.

Otherwise X will only show the modes in "Detailed Timing Descriptor".

BugLink: https://gitlab.freedesktop.org/drm/intel/issues/313
Signed-off-by: Aaron Ma 
---
 hw/xfree86/ddc/edid.h |  6 
 hw/xfree86/ddc/interpret_edid.c   | 32 +++
 hw/xfree86/ddc/xf86DDC.h  |  3 ++
 .../drivers/modesetting/drmmode_display.c |  2 +-
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/ddc/edid.h b/hw/xfree86/ddc/edid.h
index 750e4270b..dffcb3749 100644
--- a/hw/xfree86/ddc/edid.h
+++ b/hw/xfree86/ddc/edid.h
@@ -262,6 +262,10 @@
 #define MAX_H (_MAX_H(c) + _MAX_H_OFFSET(c))
 #define _MAX_CLOCK(x) x[9]
 #define MAX_CLOCK _MAX_CLOCK(c)
+#define _DEFAULT_GTF(x) (x[10] == 0x00)
+#define DEFAULT_GTF _DEFAULT_GTF(c)
+#define _RANGE_LIMITS_ONLY(x) (x[10] == 0x01)
+#define RANGE_LIMITS_ONLY _RANGE_LIMITS_ONLY(c)
 #define _HAVE_2ND_GTF(x) (x[10] == 0x02)
 #define HAVE_2ND_GTF _HAVE_2ND_GTF(c)
 #define _F_2ND_GTF(x) (x[12] * 2)
@@ -490,6 +494,8 @@ struct monitor_ranges {
 int gtf_2nd_j;
 int max_clock_khz;
 int maxwidth;   /* in pixels */
+BOOL supported_default_gtf;
+BOOL range_limits_only;
 char supported_aspect;
 char preferred_aspect;
 char supported_blanking;
diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
index 17a8f81c0..4f3cbc587 100644
--- a/hw/xfree86/ddc/interpret_edid.c
+++ b/hw/xfree86/ddc/interpret_edid.c
@@ -672,7 +672,19 @@ get_monitor_ranges(Uchar * c, struct monitor_ranges *r)
 r->max_clock = 0;
 if (MAX_CLOCK != 0xff)  /* is specified? */
 r->max_clock = MAX_CLOCK * 10 + 5;
+
+if (DEFAULT_GTF) {
+   r->supported_default_gtf = TRUE;
+} else
+   r->supported_default_gtf = FALSE;
+
+if (RANGE_LIMITS_ONLY) {
+   r->range_limits_only = TRUE;
+} else
+   r->range_limits_only = FALSE;
+
 if (HAVE_2ND_GTF) {
+r->supported_default_gtf = TRUE;
 r->gtf_2nd_f = F_2ND_GTF;
 r->gtf_2nd_c = C_2ND_GTF;
 r->gtf_2nd_m = M_2ND_GTF;
@@ -751,6 +763,26 @@ validate_version(int scrnIndex, struct edid_version *r)
 return TRUE;
 }

+Bool
+gtf_supported(int scrnIndex, xf86MonPtr mon)
+{

+struct detailed_monitor_section *det_timing_des = mon->det_mon;
+int i;
+
+if (mon && (mon->ver.revision < 4)) {

can mon be NULL ? if yes det_timing_des = mon->det_mon; will crash.
a simple
   if (!mon)
 return FALSE;

clears the fog.

+if (GTF_SUPPORTED(mon->features.msc))
+   return TRUE;
+} else {

maybe you can improve readability this way:

 if ( mon->ver.revision < 4 ) {
 if (GTF_SUPPORTED(mon->features.msc))
   return TRUE;
  else
return FALSE;
   }





+for (i = 0; i < DET_TIMINGS; det_timing_des = &(mon->det_mon[++i])) {
+   if ((det_timing_des) && (det_timing_des->type == DS_RANGES))

using a simple loop makes it more readable, and avoids the init at start
(case mon==NULL). The scope of det_timing_des is small you could call it dms or 
so.

 for(i=0;i < DET_TIMINGS;i++) 
  {
  struct detailed_monitor_section *det_timing_des = 
&(mon->det_mon[i]))
   if (! det_timing_des)// is 
this possible ?
 return FALSE;
if ( det_timing_des->type == DS_RANGES && 
   det_timing_des->section.ranges.supported_default_gtf )
return TRUE;

 }

hope that helps,
re,
 wh
+   if (det_timing_des->section.ranges.supported_default_gtf)
+   return TRUE;
+   }
+}
+
+return FALSE;
+}
+
 /*
  * Returns true if HDMI, false if definitely not or unknown.
  */
diff --git a/hw/xfree86/ddc/xf86DDC.h b/hw/xfree86/ddc/xf86DDC.h
index 7d81ab911..2fa4ba55a 100644
--- a/hw/xfree86/ddc/xf86DDC.h
+++ b/hw/xfree86/ddc/xf86DDC.h
@@ -48,6 +48,9 @@ extern _X_EXPORT Bool xf86SetDDCproperties(ScrnInfoPtr 
pScreen, xf86MonPtr DDC);
 extern _X_EXPORT Bool
  xf86MonitorIsHDMI(xf86MonPtr mon);

+extern _X_EXPORT Bool
+gtf_supported(int scrnIndex, xf86MonPtr mon);
+
 extern _X_EXPORT DisplayModePtr
 FindDMTMode(int hsize, int vsize, int refresh, Bool rb);

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 8e6b697c4..031e5efea 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ 

AW: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

[Walter Harms]

Von: xorg-devel  im Auftrag von Aaron Ma 

Gesendet: Samstag, 11. April 2020 12:14
An: xorg-devel@lists.x.org; aaron...@canonical.com
Betreff: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

EDID1.4 replaced GTF Bit with Continuous or Non-Continuous Frequency Display.

Check the "Display Range Limits Descriptor" for GTF support.
If panel doesn't support GTF, then add gtf modes.

Otherwise X will only show the modes in "Detailed Timing Descriptor".

BugLink: https://gitlab.freedesktop.org/drm/intel/issues/313
Signed-off-by: Aaron Ma 
---
 hw/xfree86/ddc/edid.h |  6 
 hw/xfree86/ddc/interpret_edid.c   | 32 +++
 hw/xfree86/ddc/xf86DDC.h  |  3 ++
 .../drivers/modesetting/drmmode_display.c |  2 +-
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/ddc/edid.h b/hw/xfree86/ddc/edid.h
index 750e4270b..dffcb3749 100644
--- a/hw/xfree86/ddc/edid.h
+++ b/hw/xfree86/ddc/edid.h
@@ -262,6 +262,10 @@
 #define MAX_H (_MAX_H(c) + _MAX_H_OFFSET(c))
 #define _MAX_CLOCK(x) x[9]
 #define MAX_CLOCK _MAX_CLOCK(c)
+#define _DEFAULT_GTF(x) (x[10] == 0x00)
+#define DEFAULT_GTF _DEFAULT_GTF(c)
+#define _RANGE_LIMITS_ONLY(x) (x[10] == 0x01)
+#define RANGE_LIMITS_ONLY _RANGE_LIMITS_ONLY(c)
 #define _HAVE_2ND_GTF(x) (x[10] == 0x02)
 #define HAVE_2ND_GTF _HAVE_2ND_GTF(c)
 #define _F_2ND_GTF(x) (x[12] * 2)
@@ -490,6 +494,8 @@ struct monitor_ranges {
 int gtf_2nd_j;
 int max_clock_khz;
 int maxwidth;   /* in pixels */
+BOOL supported_default_gtf;
+BOOL range_limits_only;
 char supported_aspect;
 char preferred_aspect;
 char supported_blanking;
diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
index 17a8f81c0..4f3cbc587 100644
--- a/hw/xfree86/ddc/interpret_edid.c
+++ b/hw/xfree86/ddc/interpret_edid.c
@@ -672,7 +672,19 @@ get_monitor_ranges(Uchar * c, struct monitor_ranges *r)
 r->max_clock = 0;
 if (MAX_CLOCK != 0xff)  /* is specified? */
 r->max_clock = MAX_CLOCK * 10 + 5;
+
+if (DEFAULT_GTF) {
+   r->supported_default_gtf = TRUE;
+} else
+   r->supported_default_gtf = FALSE;
+
+if (RANGE_LIMITS_ONLY) {
+   r->range_limits_only = TRUE;
+} else
+   r->range_limits_only = FALSE;
+
 if (HAVE_2ND_GTF) {
+r->supported_default_gtf = TRUE;
 r->gtf_2nd_f = F_2ND_GTF;
 r->gtf_2nd_c = C_2ND_GTF;
 r->gtf_2nd_m = M_2ND_GTF;
@@ -751,6 +763,26 @@ validate_version(int scrnIndex, struct edid_version *r)
 return TRUE;
 }

+Bool
+gtf_supported(int scrnIndex, xf86MonPtr mon)
+{
+struct detailed_monitor_section *det_timing_des = mon->det_mon;
+int i;
+
+if (mon && (mon->ver.revision < 4)) {

can mon be NULL ? if yes det_timing_des = mon->det_mon; will crash.

+if (GTF_SUPPORTED(mon->features.msc))
+   return TRUE;
+} else {

maybe you can improve readability this way:

 if ( mon->ver.revision < 4 ) {
 if (GTF_SUPPORTED(mon->features.msc))





+for (i = 0; i < DET_TIMINGS; det_timing_des = &(mon->det_mon[++i])) {
+   if ((det_timing_des) && (det_timing_des->type == DS_RANGES))
+   if (det_timing_des->section.ranges.supported_default_gtf)
+   return TRUE;
+   }
+}
+
+return FALSE;
+}
+
 /*
  * Returns true if HDMI, false if definitely not or unknown.
  */
diff --git a/hw/xfree86/ddc/xf86DDC.h b/hw/xfree86/ddc/xf86DDC.h
index 7d81ab911..2fa4ba55a 100644
--- a/hw/xfree86/ddc/xf86DDC.h
+++ b/hw/xfree86/ddc/xf86DDC.h
@@ -48,6 +48,9 @@ extern _X_EXPORT Bool xf86SetDDCproperties(ScrnInfoPtr 
pScreen, xf86MonPtr DDC);
 extern _X_EXPORT Bool
  xf86MonitorIsHDMI(xf86MonPtr mon);

+extern _X_EXPORT Bool
+gtf_supported(int scrnIndex, xf86MonPtr mon);
+
 extern _X_EXPORT DisplayModePtr
 FindDMTMode(int hsize, int vsize, int refresh, Bool rb);

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 8e6b697c4..031e5efea 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -2459,7 +2459,7 @@ drmmode_output_add_gtf_modes(xf86OutputPtr output, 
DisplayModePtr Modes)
 int max_x = 0, max_y = 0;
 float max_vrefresh = 0.0;

-if (mon && GTF_SUPPORTED(mon->features.msc))
+if (mon && gtf_supported(output->scrn->scrnIndex, mon))
 return Modes;

 if (!has_panel_fitter(output))
--
2.26.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Flush buffer in time for global (un)lock xserver operation

[walter harms]

Am 12.12.2019 03:13, schrieb Chen Gang:
>> On Wed, 2019-12-04 at 18:24 +0800, cheng...@emindsoft.com.cn wrote:
>>> From: Chen Gang 
>>>
>>> The global (un)lock xserver operations will have effect to the whole
>>> system, and all the other gui apps have to be blocked, so the operations
>>> should not be buffered, or may cause sync issue, e.g. reboot machine.
>>
>> Enh. I can maybe see the point of this for XUngrabServer, but I think
>> doing this for XGrabServer may actually make interactivity worse. With
>> the code as it is currently, the GrabServer request will be buffered
>> inside the client until something happens to flush it out (presumably
>> something like QueryTree or GetImage that provokes a reply), and while
>> it stays buffered other clients may still interact with the server.
>> With this change you'll immediately write the request to the server,
>> and you might lose your scheduler timeslice as a result, in which case
>> not only will the grabbing client need to wait to reschedule but every
>> other client will need to wait in line behind the grabbing client.
>>
> 
> Oh, for me, what you said is reasonable. I got the issue because the
> XUngrabServer did not call _XFlush() immediately.
> 
> _XFlush for XGrabServer is not necesscary for my current issue, either
> not good for the performance.
> 
> Originally, I felt XGrabServer and XUngrabServer were a pair, and I was
> not quite sure enought. So I used _XFlush for both of them (it must be
> safe enough, altough it has negative effect with the performance).
> 
>> Do you have a scenario in mind that this helps with? Does doing _XFlush
>> only for XUngrabServer work just as well?
>>
> 
> At least for me, _XFlush only for XUngrabServer should be work well.
> 
> Welcome you to apply the patch with the new modification (only _XFlush
> for UngrabServer).
> 

short version,
  can you provide an example works.c vs. not_works.c ?

re,
 wh



> By the way, I can not join in xorg-devel@lists.x.org mailing list. I
> tried, but got "No ReCAPTCHA response provided". Welcome any ideas about it.
> 
> Thanks.
> 
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] libx11-1.6.7: Flush buffer in time for global (un)lock xserver operation

[walter harms]

Am 27.11.2019 04:52, schrieb Chen Gang:
> Oh, sorry, XFlush should be replaced by _XFlush.
> 

it looks reasonable,

NTL can you provide an example works.c vs. not_works.c ?

re,
 wh

> On 2019/11/27 上午11:47, Chen Gang wrote:
>> The global (un)lock xserver operations will have effect to the whole
>> system, and all the other gui apps have to be blocked, so the operations
>> should not be buffered, or may cause sync issue, e.g. reboot machine.
>>
>> Signed-off-by: Chen Gang 
>> ---
>>  src/GrServer.c  | 1 +
>>  src/UngrabSvr.c | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/src/GrServer.c b/src/GrServer.c
>> index c4c62be..59ed03a 100644
>> --- a/src/GrServer.c
>> +++ b/src/GrServer.c
>> @@ -35,6 +35,7 @@ XGrabServer (register Display *dpy)
>>  _X_UNUSED register xReq *req;
>>  LockDisplay(dpy);
>>  GetEmptyReq(GrabServer, req);
>> +XFlush(dpy); /* For Global (un)lock ops must be sent quickly */
>>  UnlockDisplay(dpy);
>>  SyncHandle();
>>  return 1;
>> diff --git a/src/UngrabSvr.c b/src/UngrabSvr.c
>> index 20ad9aa..4b0ffca 100644
>> --- a/src/UngrabSvr.c
>> +++ b/src/UngrabSvr.c
>> @@ -37,6 +37,7 @@ XUngrabServer (
>>
>>  LockDisplay(dpy);
>>  GetEmptyReq(UngrabServer, req);
>> +   XFlush(dpy); /* For Global (un)lock ops must be sent quickly */
>>  UnlockDisplay(dpy);
>>  SyncHandle();
>>  return 1;
>>
> 
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xev] parse state to describe in-use modifiers

[walter harms]

Am 14.08.2019 04:36, schrieb Benjamin Elijah Griffin:
> Example for :
> 
> KeyPress event, serial 28, synthetic NO, window 0x2a1,
> root 0x176, subw 0x0, time 605237548, (-825,547), root:(303,559),
> state 0x4, keycode 54 (keysym 0x63, c), same_screen YES,
> modifier: control
> XLookupString gives 1 bytes: (03) ""
> XmbLookupString gives 1 bytes: (03) ""
> XFilterEvent returns: False
> 
> Signed-off-by: Benjamin Elijah Griffin 
> ---
>  configure.ac |  2 +-
>  xev.c| 47 +++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 6adaa43..72d22b4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -22,7 +22,7 @@ dnl Process this file with autoconf to create configure.
> 
>  # Initialize Autoconf
>  AC_PREREQ([2.60])
> -AC_INIT([xev], [1.2.3],
> +AC_INIT([xev], [1.2.4],
>  [https://gitlab.freedesktop.org/xorg/app/xev/issues], [xev])
>  AC_CONFIG_SRCDIR([Makefile.am])
>  AC_CONFIG_HEADERS([config.h])
> diff --git a/xev.c b/xev.c
> index 4c6115f..f5cc32b 100644
> --- a/xev.c
> +++ b/xev.c
> @@ -106,6 +106,43 @@ dump(char *str, int len)
>  }
> 
>  static void
> +show_Modifiers(int state)
> +{
> +int i, smask, sflag = 1;
> +
> +printf("modifier: ");
> +if (state & sflag)
> +printf("shift ");
> +sflag <<= 1;
> +if (state & sflag)
> +printf("lock ");
> +sflag <<= 1;
> +if (state & sflag)
> +printf("control ");
> +sflag <<= 1;
> +
> +for (i = 3; i < 8; i++) {
> +   if (state & sflag)
> +printf("mod%d ", i - 2);
> +sflag <<= 1;
> +}
> +
> +/* Mouse buttons are also modifiers */
> +for (i = 8; i < 13; i++) {
> +   if (state & sflag)
> +printf("Button%d ", i - 7);
> +sflag <<= 1;
> +}
> +
> +/* Any bits in state that we didn't already test? */
> +smask = ~ (sflag - 1);
> +if (state & smask)
> +printf("other: 0x%x", (state & smask));
> +
> +printf("\n");
> +}
> +
> +static void
>  do_KeyPress(XEvent *eventp)
>  {
>  XKeyEvent *e = (XKeyEvent *) eventp;
> @@ -151,6 +188,10 @@ do_KeyPress(XEvent *eventp)
>  printf("state 0x%x, keycode %u (keysym 0x%lx, %s), same_screen 
> %s,\n",
> e->state, e->keycode, (unsigned long) ks, ksname,
> e->same_screen ? Yes : No);
> +if (e->state) {
> +show_Modifiers(e->state);
> +}
> +
>  if (kc_set && e->keycode != kc)
>  printf("XKeysymToKeycode returns keycode: %u\n", kc);
>  if (nbytes < 0)
> @@ -198,6 +239,9 @@ do_ButtonPress(XEvent *eventp)
> e->root, e->subwindow, e->time, e->x, e->y, e->x_root, e->y_root);
>  printf("state 0x%x, button %u, same_screen %s\n",
> e->state, e->button, e->same_screen ? Yes : No);
> +if (e->state) {
> +show_Modifiers(e->state);
> +}
>  }
> 

You can make you life more easy when you move the check into show_Modifiers()
perhaps this is a case for "none" ?

JM2C

re,
 wh


>  static void
> @@ -215,6 +259,9 @@ do_MotionNotify(XEvent *eventp)
> e->root, e->subwindow, e->time, e->x, e->y, e->x_root, e->y_root);
>  printf("state 0x%x, is_hint %u, same_screen %s\n",
> e->state, e->is_hint, e->same_screen ? Yes : No);
> +if (e->state) {
> +show_Modifiers(e->state);
> +}
>  }
> 
>  static void
> --
> 2.7.4
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-cirrus] Declare an I2C bus name string as a static type for alp_i2c.c (walter harms)

[walter harms]

Am 19.07.2019 01:37, schrieb Kevin Brace:
> Hi Walter,
> 
> I am attempting to eliminate compilation warnings from the code.
> The previous code causes "const char*" to "char*" compilation warnings.
> That's all.
> 

ok, i understand
please add this to your patch, it will show up in the log and a seconde
person has a chance to figure out what was going on.

re,
 wh

> Regards,
> 
> Kevin Brace
> Brace Computer Laboratory blog
> https://bracecomputerlab.com
> 
> 
>> Date: Thu, 18 Jul 2019 09:02:52 +0200
>> From: walter harms 
>> To: xorg-devel@lists.x.org
>> Subject: Re: [PATCH xf86-video-cirrus] Declare an I2C bus name string
>>  as a static type for alp_i2c.c
>> Message-ID: <5d30199c.6060...@bfs.de>
>> Content-Type: text/plain; charset=UTF-8
>>
>> HI,
>> thx for your patch,
>> would you like to explain why ?
>> what problem do you try to solve ?
>>
>>
>> re,
>>  wh
>>
>> Am 18.07.2019 04:55, schrieb Kevin Brace:
>>> Signed-off-by: Kevin Brace 
>>> ---
>>>  src/alp_i2c.c | 9 +++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/alp_i2c.c b/src/alp_i2c.c
>>> index 9341543..b52f2c8 100644
>>> --- a/src/alp_i2c.c
>>> +++ b/src/alp_i2c.c
>>> @@ -16,6 +16,11 @@
>>>  #define _ALP_PRIVATE_
>>>  #include "alp.h"
>>>
>>> +
>>> +static char strI2CBus1[]   = "I2C bus 1";
>>> +static char strI2CBus2[]   = "I2C bus 2";
>>> +
>>> +
>>>  /*
>>>   * Switch between internal I2C bus and external (DDC) bus.
>>>   * There is one I2C port controlled bu SR08 and the programmable
>>> @@ -99,7 +104,7 @@ AlpI2CInit(ScrnInfoPtr pScrn)
>>>
>>> pCir->I2CPtr1 = I2CPtr;
>>>
>>> -   I2CPtr->BusName= "I2C bus 1";
>>> +   I2CPtr->BusName= strI2CBus1;
>>> I2CPtr->scrnIndex  = pScrn->scrnIndex;
>>> I2CPtr->I2CPutBits = AlpI2CPutBits;
>>> I2CPtr->I2CGetBits = AlpI2CGetBits;
>>> @@ -113,7 +118,7 @@ AlpI2CInit(ScrnInfoPtr pScrn)
>>>
>>> pCir->I2CPtr2 = I2CPtr;
>>>
>>> -   I2CPtr->BusName= "I2C bus 2";
>>> +   I2CPtr->BusName= strI2CBus2;
>>> I2CPtr->scrnIndex  = pScrn->scrnIndex;
>>> I2CPtr->I2CPutBits = AlpI2CPutBits;
>>> I2CPtr->I2CGetBits = AlpI2CGetBits;
>>> --
>>> 2.17.1
>>>
>>> ___
>>> xorg-devel@lists.x.org: X.Org development
>>> Archives: http://lists.x.org/archives/xorg-devel
>>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>
>>
>> --
>>
>> Subject: Digest Footer
>>
>> ___
>> xorg-devel mailing list
>> xorg-devel@lists.x.org
>> https://lists.x.org/mailman/listinfo/xorg-devel
>>
>> --
>>
>> End of xorg-devel Digest, Vol 126, Issue 8
>> **
>>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-cirrus] Declare an I2C bus name string as a static type for alp_i2c.c

[walter harms]

HI,
thx for your patch,
would you like to explain why ?
what problem do you try to solve ?


re,
 wh

Am 18.07.2019 04:55, schrieb Kevin Brace:
> Signed-off-by: Kevin Brace 
> ---
>  src/alp_i2c.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/alp_i2c.c b/src/alp_i2c.c
> index 9341543..b52f2c8 100644
> --- a/src/alp_i2c.c
> +++ b/src/alp_i2c.c
> @@ -16,6 +16,11 @@
>  #define _ALP_PRIVATE_
>  #include "alp.h"
> 
> +
> +static char strI2CBus1[] = "I2C bus 1";
> +static char strI2CBus2[] = "I2C bus 2";
> +
> +
>  /*
>   * Switch between internal I2C bus and external (DDC) bus.
>   * There is one I2C port controlled bu SR08 and the programmable
> @@ -99,7 +104,7 @@ AlpI2CInit(ScrnInfoPtr pScrn)
> 
>   pCir->I2CPtr1 = I2CPtr;
> 
> - I2CPtr->BusName= "I2C bus 1";
> + I2CPtr->BusName= strI2CBus1;
>   I2CPtr->scrnIndex  = pScrn->scrnIndex;
>   I2CPtr->I2CPutBits = AlpI2CPutBits;
>   I2CPtr->I2CGetBits = AlpI2CGetBits;
> @@ -113,7 +118,7 @@ AlpI2CInit(ScrnInfoPtr pScrn)
> 
>   pCir->I2CPtr2 = I2CPtr;
> 
> - I2CPtr->BusName= "I2C bus 2";
> + I2CPtr->BusName= strI2CBus2;
>   I2CPtr->scrnIndex  = pScrn->scrnIndex;
>   I2CPtr->I2CPutBits = AlpI2CPutBits;
>   I2CPtr->I2CGetBits = AlpI2CGetBits;
> --
> 2.17.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: X.Org modules which could use some help to release

[walter harms]

Am 15.07.2019 13:02, schrieb Thomas Klausner:
> On Sun, Jul 14, 2019 at 06:34:20PM -0700, Alan Coopersmith wrote:
>> These I skipped because we don't package them in Solaris and I don't know if
>> anyone else still uses them or they should just be archived now:
>>   - app/beforelight
>>   - app/fdclock
>>   - app/mdm
>>   - app/rstart
>>   - app/scripts
>>   - app/xf86dga
>>   - app/xfwp
>>   - app/xvidtune
>>   - app/xcb-demo
>>   - driver/xf86-video-ark
>>   - driver/xf86-video-armsoc
>>   - driver/xf86-video-impact
>>   - driver/xf86-video-newport
>>   - driver/xf86-video-nested  (has never had a release?)
>>   - driver/xf86-video-tga
>>   - driver/xf86-video-tseng
>>   - driver/xf86-video-xgi
>>   - driver/xf86-video-xgixp
>>   - lib/libWindowsWM
>>   - lib/libXTrap
>>   - util/gccmakedep
>>   - util/install-check
> 
> As a datapoint:
> 
> Of these, the following have packages in pkgsrc:
> 
> beforelight
> gccmakedep
> libWindowsWM
> libXTrap
> rstart
> xf86-video-ark
> xf86-video-newport
> xf86-video-tga
> xf86-video-tseng
> xf86-video-xgi
> xf86dga
> xfwp
> xvidtune
> 
> and the following:
> 
> beforelight
> xf86-video-ark
> xf86-video-newport
> xf86-video-tga
> xf86-video-tseng
> xf86-video-xgi
> xf86dga
> xfwp
> xvidtune
> 
> are included in NetBSD's xsrc, probably some with patches.
> 


BSD only patches, or something you can feed back ?

re,
 wh

> I can't tell if they are there for historic reasons or if anyone is
> actually still using them.
>  Thomas
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: X.Org modules which could use some help to release

[walter harms]

Am 15.07.2019 03:34, schrieb Alan Coopersmith:
> As you may have noticed, I've been working through the various modules to make
> releases of those with significant enough change to justify a release (i.e.
> more than just the autogen.sh cleanups & gitlab README/configure.ac updates).
> 
> There's some modules which have more than enough change that I've left for
> now for other reasons, which others could help with:
> 
> app/mkcomposecache:
>   - Does anyone actually use this?  I think the code to read the files is
> in Xlib, but do packagers ship this to build the files?
> 
> app/twm:
>   - Needs someone to evaluate:
> https://gitlab.freedesktop.org/xorg/app/twm/issues/7
> https://patchwork.freedesktop.org/project/Xorg/list/?q=twm
> patches in https://gitlab.freedesktop.org/xorg/app/twm/issues
> 
> app/xkbcomp:
>   - Needs someone to evaluate:
> https://patchwork.freedesktop.org/project/Xorg/list/?q=xkbcomp
> https://gitlab.freedesktop.org/xorg/app/xkbcomp/issues
> 
> app/xkbutils:
>   - Needs someone to evaluate:
> patch in https://gitlab.freedesktop.org/xorg/app/xkbutils/issues/1
> 
> app/xrandr:
>   - Needs someone to evaluate:
> https://gitlab.freedesktop.org/xorg/app/xrandr/merge_requests/1
> https://patchwork.freedesktop.org/project/Xorg/list/?q=xrandr
> 
> app/xrestop:
>   - Are we shipping this as an X.Org project now despite the GPL license?
>   - Previous releases are on yoctoproject.org, not xorg.freedesktop.org -
> do we care about that?
> 

i have some cleanup patches on my laptop. Are there patches at yoctoproject.org
that we should pick up ?

re,
 wh

> app/xresponse:
>   - I didn't even know this existed until seeing it in gitlab, but it's
> apparently another GPL project we inherited from openedhand.
> It looks like it's been abandoned since 2007 - does anyone use it
> or should it just be archived now?
> 
> app/xscope:
>   - I pushed the fixes to show peer process info on Linux & Solaris.
> Does anyone want to provide support for any other platforms before we
> ship this?  See:
> https://lists.x.org/archives/xorg-devel/2019-February/057982.html
> 
> app/xshowdamage:
>   - This appears to be test code, never released as a tarball, but might
> still be useful for debugging, so I don't think it should be archived.
> 
> driver/xf86-input-keyboard:
>   - Needs a FreeBSD person to figure out the correct patch to apply for:
> https://gitlab.freedesktop.org/xorg/driver/xf86-input-keyboard/issues/28
> 
> driver/xf86-video-dummy:
>   - Needs someone to evaluate:
> 
> https://gitlab.freedesktop.org/xorg/driver/xf86-video-dummy/merge_requests/1
> 
> These I left because I believe they have maintainers to handle their releases
> (or to decide whether it's time for a release or not):
>   - app/intel-gpu-tools
>   - app/xinput
>   - driver/xf86-input-joystick
>   - driver/xf86-input-vmmouse
>   - driver/xf86-video-amdgpu
>   - driver/xf86-video-ati
>   - driver/xf86-video-cirrus
>   - driver/xf86-video-fbdev
>   - driver/xf86-video-freedreno
>   - driver/xf86-video-geode
>   - driver/xf86-video-glint
>   - driver/xf86-video-intel
>   - driver/xf86-video-mach64
>   - driver/xf86-video-omap
>   - driver/xf86-video-qxl
>   - driver/xf86-video-rendition
>   - driver/xf86-video-s3
>   - driver/xf86-video-savage
>   - driver/xf86-video-sis
>   - driver/xf86-video-trident
>   - driver/xf86-video-v4l
>   - driver/xf86-video-vesa
>   - driver/xf86-video-vmware
>   - driver/xf86-video-wsfb
>   - lib/libAppleWM
>   - lib/libxrandrutils
>   - lib/libXt
>   - test/rendercheck (besides, I don't know how to make meson-only releases)
> 
> These I skipped because we don't package them in Solaris and I don't know if
> anyone else still uses them or they should just be archived now:
>   - app/beforelight
>   - app/fdclock
>   - app/mdm
>   - app/rstart
>   - app/scripts
>   - app/xf86dga
>   - app/xfwp
>   - app/xvidtune
>   - app/xcb-demo
>   - driver/xf86-video-ark
>   - driver/xf86-video-armsoc
>   - driver/xf86-video-impact
>   - driver/xf86-video-newport
>   - driver/xf86-video-nested  (has never had a release?)
>   - driver/xf86-video-tga
>   - driver/xf86-video-tseng
>   - driver/xf86-video-xgi
>   - driver/xf86-video-xgixp
>   - lib/libWindowsWM
>   - lib/libXTrap
>   - util/gccmakedep
>   - util/install-check
> 
> (Of those, only xfwpm, xvidtune, & elographics have an entry on
>  release-monitoring.org, which is one sign of use in distros,
>  but not definitive.)
> 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: libXt release

[walter harms]

Hello Matt,
Thomas Dickey has an impressive array of systems where he can test.

Is there a way to test on more systems ? We still have a bunch of
#ifdef stuff. I would like see tests on AIX, SUN and WIN32 so we
can figure out what is relevant.

re,
 wh


Am 17.06.2019 02:08, schrieb Thomas Dickey:
> On Thu, Jun 13, 2019 at 03:43:04PM -0400, Thomas Dickey wrote:
>> - Original Message -
>> | From: "Matt Turner" 
>> | To: "Thomas Dickey" 
>> | Cc: "xorg-devel" 
>> | Sent: Thursday, June 13, 2019 1:47:04 PM
>> | Subject: libXt release
>>
>> | Hi Thomas,
>> | 
>> | I'd like to do a tarball release of libXt since there are now quite a
>> | few commits since 1.1.5, released in 2015. Are you okay with me making
>> | a 1.2.0 release now, or is there anything else you would want to get
>> | into a new release?
>>
>> I've been working on some scripts to check for
>> breakage in the specification document - might have some fixes there.
>>
>> I'll review the current state on the weekend, to give better advice.
> 
> I think it's in good shape.  Because of the interface change (using
> const), I was going to suggest that it should be 1.2.0
> 
> I've built the library on a dozen of my VM's, and am able to test with
> most of those.
> 
> I've built/run packages for
>   Debian 8, 9 and testing, as well as
>   Fedora 28, 29 and rawhide,
>   OpenSUSE tumbleweed
>   CentOS 6, 7
>   Scientific Linux 7
> 
> I've also compiled (and am running from) Xt on MacOS 10.13.6,
> and have built/run from Cygwin.  For both of those, I copied
> the shared library to the appropriate place.
> 
> I've built, but don't see how to run... with Solaris 11.4
> (since the existing libraries are linked to an ABI 4 libXt).
> 
> Also, I've built the library with Mageia 6, but the rpm scripts
> aren't up-to-date.
> 
> I've built/run with FreeBSD 12 as well, but just on one-off's
> (haven't done it with the ports scripts).
> 
> Still on my to-do list are NetBSD, OpenBSD and Arch Linux.
> 
> 
> 
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: rfc: WIN32 vs _WIN32 and more

[walter harms]

Am 20.05.2019 21:09, schrieb Jon Turney:
> On 17/05/2019 21:00, Thomas Dickey wrote:
>> On Fri, May 17, 2019 at 10:08:47AM +0200, walter harms wrote:
>>> Hi list,
>>> is there a common ground for using OS related defines ?
>>> I was look at some libs and found some defines that
>>> look pretty ancient. And some like
>>> WIN32 vs _WIN32
>>
>> however, they're distinct:
>>
>> https://stackoverflow.com/questions/662084/whats-the-difference-between-the-win32-and-win32-defines-in-c
>>
> 
> 
> Yeah, it's a total mess.
> 
> IMHO, _WIN32 is the 'correct' thing to test on, since it's in the
> implementation reserved namespace (since it starts with an underscore).


hello,
may goal was it to reduce the number of defines, WIN32 is special because we
have two close related symbols. Can you try to replace WIN32 with _WIN32 and
see what happens ?

re,
 wh


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: rfc: WIN32 vs _WIN32 and more

[walter harms]

Am 17.05.2019 21:51, schrieb Thomas Dickey:
> On Fri, May 17, 2019 at 04:26:10PM +0200, walter harms wrote:
>>
>>
>> Am 17.05.2019 13:01, schrieb James Larrowe:
>>> I use _WIN32 or __WIN32__ depending on the context.
>>>
>> my idea was to reduce the number of defines :)
>>
>> the problem is that i have no way to test what would happen if
>> i replace WIN32 with _WIN32.
>>
>> So the question is left, is WIN32 still used ?
> 
> sure - there was a recent update in Intrinsic.c which relies upon that slice.
> 

Actually that made me investigate the use.
There is a single use for _WIN32 in NextEvent.c
making this to WIN32 would clean the table, but
i can not test.


#ifdef _WIN32
typedef long suseconds_t;
#endif


re,
 wh

> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: rfc: WIN32 vs _WIN32 and more

[walter harms]

Am 17.05.2019 21:57, schrieb Thomas Dickey:
> On Fri, May 17, 2019 at 10:08:47AM +0200, walter harms wrote:
>> Hi list,
>> is there a common ground for using OS related defines ?
>> I was look at some libs and found some defines that
>> look pretty ancient. And some like
>> WIN32 vs _WIN32
>> seems to confuse other people also ( ask you search engine)
>>
>> I found also:
>> ISC  *
> 
> I'll be removing that one.
> 
>> hpux
> 
> That one's for HPUX 9 vs 10 (both old).
> 
> However, before removing code, it helps to read and investigate the
> comment, which says that has to match existing code in Xlib.
> 

I am aware, the point was Alan what said ''We've removed a bunch of code using 
those defines already (I've mostly
used #unifdef to do so, with manual editing only for special cases).

We still need the #ifdefs for sun & sparc, but prefer the __sun & __sparc
forms so they still work in strict standards compliance mode. ''

I was look for a common set of defines and systems that are defined in all 
libraries.
Obviously, it is a for a user no help if FOO is defined in one and removed in 
others.

more over, i would argue for one define per system. If need this can handled if 
#ifdef.
Now i would like to see what ancient code can be removed.

re,
 wh

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: rfc: WIN32 vs _WIN32 and more

[walter harms]

Am 17.05.2019 13:01, schrieb James Larrowe:
> I use _WIN32 or __WIN32__ depending on the context.
> 
my idea was to reduce the number of defines :)

the problem is that i have no way to test what would happen if
i replace WIN32 with _WIN32.

So the question is left, is WIN32 still used ?

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

rfc: WIN32 vs _WIN32 and more

[walter harms]

Hi list,
is there a common ground for using OS related defines ?
I was look at some libs and found some defines that
look pretty ancient. And some like
WIN32 vs _WIN32
seems to confuse other people also ( ask you search engine)

I found also:
ISC  *
MOTOROLA *
VMS*
USG*
hpux
sgi
sparc
sun
ultrix *
__osf__ *


* dead ?

For easier maintainability i would like to remove defines that are not
in use any more.

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Proposed addition to CodingStyle web page about assert(a && b)

[walter harms]

Am 05.05.2019 09:11, schrieb Matthieu Herrb:
> On Sat, May 04, 2019 at 03:47:31PM -0700, Adam Richter wrote:
>> Hi, everyone.
>>
>> I would like to propose that whoever has the ability to edit the web
>> page add a line like the following to
>> https://www.x.org/wiki/CodingStyle/ :
>>
>> - Separate assert(a && b) into assert(a) and assert(b).
>>
>>
>> Thanks in advance for any input on this.
> 
> Hi,
> 
> I'm not sure if this advice belongs to this wiki page which is more
> oriented on the appearance of the code than on semantics or
> development good practices.
> 
> On the development good practices side, I think assert() should be
> banned as much as possible form libraries and drivers.
> 
> You don't know anything about the caller context and having it beeing
> brutally abort()ing is brutal and my lead to security issues
> (data leaks in the core file for instance) or data corruption.
> 
> In libraries assert() should never be used to reject bad user input or
> any other error condition that can happen for some known reason. It
> should really only be used to document conditions that should really
> never happen. In all other cases the function should be able to return
> an error to the caller (which should of course not ignore them).
> 
> 

i do not comment on the use of assert() generally, it can be used
by anyone who likes that. Things are getting problematic when use
like this:

   assert(0 < asprintf(, "%s/Library/Logs/X11", home));

this is simply dangerous as you can define NDEBUG and let everything vanish.

BTW are the libraries routinely compiled with NDEBUG enabled ?

re,
 wh



___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

update

[walter harms]

hi,
es gab ein update für inno.x auf der 183.121.
Sichbar vor allem ist das es nun ein startscript gibt (inno.sh)
das aus/an schalten der sonde erfolgt nun extern im script *immer*
wenn sich das programm beendet hat.

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 4/9] xkbtext: fix copy-paste error

[Walter Harms]

> Konstantin Kharlamov  hat am 24. März 2019 um 00:51
> geschrieben:
> 
> 
> As can be seen in diff, nOut is always 0 here. The code was likely
> copy-pasted from comparisons further below.
> 
> Fixes LGTM warning "Comparison is always false because nOut <= 0."
> 
> Signed-off-by: Konstantin Kharlamov 
> ---
>  xkb/xkbtext.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xkb/xkbtext.c b/xkb/xkbtext.c
> index d2a2567fc..54cca3fb4 100644
> --- a/xkb/xkbtext.c
> +++ b/xkb/xkbtext.c
> @@ -966,7 +966,7 @@ CopySetLockControlsArgs(XkbDescPtr xkb, XkbAction *action,
> char *buf, int *sz)
>  int nOut = 0;
>  
>  if (tmp & XkbRepeatKeysMask) {
> -snprintf(tbuf, sizeof(tbuf), "%sRepeatKeys", (nOut > 0 ? "+" :
> ""));
> +snprintf(tbuf, sizeof(tbuf), "RepeatKeys");
>  TryCopyStr(buf, tbuf, sz);

maybe that can go directly in TryCopyStr() ?

re,
 wh

>  nOut++;
>  }
> -- 
> 2.21.0
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-trident 1/3] Tab to spaces conversion for blade_accel.c

[Walter Harms]

maybe it is cheaper to kill the dead code instead of fixing white space ?

re,
 wh 


> Kevin Brace  hat am 21. Februar 2019 um 20:55 geschrieben:
> 
> 
> Signed-off-by: Kevin Brace 
> ---
>  src/blade_accel.c | 491
> +++---
>  1 file changed, 244 insertions(+), 247 deletions(-)
> 
> diff --git a/src/blade_accel.c b/src/blade_accel.c
> index 1645e1d..86a67a2 100644
> --- a/src/blade_accel.c
> +++ b/src/blade_accel.c
> @@ -45,72 +45,72 @@
>  static void BladeSync(ScrnInfoPtr pScrn);
>  #if 0
>  static void BladeSetupForSolidLine(ScrnInfoPtr pScrn, int color,
> - int rop, unsigned int planemask);
> +int rop, unsigned int planemask);
>  static void BladeSubsequentSolidBresenhamLine(ScrnInfoPtr pScrn,
> - int x, int y, int dmaj, int dmin, int e, 
> - int len, int octant);
> +int x, int y, int dmaj, int dmin, int e,
> +int len, int octant);
>  static void BladeSubsequentSolidTwoPointLine( ScrnInfoPtr pScrn,
> - int x1, int y1, int x2, int y2, int flags); 
> +int x1, int y1, int x2, int y2, int flags);
>  static void BladeSetupForDashedLine(ScrnInfoPtr pScrn, int fg, int bg,
> - int rop, unsigned int planemask, int length,
> - unsigned char *pattern);
> +int rop, unsigned int planemask, int length,
> +unsigned char *pattern);
>  static void BladeSubsequentDashedTwoPointLine( ScrnInfoPtr pScrn,
> - int x1, int y1, int x2, int y2, int flags,
> - int phase); 
> +int x1, int y1, int x2, int y2, int flags,
> +int phase);
>  #endif
>  static void BladeSetupForFillRectSolid(ScrnInfoPtr pScrn, int color,
> - int rop, unsigned int planemask);
> +int rop, unsigned int planemask);
>  static void BladeSubsequentFillRectSolid(ScrnInfoPtr pScrn, int x,
> - int y, int w, int h);
> +int y, int w, int h);
>  static void BladeSubsequentScreenToScreenCopy(ScrnInfoPtr pScrn,
> - int x1, int y1, int x2,
> - int y2, int w, int h);
> +int x1, int y1, int x2,
> +int y2, int w, int h);
>  static void BladeSetupForScreenToScreenCopy(ScrnInfoPtr pScrn,
> - int xdir, int ydir, int rop, 
> -unsigned int planemask,
> - int transparency_color);
> +int xdir, int ydir, int rop,
> +unsigned int planemask,
> +int transparency_color);
>  #if 0
>  static void BladeSetupForScreenToScreenColorExpand(ScrnInfoPtr pScrn,
> - int fg, int bg, int rop,
> - unsigned int planemask);
> +int fg, int bg, int rop,
> +unsigned int planemask);
>  static void BladeSubsequentScreenToScreenColorExpand(ScrnInfoPtr pScrn,
> - int x, int y, int w, int h, int srcx, int srcy,
> - int offset);
> +int x, int y, int w, int h, int srcx, int srcy,
> +int offset);
>  #endif
>  static void BladeSetupForCPUToScreenColorExpand(ScrnInfoPtr pScrn,
> - int fg, int bg, int rop,
> - unsigned int planemask);
> +int fg, int bg, int rop,
> +unsigned int planemask);
>  static void BladeSubsequentCPUToScreenColorExpand(ScrnInfoPtr pScrn,
> - int x, int y, int w, int h, int skipleft);
> -static void BladeSetClippingRectangle(ScrnInfoPtr pScrn, int x1, int y1, 
> - int x2, int y2);
> +int x, int y, int w, int h, int skipleft);
> +static void BladeSetClippingRectangle(ScrnInfoPtr pScrn, int x1, int y1,
> +int x2, int y2);
>  static void BladeDisableClipping(ScrnInfoPtr pScrn);
> -static void BladeSetupForMono8x8PatternFill(ScrnInfoPtr pScrn, 
> - int patternx, int patterny, int fg, int bg, 
> - int rop, unsigned int planemask);
> -static void BladeSubsequentMono8x8PatternFillRect(ScrnInfoPtr pScrn, 
> - int patternx, int patterny, int x, int y, 
> - int w, int h);
> +static void BladeSetupForMono8x8PatternFill(ScrnInfoPtr pScrn,
> +int patternx, int patterny, int fg, int bg,
> +int rop, unsigned int planemask);
> +static void BladeSubsequentMono8x8PatternFillRect(ScrnInfoPtr pScrn,
> +int patternx, int patterny, int x, int y,
> +int w, int h);
>  #if 0
> -static void BladeSetupForColor8x8PatternFill(ScrnInfoPtr pScrn, 
> - int patternx, int patterny, 
> - int rop, unsigned int planemask, int trans_col);
> -static void 

Re: Let's merge mkfontdir into mkfontscale

[Walter Harms]

> Alan Coopersmith  hat am 19. Februar 2019 um
> 03:40 geschrieben:
> 
> 
> Maintaining a separate repo & set of tarball releases for a 3 line
> shell script wrapper around mkfontscale and its man page just doesn't
> seem to have been a good use of time for the past 15 years, so let's
> stop doing that.
> 
> https://gitlab.freedesktop.org/xorg/app/mkfontscale/merge_requests/2
> 
> If no one objects by Friday, I'll go ahead and merge this, cut a
> mkfontscale 1.2 release, and archive the mkfontdir gitlab project.
> 


mmh,
since mkfontdir is actualy mkfontscale with 3 Options already set ...
would it be an option to merge in completly using a argv[0] check ?

ntl i should be merged. 

re,
 wh

ps:are there any users ?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-input-synaptics] Use fabs() instead of abs() on double values.

[Walter Harms]

> Matthieu Herrb  hat am 10. Februar 2019 um 17:29
> geschrieben:
> 
> 
> Silences clang warnings.
> 
> Signed-off-by: Matthieu Herrb 

seems resonable
Reviewed-by: Walter Harms wharms@bfs,de
> ---
>  src/synaptics.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/synaptics.c b/src/synaptics.c
> index 930d02e..bc2d2af 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -2602,7 +2602,7 @@ HandleScrolling(SynapticsPrivate * priv, struct
> SynapticsHwState *hw,
>  
>  priv->scroll.delta_y += priv->scroll.coast_speed_y * dtime *
> abs(para->scroll_dist_vert);
>  delay = MIN(delay, POLL_MS);
> -if (abs(priv->scroll.coast_speed_y) < ddy) {
> +if (fabs(priv->scroll.coast_speed_y) < ddy) {
>  priv->scroll.coast_speed_y = 0;
>  priv->scroll.packets_this_scroll = 0;
>  }
> @@ -2617,7 +2617,7 @@ HandleScrolling(SynapticsPrivate * priv, struct
> SynapticsHwState *hw,
>  double ddx = para->coasting_friction * dtime;
>  priv->scroll.delta_x += priv->scroll.coast_speed_x * dtime *
> abs(para->scroll_dist_horiz);
>  delay = MIN(delay, POLL_MS);
> -if (abs(priv->scroll.coast_speed_x) < ddx) {
> +if (fabs(priv->scroll.coast_speed_x) < ddx) {
>  priv->scroll.coast_speed_x = 0;
>  priv->scroll.packets_this_scroll = 0;
>  }
> @@ -2673,8 +2673,8 @@ clickpad_guess_clickfingers(SynapticsPrivate * priv,
>   * really, this should be dependent on the touchpad size. Also,
>   * you'll need to find a touchpad that doesn't lie about it's
>   * size. Good luck. */
> -if (abs(x1 - x2) < (priv->maxx - priv->minx) * .3 &&
> -abs(y1 - y2) < (priv->maxy - priv->miny) * .3) {
> +if (fabs(x1 - x2) < (priv->maxx - priv->minx) * .3 &&
> +fabs(y1 - y2) < (priv->maxy - priv->miny) * .3) {
>  close_point |= (1 << j);
>  close_point |= (1 << i);
>  }
> -- 
> 2.20.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-neomagic] Use fabs() instead of abs() on double value.

[Walter Harms]

> Matthieu Herrb  hat am 10. Februar 2019 um 17:20
> geschrieben:
> 
> 
> Silences clang warnings.
> 
> Signed-off-by: Matthieu Herrb 

Reviewed-by: Walter Harms wharms@bfs,de
> ---
>  src/neo_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/neo_driver.c b/src/neo_driver.c
> index 84e09ca..4301881 100644
> --- a/src/neo_driver.c
> +++ b/src/neo_driver.c
> @@ -2953,7 +2953,7 @@ neoCalcVCLK(ScrnInfoPtr pScrn, long freq)
>   for (n = 0; n <= MAX_N; n++)
>   for (d = 1; d <= MAX_D; d++) {
>   f_out = (n+1.0)/((d+1.0)*(1< - f_diff = abs(f_out-f_target);
> + f_diff = fabs(f_out-f_target);
>   if (f_diff < f_best_diff) {
>   f_best_diff = f_diff;
>   n_best = n;
> -- 
> 2.20.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-input-joystick] Silence clang warnings on floating point abs() / pow() calls.

[Walter Harms]

> Matthieu Herrb  hat am 10. Februar 2019 um 17:15
> geschrieben:
> 
> 
> Signed-off-by: Matthieu Herrb 
> ---
>  src/jstk_axis.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/jstk_axis.c b/src/jstk_axis.c
> index cb4a004..37bb66b 100644
> --- a/src/jstk_axis.c
> +++ b/src/jstk_axis.c
> @@ -136,11 +136,11 @@ jstkAxisTimer(OsTimerPtrtimer,
>*/
>  
>  /* How many pixels should this axis move the cursor */
> -p1 = (pow((abs((float)axis->value) - (float)axis->deadzone) *
> +p1 = (powf((fabsf((float)axis->value) - (float)axis->deadzone) *
>scale / 23, 1.4f) + 100.0f) *
>   ((float)NEXTTIMER / 4.0f);

look reasonable perhaps we can improve readability also, like:
 double delta=fabsf(axis->value) - axis->deadzone;
 double ftimer=(float)NEXTTIMER / 4.0f;

 p1= powf(delta/23.0*scale,1.4f)/ftimer+100.0f/ftimer;


>  /* How many "pixels" should this axis scroll */
> -p2 = ((pow((abs((float)axis->value) - (float)axis->deadzone) *

   ftimer=(float)NEXTTIMER / 20.0f;
   p2= powf(delta/1000.0*scale,2.5f)/ftimer+200.0f/ftimer;
you can earn bonus points for explaining the magic numbers.

just my 2 cents,
   re,
wh
 
> +p2 = ((powf((fabsf((float)axis->value) - (float)axis->deadzone) *
>scale / 1000.0f, 2.5f)) + 200.0f) *
>   ((float)NEXTTIMER / 20.0f);
>  } else if (axis->type == JSTK_TYPE_ACCELERATED) {
> -- 
> 2.20.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xinit] Buffer overflow with many arguments.

[Walter Harms]

> Tobias Stoeckmann  hat am 7. Februar 2019 um 20:54
> geschrieben:
> 
> 
> Command line arguments are copied into clientargv and serverargv without
> verifying that enough space is available. A high amount of arguments can
> therefore trigger a buffer overflow like this:
> 
> $ xinit $(seq 1 500)
> 
> Signed-off-by: Tobias Stoeckmann 


works for me

Reviewed-by: Walter Harms wharms@bfs,de

> ---
> Integrated calculation as suggested by Walter with style according to
> rest of the code.
> ---
>  xinit.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xinit.c b/xinit.c
> index f826b7a..06c92b2 100644
> --- a/xinit.c
> +++ b/xinit.c
> @@ -151,7 +151,6 @@ main(int argc, char *argv[])
>  register char **ptr;
>  pid_t pid;
>  int client_given = 0, server_given = 0;
> -int client_args_given = 0, server_args_given = 0;
>  int start_of_client_args, start_of_server_args;
>  struct sigaction sa, si;
>  #ifdef __APPLE__
> @@ -174,7 +173,8 @@ main(int argc, char *argv[])
>  }
>  start_of_client_args = (cptr - client);
>  while (argc && strcmp(*argv, "--")) {
> -client_args_given++;
> +if (cptr > clientargv + sizeof(clientargv) / sizeof(*clientargv) - 2)
> +Fatalx("too many client arguments");
>  *cptr++ = *argv++;
>  argc--;
>  }
> @@ -202,7 +202,8 @@ main(int argc, char *argv[])
>  
>  start_of_server_args = (sptr - server);
>  while (--argc >= 0) {
> -server_args_given++;
> +if (sptr > serverargv + sizeof(serverargv) / sizeof(*serverargv) - 2)
> +Fatalx("too many server arguments");
>  *sptr++ = *argv++;
>  }
>  *sptr = NULL;
> -- 
> 2.20.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [xorgproto] Remove RCS Ids

[walter harms]

Am 30.01.2019 21:35, schrieb Matthieu Herrb:
> Signed-off-by: Matthieu Herrb 
> ---
>  include/X11/extensions/xcalibrateproto.h | 2 --
>  include/X11/extensions/xcalibratewire.h  | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/include/X11/extensions/xcalibrateproto.h 
> b/include/X11/extensions/xcalibrateproto.h
> index cafdab2..f9b9425 100644
> --- a/include/X11/extensions/xcalibrateproto.h
> +++ b/include/X11/extensions/xcalibrateproto.h
> @@ -1,6 +1,4 @@
>  /*
> - * $Id: xcalibrateproto.h,v 1.1.1.1 2004/06/02 19:18:47 pb Exp $
> - *
>   * Copyright © 2003 Philip Blundell
>   *
>   * Permission to use, copy, modify, distribute, and sell this software and 
> its
> diff --git a/include/X11/extensions/xcalibratewire.h 
> b/include/X11/extensions/xcalibratewire.h
> index 0a1c904..d1d2edf 100644
> --- a/include/X11/extensions/xcalibratewire.h
> +++ b/include/X11/extensions/xcalibratewire.h
> @@ -1,6 +1,4 @@
>  /*
> - * $Id: xcalibratewire.h,v 1.1.1.1 2004/06/02 19:18:47 pb Exp $
> - *
>   * Copyright © 2003 Philip Blundell
>   *
>   * Permission to use, copy, modify, distribute, and sell this software and 
> its


Reviewed-by: Walter Harms wharms@bfs,de
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: xwd: ./dsimple.c:277:17: warning: macro expands to multiple statements [-Wmultistatement-macros]

[Walter Harms]

Perhaps it is better to remove the #define totaly.

just my 2 cents,
 re,
   wh

> "MEERSMAN Koen (EXT)"  hat am 24. Januar
> 2019 um 11:00 geschrieben:
> 
> 
> Hi
> 
> When compiling dsimple.c with -Wall we get warning:
> 
> ./dsimple.c: In function 'Select_Window_Args':
> ./dsimple.c:277:17: warning: macro expands to multiple statements
> [-Wmultistatement-macros]
>  #define COPYOPT nargv++[0]=OPTION; nargc++
>  ^
> ./dsimple.c:283:27: note: in expansion of macro 'COPYOPT'
>COPYOPT;
>^~~
> ./dsimple.c:282:25: note: some parts of macro expansion are not
> guarded by this 'while' clause
>  while (NXTOPTP)
>  ^
> 
> Question I have is what should the code do:
> 
> -- original code:
> 
> #define COPYOPT nargv++[0]=OPTION; nargc++
> 
> while (NXTOPTP) {
> if (!strcmp(OPTION, "-")) {
> COPYOPT;
> while (NXTOPTP)
>   COPYOPT;
> break;
> }
> 
> -- fixed code
> #define COPYOPT nargv++[0]=OPTION; nargc++
> 
> while (NXTOPTP) {
> if (!strcmp(OPTION, "-")) {
> COPYOPT;
> >   while (NXTOPTP) {
> > nargv++[0]=OPTION
> >   }
> break;
> }
> 
> -- now it does: (what looks wrong to me)
> 
> #define COPYOPT nargv++[0]=OPTION; nargc++
> 
> while (NXTOPTP) {
> if (!strcmp(OPTION, "-")) {
> COPYOPT;
> >   while (NXTOPTP) {
> > COPYOPT;
> >   }
> >   nargc++
> break;
> }
> 
> Can't find any usage of Select_Window_Args so I can't test.
> 
> 
> 
> Tnx,
> 
> Koen
> 
> 
> 
> 
> 
> /*
>  * Select_Window_Args: a rountine to provide a common interface for
>  * applications that need to allow the user to select one
>  * window on the screen for special consideration.
>  * This routine implements the following command line
>  * arguments:
>  *
>  *   -rootSelects the root window.
>  *   -id  Selects window with id . 
> may
>  *be either in decimal or hex.
>  *   -name  Selects the window with name .
>  *
>  * Call as Select_Window_Args(, argv) in main before
>  * parsing any of your program's command line arguments.
>  * Select_Window_Args will remove its arguments so that
>  * your program does not have to worry about them.
>  * The window returned is the window selected or 0 if
>  * none of the above arguments was present.  If 0 is
>  * returned, Select_Window should probably be called after
>  * all command line arguments, and other setup is done.
>  * For examples of usage, see xwininfo, xwd, or xprop.
>  */
> Window Select_Window_Args(
>   int *rargc,
>   char **argv)
> #define ARGC (*rargc)
> {
> int nargc=1;
> int argc;
> char **nargv;
> Window w=0;
> 
> nargv = argv+1; argc = ARGC;
> #define OPTION argv[0]
> #define NXTOPTP ++argv, --argc>0
> #define NXTOPT if (++argv, --argc==0) usage()
> #define COPYOPT nargv++[0]=OPTION; nargc++
> 
> while (NXTOPTP) {
> if (!strcmp(OPTION, "-")) {
> COPYOPT;
> while (NXTOPTP)
>   COPYOPT;
> break;
> }
> if (!strcmp(OPTION, "-root")) {
> w=RootWindow(dpy, screen);
> continue;
> }
> if (!strcmp(OPTION, "-name")) {
> NXTOPT;
> w = Window_With_Name(dpy, RootWindow(dpy, screen),
>  OPTION);
> if (!w)
>   Fatal_Error("No window with name %s
> exists!",OPTION);
> continue;
> }
> if (!strcmp(OPTION, "-id")) {
> NXTOPT;
> w=0;
> sscanf(OPTION, "0x%lx", );
>

Re: [PATCH app/xinit] Buffer overflow with many arguments.

[Walter Harms]

> Tobias Stöckmann  hat am 19. Januar 2019 um 20:37
> geschrieben:
> 
> 
> > hi,
> > nice catch.
> > 
> > instead of letting 98 magicly popup what is about
> > sizeof(serverargv)/sizeof(*serverargv) ?
> > Dito clientargv,
> > 
> > re,
> >  wh
> 
> There is still a pseudo-magical - 2 missing there, to keep space for the
> last NULL assignment.
> 
> But I'm fine with both. As long as 98 is the result. :)
> 
> 

this is my version, like your patch but the array limit is now calculated.
NTL the program needs some more.


Signed-off-by: Walter Harms 
---
 xinit.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xinit.c b/xinit.c
index f826b7a..b93fe20 100644
--- a/xinit.c
+++ b/xinit.c
@@ -151,7 +151,6 @@ main(int argc, char *argv[])
 register char **ptr;
 pid_t pid;
 int client_given = 0, server_given = 0;
-int client_args_given = 0, server_args_given = 0;
 int start_of_client_args, start_of_server_args;
 struct sigaction sa, si;
 #ifdef __APPLE__
@@ -174,7 +173,8 @@ main(int argc, char *argv[])
 }
 start_of_client_args = (cptr - client);
 while (argc && strcmp(*argv, "--")) {
-client_args_given++;
+if (cptr > clientargv + sizeof(clientargv)/sizeof(*clientargv)-2)
+   Fatalx("too many client arguments");
 *cptr++ = *argv++;
 argc--;
 }
@@ -202,7 +202,9 @@ main(int argc, char *argv[])
 
 start_of_server_args = (sptr - server);
 while (--argc >= 0) {
-server_args_given++;
+if (sptr > serverargv + sizeof(serverargv) /sizeof(*serverargv)-2 )
+Fatalx("too many server arguments");
+
 *sptr++ = *argv++;
 }
 *sptr = NULL;
-- 
2.1.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] XF86keysym: Add XF86XK_RotationLockToggle

[Walter Harms]

> Hans de Goede  hat am 21. Januar 2019 um 20:23
> geschrieben:
> 
> 
> Add XF86XK_RotationLockToggle keysym, to be used as mapping for evdev's
> KEY_ROTATE_LOCK_TOGGLE.
> 
> I've a Point of View P1006W-232 Windows tablet which actually has a
> rotate-lock toggle-button. The latest kernel correctly generates
> KEY_ROTATE_LOCK_TOGGLE events for this. So now I'm hooking up support for
> it through all the higher layers.
> 
> Signed-off-by: Hans de Goede 
> ---
>  include/X11/XF86keysym.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/X11/XF86keysym.h b/include/X11/XF86keysym.h
> index 9ad8948..dd287e2 100644
> --- a/include/X11/XF86keysym.h
> +++ b/include/X11/XF86keysym.h
> @@ -205,6 +205,8 @@
>  
>  #define XF86XK_AudioPreset   0x1008FFB6   /* Select equalizer preset, e.g.
> theatre-mode */
>  
> +#define XF86XK_RotationLockToggle 0x1008FFB7 /* Toggle screen rotation lock
> on/off */
> +
>  /* Keys for special action keys (hot keys) */
>  /* Virtual terminals on some operating systems */
>  #define XF86XK_Switch_VT_1   0x1008FE01
> -- 
> 2.20.1
> 

This is ok with me but i  have a general question:
Is there a policy what to add ? ( i coud not find one).
E.g. i have seems a keyboard with a key to change the backlight
of the keyboard (do ot ask, not mine) Do we simply add everything
found in the wild ? or are there limitations ?

re,
 wh



> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xinit] Buffer overflow with many arguments.

[Walter Harms]

> Tobias Stoeckmann  hat am 18. Januar 2019 um 21:59
> geschrieben:
> 
> 
> Command line arguments are copied into clientargv and serverargv without
> verifying that enough space is available. A high amount of arguments can
> therefore trigger a buffer overflow like this:
> 
> $ xinit $(seq 1 500)
> 
> Signed-off-by: Tobias Stoeckmann 
> ---
>  xinit.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xinit.c b/xinit.c
> index f826b7a..8efd0be 100644
> --- a/xinit.c
> +++ b/xinit.c
> @@ -151,7 +151,6 @@ main(int argc, char *argv[])
>  register char **ptr;
>  pid_t pid;
>  int client_given = 0, server_given = 0;
> -int client_args_given = 0, server_args_given = 0;
>  int start_of_client_args, start_of_server_args;
>  struct sigaction sa, si;
>  #ifdef __APPLE__
> @@ -174,7 +173,8 @@ main(int argc, char *argv[])
>  }
>  start_of_client_args = (cptr - client);
>  while (argc && strcmp(*argv, "--")) {
> -client_args_given++;
> +if (cptr > clientargv + 98)
> +Fatalx("too many client arguments");
>  *cptr++ = *argv++;
>  argc--;
>  }
> @@ -202,7 +202,8 @@ main(int argc, char *argv[])
>  
>  start_of_server_args = (sptr - server);
>  while (--argc >= 0) {
> -server_args_given++;
> +if (sptr > serverargv + 98)
> +Fatalx("too many server arguments");
>  *sptr++ = *argv++;
>  }
>  *sptr = NULL;
> -- 

hi,
nice catch.

instead of letting 98 magicly popup what is about
sizeof(serverargv)/sizeof(*serverargv) ?
Dito clientargv,

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] Fix checking for valid argument of --dpi

[Walter Harms]

> Pali Rohár  hat am 29. Dezember 2018 um 18:45
> geschrieben:
> 
> 
> On Monday 26 November 2018 22:17:24 Pali Rohár wrote:
> > Function strtod() sets strtod_error to the pointer of the first invalid
> > character and therefore it does not have to be first character from input.
> > When input is valid then it points to nul byte. Conversion error is
> > indicated by setted errno. Zero-length argument, non-positive DPI or DPI
> > with trailing or leading whitespaces is invalid too.
> > 
> > Update also error message about invalid argument.
> > 
> > Signed-off-by: Pali Rohár 
> > 
> > --
> > 
> > Changes since v1:
> > * Get same error message for `xrandr --dpi ''` and `xrandr --dpi ' '`
> > * Make the check for dpi <= 0
> > * Do not accept leading whitespaces (trailing were already disallowed)
> > ---
> >  xrandr.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xrandr.c b/xrandr.c
> > index ce3cd91..4baa075 100644
> > --- a/xrandr.c
> > +++ b/xrandr.c
> > @@ -40,6 +40,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #ifdef HAVE_CONFIG_H
> >  #include "config.h"
> > @@ -3118,8 +3119,9 @@ main (int argc, char **argv)
> > if (!strcmp ("--dpi", argv[i])) {
> > char *strtod_error;
> > if (++i >= argc) argerr ("%s requires an argument\n", argv[i-1]);
> > +   errno = 0;
> > dpi = strtod(argv[i], _error);
> > -   if (argv[i] == strtod_error)
> > +   if (!argv[i][0] || isspace(argv[i][0]) || *strtod_error || errno ||
> > dpi <= 0)
> > {
> > dpi = 0.0;
> > dpi_output_name = argv[i];


Why spending so much effort ? 
Using atoi(argv[i]) and checking for an invalid value seems fine.
Special cases like NAN do not matter here.

BTW: leading whitespaces are ignored with strtod() (so far i know)

just my 2 cents
re,
 wh



> > @@ -3569,7 +3571,7 @@ main (int argc, char **argv)
> > XRROutputInfo   *output_info;
> > XRRModeInfo *mode_info;
> > if (!dpi_output)
> > -   fatal ("Cannot find output %s\n", dpi_output_name);
> > +   fatal ("%s is not valid DPI nor valid output\n", 
> > dpi_output_name);
> > output_info = dpi_output->output_info;
> > mode_info = dpi_output->mode_info;
> > if (output_info && mode_info && output_info->mm_height)
> 
> Hello, can you review this v2 patch?
> 
> -- 
> Pali Rohár
> pali.ro...@gmail.com
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/twm 1/2] When replacing a colormap, free old map, not new one

[Walter Harms]

> Alan Coopersmith  hat am 30. September 2018 um
> 23:44 geschrieben:
> 
> 
> Found by Oracle's Parfait 2.2 static analyzer:
> 
> Error: Use after free
>Use after free [use-after-free] (CWE 416):
>   Use after free of pointer Scr
> at line 421 of src/util.c in function 'InsertRGBColormap'.
> Invalid pointer accessible via global Scr at line 105 of src/twm.c
>   maps escapes to sc->maps at line 419 of src/util.c in function
> 'InsertRGBColormap'
>   maps freed with XFree at line 406
> 
> Signed-off-by: Alan Coopersmith 
> ---
>  src/util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util.c b/src/util.c
> index 8e9dab9..e254cd5 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -403,7 +403,7 @@ InsertRGBColormap (Atom a, XStandardColormap *maps, int
> nmaps, Bool replace)
>  }
>  
>  if (replace) {   /* just update contents */
> - if (sc->maps) XFree (maps);
> + if (sc->maps) XFree (sc->maps);

just
 XFree (sc->maps);

is sufficient

re,
 wh
 
>   if (sc == Scr->StdCmapInfo.mru) Scr->StdCmapInfo.mru = NULL;
>  } else { /* else appending */
>   sc->next = NULL;
> -- 
> 2.15.2
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libXpm] After fdopen(), use fclose() instead of close() in error path

[Walter Harms]

> Alan Coopersmith  hat am 1. Oktober 2018 um 00:14
> geschrieben:
> 
> 
> Found by Oracle's Parfait 2.2 static analyzer:
> 
> Error: File Leak
>File Leak [file-ptr-leak]:
>   Leaked File fp
> at line 94 of lib/libXpm/src/RdFToBuf.c in function
> 'XpmReadFileToBuffer
> '.
>   fp initialized at line 86 with fdopen
>   fp leaks when len < 0 at line 92.
> 
> Introduced-by: commit 8b3024e6871ce50b34bf2dff924774bd654703bc
> 
> Signed-off-by: Alan Coopersmith 
> ---
>  src/RdFToBuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/RdFToBuf.c b/src/RdFToBuf.c
> index 69e3347..1b386f8 100644
> --- a/src/RdFToBuf.c
> +++ b/src/RdFToBuf.c
> @@ -86,15 +86,15 @@ XpmReadFileToBuffer(
>  fp = fdopen(fd, "r");
>  if (!fp) {
>   close(fd);
>   return XpmOpenFailed;
>  }
>  len = stats.st_size;
>  if (len < 0 || len >= SIZE_MAX) {
> - close(fd);
> + fclose(fp);
>   return XpmOpenFailed;
>  }

IMHO it should do both,
otherwise you have a different behavier when
fdopen failed and returning XpmOpenFailed
or size check failed and returning XpmOpenFailed



>  ptr = (char *) XpmMalloc(len + 1);
>  if (!ptr) {
>   fclose(fp);
>   return XpmNoMemory;
>  }

this does not close(fd) either, maybe
it is better not to close it in the first case 
to have a similar behavier ?

re,
 wh


> -- 
> 2.15.2
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] devices: break after finding and removing device from lists

[Walter Harms]

> Dave Airlie  hat am 12. September 2018 um 03:40
> geschrieben:
> 
> 
> From: Dave Airlie 
> 
> Coverity complains about a use after free in here after the
> freeing, I can't follow the linked list so well, but whot
> says the device can only be on one list once, so break should
> fix it.
> 
> Signed-off-by: Dave Airlie 

Is a list of Coverity complains some where online available ?

re,
 wh

> ---
>  dix/devices.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/dix/devices.c b/dix/devices.c
> index 4a628afb0..1b18b168e 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c
> @@ -1177,6 +1177,7 @@ RemoveDevice(DeviceIntPtr dev, BOOL sendevent)
>  flags[tmp->id] = IsMaster(tmp) ? XIMasterRemoved :
> XISlaveRemoved;
>  CloseDevice(tmp);
>  ret = Success;
> +break;
>  }
>  }
>  
> @@ -1193,6 +1194,7 @@ RemoveDevice(DeviceIntPtr dev, BOOL sendevent)
>  prev->next = next;
>  
>  ret = Success;
> +break;
>  }
>  }
>  
> -- 
> 2.17.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/sessreg] Replace strncpy calls with a sane version that always terminates

[Walter Harms]

> Peter Hutterer  hat am 12. September 2018 um 06:50
> geschrieben:
> 
> 
> Fixes coverity complaints about potentially unterminated strings
> 
> Signed-off-by: Peter Hutterer 
> ---
>  sessreg.c | 26 +-
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/sessreg.c b/sessreg.c
> index 0a8fdb2..53b30b0 100644
> --- a/sessreg.c
> +++ b/sessreg.c
> @@ -192,6 +192,14 @@ sysnerr (int x, const char *s)
>   return x;
>  }
>  
> +static void
> +safe_strncpy(char *dest, const char *src, size_t n)
> +{

if you add
 if (!src)
  return;

you can also remove the if (x) before strncpy()

> +(void)strncpy(dest, src, n);
> +if (n > 0)
> +dest[n - 1] = '\0';
> +}
> +


just my 2 cents

re,
 wh

>  int
>  main (int argc, char **argv)
>  {
> @@ -406,9 +414,9 @@ main (int argc, char **argv)
>   memset(, 0, sizeof(ll));
>   ll.ll_time = current_time;
>   if (line)
> -  (void) strncpy (ll.ll_line, line, sizeof (ll.ll_line));
> +  safe_strncpy (ll.ll_line, line, sizeof (ll.ll_line));
>   if (host_name)
> -  (void) strncpy (ll.ll_host, host_name, sizeof 
> (ll.ll_host));
> +  safe_strncpy (ll.ll_host, host_name, sizeof 
> (ll.ll_host));
>  
>   sysnerr (write (llog, (char *) , sizeof (ll))
>   == sizeof (ll), "write lastlog entry");
> @@ -429,11 +437,11 @@ set_utmp (struct utmp *u, char *line, char *user, char
> *host, time_t date, int a
>  {
>   memset (u, 0, sizeof (*u));
>   if (line)
> - (void) strncpy (u->ut_line, line, sizeof (u->ut_line));
> + safe_strncpy (u->ut_line, line, sizeof (u->ut_line));
>   else
>   memset (u->ut_line, 0, sizeof (u->ut_line));
>   if (addp && user)
> - (void) strncpy (u->ut_name, user, sizeof (u->ut_name));
> + safe_strncpy (u->ut_name, user, sizeof (u->ut_name));
>   else
>   memset (u->ut_name, 0, sizeof (u->ut_name));
>  #ifdef HAVE_STRUCT_UTMP_UT_ID
> @@ -451,7 +459,7 @@ set_utmp (struct utmp *u, char *line, char *user, char
> *host, time_t date, int a
>   i -= sizeof (u->ut_id);
>   else
>   i = 0;
> - (void) strncpy (u->ut_id, line + i, sizeof (u->ut_id));
> + safe_strncpy (u->ut_id, line + i, sizeof (u->ut_id));
>   } else
>   memset (u->ut_id, 0, sizeof (u->ut_id));
>  #endif
> @@ -469,7 +477,7 @@ set_utmp (struct utmp *u, char *line, char *user, char
> *host, time_t date, int a
>  #endif
>  #ifdef HAVE_STRUCT_UTMP_UT_HOST
>   if (addp && host)
> - (void) strncpy (u->ut_host, host, sizeof (u->ut_host));
> + safe_strncpy (u->ut_host, host, sizeof (u->ut_host));
>   else
>   memset (u->ut_host, 0, sizeof (u->ut_host));
>  #endif
> @@ -513,7 +521,7 @@ set_utmpx (struct utmpx *u, const char *line, const char
> *user,
>   if(strcmp(line, ":0") == 0)
>   (void) strcpy(u->ut_line, "console");
>   else
> - (void) strncpy (u->ut_line, line, sizeof (u->ut_line));
> + safe_strncpy (u->ut_line, line, sizeof (u->ut_line));
>  
>   strncpy(u->ut_host, line, sizeof(u->ut_host));
>  #ifdef HAVE_STRUCT_UTMPX_UT_SYSLEN
> @@ -523,7 +531,7 @@ set_utmpx (struct utmpx *u, const char *line, const char
> *user,
>   else
>   memset (u->ut_line, 0, sizeof (u->ut_line));
>   if (addp && user)
> - (void) strncpy (u->ut_user, user, sizeof (u->ut_user));
> + safe_strncpy (u->ut_user, user, sizeof (u->ut_user));
>   else
>   memset (u->ut_user, 0, sizeof (u->ut_user));
>  
> @@ -541,7 +549,7 @@ set_utmpx (struct utmpx *u, const char *line, const char
> *user,
>   i -= sizeof (u->ut_id);
>   else
>   i = 0;
> - (void) strncpy (u->ut_id, line + i, sizeof (u->ut_id));
> + safe_strncpy (u->ut_id, line + i, sizeof (u->ut_id));
>  
>   /* make sure there is no entry using identical ut_id */
>   if (!UtmpxIdOpen(u->ut_id) && addp) {
> -- 
> 2.17.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/fonttosfnt 5/5] Replace _BSD_SOURCE with _DEFAULT_SOURCE

[Walter Harms]

> Peter Hutterer  hat am 31. Juli 2018 um 03:53
> geschrieben:
> 
> 
> /usr/include/features.h:184:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE
> are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
>  # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"


Why is is needed in the first place ? (what would break if not set ?)

re,
 wh

> 
> Signed-off-by: Peter Hutterer 
> ---
>  Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6ce70b7..29169b1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -22,7 +22,7 @@
>  SUBDIRS = man
>  bin_PROGRAMS = fonttosfnt
>  
> -AM_CFLAGS = -DXVENDORNAME=\"X.Org\ Foundation\" $(FONTTOSFNT_CFLAGS)
> -DXVENDORNAMESHORT=\"X.Org\" -D_BSD_SOURCE -D_GNU_SOURCE $(CWARNFLAGS)
> +AM_CFLAGS = -DXVENDORNAME=\"X.Org\ Foundation\" $(FONTTOSFNT_CFLAGS)
> -DXVENDORNAMESHORT=\"X.Org\" -D_DEFAULT_SOURCE -D_GNU_SOURCE $(CWARNFLAGS)
>  fonttosfnt_LDADD = $(FONTTOSFNT_LIBS) -lm
>  
>  fonttosfnt_SOURCES = \
> -- 
> 2.17.1
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xev] xev: fix 'mode' in default case (version 2)

[Walter Harms]

Hello Eitan,
thx for your effort just a general remark:
be verbose what you are changing and why (it took me a while to notice the , ->
; change)
exspecialy if it fixed a bug (e.g. "... otherwise abc is never assigned.")

Dito changes in patches, it is ok to fix a patch, just write it inside like:

V2: fix also second occurence 
V1: fixes warning

got the idea ?

re,
 wh


> Eitan Adler  hat am 28. Juli 2018 um 10:28 geschrieben:
> 
> 
> Signed-off-by: Eitan Adler 
> ---
>  xev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xev.c b/xev.c
> index 6cde09a..638eae7 100644
> --- a/xev.c
> +++ b/xev.c
> @@ -225,7 +225,7 @@ do_EnterNotify (XEvent *eventp)
>case NotifyGrab:  mode = "NotifyGrab"; break;
>case NotifyUngrab:  mode = "NotifyUngrab"; break;
>case NotifyWhileGrabbed:  mode = "NotifyWhileGrabbed"; break;
> -  default:  mode = dmode, sprintf (dmode, "%u", e->mode); break;
> +  default:  mode = dmode; sprintf (dmode, "%u", e->mode); break;
>  }
>  
>  switch (e->detail) {
> @@ -265,7 +265,7 @@ do_FocusIn (XEvent *eventp)
>case NotifyGrab:  mode = "NotifyGrab"; break;
>case NotifyUngrab:  mode = "NotifyUngrab"; break;
>case NotifyWhileGrabbed:  mode = "NotifyWhileGrabbed"; break;
> -  default:  mode = dmode, sprintf (dmode, "%u", e->mode); break;
> +  default:  mode = dmode; sprintf (dmode, "%u", e->mode); break;
>  }
>  
>  switch (e->detail) {
> -- 
> 2.18.0
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xauth 2/2] Sort entries from most specific to most generic.

[Walter Harms]

> Michal Srb  hat am 7. Juni 2018 um 16:01 geschrieben:
> 
> 
> On čtvrtek 7. června 2018 14:59:44 CEST Walter Harms wrote:
> > I was thinking about a more permanent solution and sofar i see the correct
> > way would to add the two functions to libXau since i do not see that other
> > applications would be more clever that xauth.
> 
> That would be nice, sadly libXau does not provide any functions to manipulate 
> the list of xauth records. Just functions for writing and reading of a single 
> record, locking and matching.
> 
> One option would be to change the semantics of XauGetBestAuthByAddr to return 
> the most precise match first, instead of returning the first match of any 
> kind. But I don't think changing that behavior is good idea.
> 
> Alternatively we can just take the patch #1, so that xauth does overwrite 
> records with other random records. I believe that is correct fix. And we can 
> ignore this patch #2 that does the sorting. I could probably workaround my 
> issue by invoking xauth differently. (I could create 1-item list first 
> containing the new record and then merge the original list into it, so the new
> 
> record will end up on top.)
> 
> Michal Srb


I am waiting if someone will complain, if nothing happens until fr. i will see
to add your patch and will mail you for testing.

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xauth 2/2] Sort entries from most specific to most generic.

[Walter Harms]

> Michal Srb  hat am 6. Juni 2018 um 08:34 geschrieben:
> 
> 
> On úterý 5. června 2018 17:46:05 CEST Walter Harms wrote:
> > MMh, so far i understand you do an add and then sort the whole thing.
> > perhaps it would be more easy to start with a sorted field and insert (add)
> > a key on the correct location ?
> 
> That would work if we have the guarantee that the list is sorted to begin 
> with. The list will be sorted now if it was created by xauth. But if it was 
> made by anything else (e.g. some display manager) it can be in any order. So 
> when adding new entry, xauth will re-sort it.
> 
> Alternative would be to just insert the new entry in front of the first more 
> general entry and ignore any entries that are never used because of their 
> position in the list.
> 
> I don't have opinion on which is better. Both would work for my usecase.
> 

I see your ideas,
I was thinking about a more permanent solution and sofar i see the correct
way would to add the two functions to libXau since i do not see that other
applications would be more clever that xauth.

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH app/xauth 2/2] Sort entries from most specific to most generic.

[Walter Harms]

> Michal Srb  hat am 31. Mai 2018 um 15:12 geschrieben:
> 
> 
> There is no point in adding entry or merging lists if a FamilyWild entry would
> end in front of any entry, or entry without display number would end in front
> of entry with number.
> 
> This sorts all entries in order:
>   * FamilyWild without display number
>   * FamilyWild with display number
>   * Other family without display number
>   * Other family with display number
> 
> The order of the entries in each category is kept.
> ---
>  process.c | 41 +
>  1 file changed, 41 insertions(+)
> 
> diff --git a/process.c b/process.c
> index d3ea435..f3d6ca4 100644
> --- a/process.c
> +++ b/process.c
> @@ -1170,6 +1170,45 @@ merge_entries(AuthList **firstp, AuthList *second, int
> *nnewp, int *nreplp)
>  
>  }
>  
> +static void
> +sort_entries(AuthList **firstp)
> +{
> +/* Insert sort, in each pass it removes auth records of certain */
> +/* cathegory from the given list and inserts them into the sorted list.
> */
> +
> +AuthList *sorted = NULL, *sorted_tail = NULL;
> +AuthList *prev, *iter, *next;
> +
> +#define SORT_OUT(EXPRESSION) { \
> + prev = NULL; \
> + for (iter = *firstp; iter; iter = next) { \
> + next = iter->next; \
> + if (EXPRESSION) { \
> + if (prev) \
> + prev->next = next; \
> + else \
> + *firstp = next; \
> + if (sorted_tail == NULL) { \
> + sorted = sorted_tail = iter; \
> + } else { \
> + sorted_tail->next = iter; \
> + sorted_tail = iter; \
> + } \
> + iter->next = NULL; \
> + } else { \
> + prev = iter; \
> + } \
> + } \
> +}
> +
> +SORT_OUT(iter->auth->family != FamilyWild && iter->auth->number_length !=
> 0);
> +SORT_OUT(iter->auth->family != FamilyWild && iter->auth->number_length ==
> 0);
> +SORT_OUT(iter->auth->family == FamilyWild && iter->auth->number_length !=
> 0);
> +SORT_OUT(iter->auth->family == FamilyWild && iter->auth->number_length ==
> 0);
> +
> +*firstp = sorted;
> +}
> +
>  static Xauth *
>  copyAuth(Xauth *auth)
>  {
> @@ -1508,6 +1547,7 @@ do_merge(const char *inputfilename, int lineno, int
> argc, const char **argv)
> printf ("%d entries read in:  %d new, %d replacement%s\n",
> nentries, nnew, nrepl, nrepl != 1 ? "s" : "");
>   if (nentries > 0) xauth_modified = True;
> + sort_entries(_head);
>  }
>  
>  return 0;
> @@ -1656,6 +1696,7 @@ do_add(const char *inputfilename, int lineno, int argc,
> const char **argv)
>   fprintf (stderr, "unable to merge in added record\n");
>   return 1;
>  }
> +sort_entries(_head);
>  
>  xauth_modified = True;
>  return 0;


MMh, so far i understand you do an add and then sort the whole thing.
perhaps it would be more easy to start with a sorted field and insert (add)
a key on the correct location ?

re,
 wh

> -- 
> 2.13.6
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xscope] Improve help and manpage.

[Walter Harms]

> Alan Coopersmith  hat am 6. Mai 2018 um 04:35
> geschrieben:
> 
> 
> On 07/ 8/13 05:40 PM, Stéphane Aulery wrote:
> > Add some details on the verbosity and options -T and -A.
> > These two options are not very clear to me.
> > 
> > Signed-off-by: Stéphane Aulery 
> > ---
> >  man/xscope.man |   89
> > +---
> >  scope.c|   33 -
> >  2 files changed, 84 insertions(+), 38 deletions(-)
> 
> Sorry for letting this get lost in our massive backlog, but I've finally
> gotten this pushed to git master:
> https://cgit.freedesktop.org/xorg/app/xscope/commit/?id=51c3ab198e6fa440961d02f6af13b1d73d698101
> 
> I've also pushed a followup to fix up the NAS protocol options (-A, -n, -a),
> though I'm not sure anyone still uses that protocol:
> https://cgit.freedesktop.org/xorg/app/xscope/commit/?id=f7af80a0ad7807e35f426a2377d7312b415c9d68
> 
> Even after digging through the code, I can't explain why the -T option exists
> - it just seems to add 0x20 to every character code before printing strings.
> Maybe someone else can figure that out.
> 

mybe this from wikipedia can help:

"In typography, letter-spacing, usually called tracking by typographers, 
refers to a consistent degree of increase (or sometimes decrease) of space 
between letters to affect density in a line or block of text."

T= tracking

most likely that was intended for printing.

re,
 wh

> Thanks for the patch!
> 
> -- 
>   -Alan Coopersmith-   alan.coopersm...@oracle.com
>Oracle Solaris Engineering - https://blogs.oracle.com/alanc
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH util/makedepend V2] Quote colons in filenames/paths

[Walter Harms]

> Alan Coopersmith  hat am 5. Mai 2018 um 20:11
> geschrieben:
> 
> 
> From: Antonio Larrosa 
> 
> Makefile doesn't like colons in filenames/paths so they must
> be quoted in the output. Otherwise makedepend doesn't work with
> full paths that contain a colon.
> 
> V2: Use quoted filename when measuring name length
> Signed-off-by: Alan Coopersmith 
> ---
>  pr.c | 48 
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/pr.c b/pr.c
> index 04abef9..9a49635 100644
> --- a/pr.c
> +++ b/pr.c
> @@ -63,25 +63,65 @@ add_include(struct filepointer *filep, struct inclist
> *file,
>   }
>  }
>  
> +/**
> + * Replaces all ":" occurrences in @p input with "\:" using @p outputbuffer
> (of size @p bufsize)
> + * possibly to hold the result. @p returns the string with quoted colons
> + */
> +static const char *
> +quoteColons(const char *input, char *outputbuffer, size_t bufsize)
> +{
> +const char *tmp=input;
> +const char *loc;
> +char *output=outputbuffer;
> +
> +loc = strchr(input, ':');
> +if (loc == NULL) {
> +return input;
> +}
> +
> +tmp=input;
> +while (loc != NULL && bufsize > loc-tmp+2 ) {
> +memcpy(output, tmp, loc-tmp);
> +output+=loc-tmp;
> +bufsize-=loc-tmp+2;
> +tmp=loc+1;
> +*output='\\';
> +output++;
> +*output=':';
> +output++;
> +loc = strchr(tmp, ':');
> +}
> +
> +if (strlen(tmp) <= bufsize)
> +   strcpy(output, tmp);
> +else {
> +   strncpy(output, tmp, bufsize-1);
> +   output[bufsize]=0;
> +}
> +return outputbuffer;
> +}
> +
hi,
i am sure that this is a very fast code but ..
do you not thing it would be better to use a more simple solution ?
like:

for( ; *s ; s++) {
if (*s == ':')
*d++='\\';
*d++=*s;
}
*d=*s;
or did i miss a trick here ?

To avoid a const size buffer you could simply add:
char d[strlen(ip->i_file)*2+1];

and return strdup(d);

BTW why 
 fwrite(buf, len, 1, stdout);

Is there a loop that i am missing ? Otherwise a simple printf()
would do instead of snprinft()

just my 2 cents,
re,
 wh

>  static void
>  pr(struct inclist *ip, const char *file, const char *base)
>  {
>   static const char *lastfile;
>   static int  current_len;
>   register intlen, i;
> + const char *quoted;
>   charbuf[ BUFSIZ ];
> + charquotebuf[ BUFSIZ ];
>  
>   printed = TRUE;
> - len = strlen(ip->i_file)+1;
> + quoted = quoteColons(ip->i_file, quotebuf, sizeof(quotebuf));
> + len = strlen(quoted)+1;
>   if (current_len + len > width || file != lastfile) {
>   lastfile = file;
>   snprintf(buf, sizeof(buf), "\n%s%s%s: %s",
> -  objprefix, base, objsuffix, ip->i_file);
> +  objprefix, base, objsuffix, quoted);
>   len = current_len = strlen(buf);
>   }
>   else {
> - buf[0] = ' ';
> - strcpy(buf+1, ip->i_file);
> + snprintf(buf, sizeof(buf), " %s", quoted);
>   current_len += len;
>   }
>   fwrite(buf, len, 1, stdout);
> -- 
> 2.15.0
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] modesetting: Fix inverted check in dri2 WaitMSC

[Walter Harms]

> Frank Binns  hat am 16. April 2018 um 22:17
> geschrieben:
> 
> 
> Adam Jackson  writes:
> 
> > ms_queue_vblank() returns false on failure.
> >
> > Reported-by: Chris Wilson 
> > Signed-off-by: Adam Jackson 
> 
> Reviewed-by: Frank Binns 
> 
> > ---
> >  hw/xfree86/drivers/modesetting/dri2.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/xfree86/drivers/modesetting/dri2.c
> > b/hw/xfree86/drivers/modesetting/dri2.c
> > index fd36aa118..96ef7 100644
> > --- a/hw/xfree86/drivers/modesetting/dri2.c
> > +++ b/hw/xfree86/drivers/modesetting/dri2.c
> > @@ -749,7 +749,7 @@ ms_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr
> > draw, CARD64 target_msc,
> >  target_msc = current_msc;
> >  
> >  ret = ms_queue_vblank(crtc, MS_QUEUE_ABSOLUTE, target_msc,
> > _msc, seq);
> > -if (ret) {
> > +if (!ret) {
> >  static int limit = 5;
> >  if (limit) {
> >  xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> 


A few lines later there is:

if (!ms_queue_vblank(crtc, MS_QUEUE_ABSOLUTE, request_msc, _msc,
seq)) {
static int limit = 5;
if (limit) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "%s:%d get vblank counter failed: %s\n",
   __FUNCTION__, __LINE__,
   strerror(errno));
limit--;
}
goto out_free;
}

perhaps that can be changed into the same style ?
Either with or without ret i do not care but the same.

re,
 wh


> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/2] modesetting: Don't reuse iterator in nested loop

[Walter Harms]

> Daniel Stone <dan...@fooishbar.org> hat am 5. April 2018 um 16:08 geschrieben:
> 
> 
> On 5 April 2018 at 15:06, Walter Harms <wha...@bfs.de> wrote:
> >> Daniel Stone <dani...@collabora.com> hat am 5. April 2018 um 15:58
> >> geschrieben:
> >> -for (i = 0; i < xf86_config->num_output; i++) {
> >> -xf86OutputPtr output = xf86_config->output[i];
> >> +for (j = 0; i < xf86_config->num_output; j++) {
> >> +xf86OutputPtr output = xf86_config->output[j];
> >
> > Your intention is to loop over J but terminate with i ?
> > that is at least confusing.
> 
> Today really isn't my day. Apologies.


on the positiv side somebody is actualy reading the code  :)

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/2] modesetting: Don't reuse iterator in nested loop

[Walter Harms]

> Daniel Stone  hat am 5. April 2018 um 15:58
> geschrieben:
> 
> 
> drmmode_crtc_set_mode has a loop nested inside another loop, where both
> of them were using 'i' as the loop iterator. Rename it to avoid an
> infinite loop.
> 
> Signed-off-by: Daniel Stone 
> Reported-by: Michel Dänzer 
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 0eacefba2..f5f9fa582 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -778,12 +778,13 @@ drmmode_crtc_set_mode(xf86CrtcPtr crtc, Bool test_only)
>  drmmode_crtc_private_ptr other_drmmode_crtc =
> other_crtc->driver_private;
>  int lost_outputs = 0;
>  int remaining_outputs = 0;
> + int j;
>  
>  if (other_crtc == crtc)
>  continue;
>  
> -for (i = 0; i < xf86_config->num_output; i++) {
> -xf86OutputPtr output = xf86_config->output[i];
> +for (j = 0; i < xf86_config->num_output; j++) {
> +xf86OutputPtr output = xf86_config->output[j];

Your intention is to loop over J but terminate with i ?
that is at least confusing.

re,
 wh


>  drmmode_output_private_ptr drmmode_output =
> output->driver_private;
>  
>  if (drmmode_output->current_crtc == other_crtc) {
> -- 
> 2.17.0
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libXaw] Fix xload crashes if the window is wider than 2048 pixels

[Walter Harms]

> Alan Coopersmith  hat am 25. März 2018 um 07:45
> geschrieben:
> 
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=96670
> 
> Signed-off-by: Alan Coopersmith 
> ---
> 
> I wasn't sure if the size of valuedata could be changed without breaking
> the ABI, since it's embedded in the class structure, so I just added
> bounds checks to keep us from exceeding it.   If you need more than 2048
> points of load average data, you probably want a better monitoring tool
> than xload anyway.
> 
>  src/StripChart.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/StripChart.c b/src/StripChart.c
> index fa7357c..401036b 100644
> --- a/src/StripChart.c
> +++ b/src/StripChart.c
> @@ -57,6 +57,8 @@ SOFTWARE.
>  #include "Private.h"
>  
>  #define MS_PER_SEC 1000
> +#define NUM_VALUEDATA(w) (sizeof((w)->strip_chart.valuedata) / \
> +  sizeof((w)->strip_chart.valuedata[0]))
>  

A more common name would be ARRAY_SIZE 

>  /*
>   * Class Methods
> @@ -317,7 +319,8 @@ draw_it(XtPointer client_data, XtIntervalId *id)
>   w->strip_chart.update * MS_PER_SEC,draw_it,
>   client_data);
>  
> -if (w->strip_chart.interval >= XtWidth(w))
> +if ((w->strip_chart.interval >= XtWidth(w)) ||
> +(w->strip_chart.interval >= NUM_VALUEDATA(w)))
>   MoveChart((StripChartWidget)w, True);
>  
>  /* Get the value, stash the point and draw corresponding line */
> @@ -410,6 +413,9 @@ repaint_window(StripChartWidget w, int left, int width)
>   if (next < ++width)
>   width = next;
>  
> + if (width > NUM_VALUEDATA(w))
> + width = NUM_VALUEDATA(w);
> +
>   /* Draw data point lines */
>   for (i = left; i < width; i++) {
>   int y = XtHeight(w) - (XtHeight(w) * w->strip_chart.valuedata[i])
> @@ -449,12 +455,16 @@ MoveChart(StripChartWidget w, Bool blit)
>  if (!XtIsRealized((Widget)w))
>   return;
>  
> +if (XtWidth(w) > NUM_VALUEDATA(w))
> + j = (int) NUM_VALUEDATA(w);
> +else
> + j = (int) XtWidth(w);
>  if (w->strip_chart.jump_val < 0)
>   w->strip_chart.jump_val = DEFAULT_JUMP;
>  if (w->strip_chart.jump_val == DEFAULT_JUMP)
> - j = XtWidth(w) >> 1;
> + j = j >> 1;
>  else {
> - j = (int)XtWidth(w) - w->strip_chart.jump_val;
> + j -= w->strip_chart.jump_val;
>   if (j < 0)
>   j = 0;
>  }
> -- 
> 2.15.0
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] present: cap the version returned to the client

[Walter Harms]

> Emil Velikov  hat am 19. März 2018 um 17:04
> geschrieben:
> 
> 
> From: Emil Velikov 
> 
> As per the protocol, the server should not return version greater than
> the one supported by the client.
> 
> Add a spec quote and tweak the numbers accordingly.
> 
> Fixes: 5c5c1b77982 ("present: Add Present extension")
> Cc: Thierry Reding 
> Cc: Daniel Stone 
> Cc: Keith Packard 
> Signed-off-by: Emil Velikov 
> ---
> Analogous to the DRI3 patch here
> https://patchwork.freedesktop.org/patch/210343/
> ---
>  present/present_request.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/present/present_request.c b/present/present_request.c
> index c6afe5fa7..f52efa52b 100644
> --- a/present/present_request.c
> +++ b/present/present_request.c
> @@ -41,7 +41,19 @@ proc_present_query_version(ClientPtr client)
>  };
>  
>  REQUEST_SIZE_MATCH(xPresentQueryVersionReq);
> -(void) stuff;
> +/* From presentproto:
> + *
> + * The client sends the highest supported version to the server
> + * and the server sends the highest version it supports, but no
> + * higher than the requested version.
> + */

Alternativ wording:
/*
 * The client sends the highest supported version to the server
 * and the server replies with less or equal the requested version
 * what ever is supported.
 */

I would suggest to avoid "no" as people tend to skip these tiny word.
Simply saying "less or equal the requested version" make thinks clear.
Maybe some native has a better wording (if that is good it should go to
presentproto also).

re,
 wh

> +
> +if (rep.majorVersion > stuff->majorVersion ||
> +rep.minorVersion > stuff->minorVersion) {
> +rep.majorVersion = stuff->majorVersion;
> +rep.minorVersion = stuff->minorVersion;
> +}
> +
>  if (client->swapped) {
>  swaps();
>  swapl();
> -- 
> 2.16.0
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] Use flexible array member instead of fake size.

[Walter Harms]

> Michal Srb  hat am 15. März 2018 um 09:50 geschrieben:
> 
> 
> The _XimCacheStruct structure is followed in memory by two strings containing
> fname and encoding. The memory was accessed using the last member of the
> structure `char fname[1]`. That is a lie, prohibits us from using sizeof and
> confuses checkers. Lets declare it properly as a flexible array, so compilers
> don't complain about writing past that array. As bonus we can replace the
> XOffsetOf with regular sizeof.
> 
> Fixes GCC8 error:
>   In function 'strcpy',
>   inlined from '_XimWriteCachedDefaultTree' at imLcIm.c:479:5,
>   inlined from '_XimCreateDefaultTree' at imLcIm.c:616:2,
>   inlined from '_XimLocalOpenIM' at imLcIm.c:700:5:
>   /usr/include/bits/string_fortified.h:90:10: error: '__builtin_strcpy'
>   forming offset 2 is out of the bounds [0, 1] [-Werror=array-bounds]
>  return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
> 
> Caused by this line seemingly writing past the fname[1] array:
>   imLcIm.c:479:  strcpy (m->fname+strlen(name)+1, encoding);
> ---
>  modules/im/ximcp/imLcIm.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/modules/im/ximcp/imLcIm.c b/modules/im/ximcp/imLcIm.c
> index c19695df..743df77b 100644
> --- a/modules/im/ximcp/imLcIm.c
> +++ b/modules/im/ximcp/imLcIm.c
> @@ -82,8 +82,8 @@ struct _XimCacheStruct {
>  DTCharIndex mbused;
>  DTCharIndex wcused;
>  DTCharIndex utf8used;
> -charfname[1];
> -/* char encoding[1] */
> +charfname[];
> +/* char encoding[] */
>  };


looks good to me.

re,
 wh

>  
>  static struct  _XimCacheStruct* _XimCache_mmap = NULL;
> @@ -281,7 +281,7 @@ _XimReadCachedDefaultTree(
>  assert (m->id == XIM_CACHE_MAGIC);
>  assert (m->version == XIM_CACHE_VERSION);
>  if (size != m->size ||
> - size < XOffsetOf (struct _XimCacheStruct, fname) + namelen + 
> encodinglen) {
> + size < sizeof (struct _XimCacheStruct) + namelen + encodinglen) {
>   fprintf (stderr, "Ignoring broken XimCache %s [%s]\n", name, encoding);
>  munmap (m, size);
>  return False;
> @@ -442,7 +442,7 @@ _XimWriteCachedDefaultTree(
>  int   fd;
>  FILE *fp;
>  struct _XimCacheStruct *m;
> -int   msize = (XOffsetOf(struct _XimCacheStruct, fname)
> +int   msize = (sizeof(struct _XimCacheStruct)
>  + strlen(name) + strlen(encoding) + 2
>  + XIM_CACHE_TREE_ALIGNMENT-1) & -XIM_CACHE_TREE_ALIGNMENT;
>  DefTreeBase *b = >private.local.base;
> -- 
> 2.13.6
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xorgproto] randr: MONITORINFO has outputs, not crtcs

[walter harms]

It would be nice to have an explanation why this change.

re,
 wh


Am 06.02.2018 02:03, schrieb Giuseppe Bilotta:
> ---
>  randrproto.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/randrproto.txt b/randrproto.txt
> index c06bc90..f57301d 100644
> --- a/randrproto.txt
> +++ b/randrproto.txt
> @@ -2363,14 +2363,14 @@ A.1.1 Common Types added in version 1.5 of the 
> protocol
>   4   ATOMname
>   1   BOOLprimary
>   1   BOOLautomatic
> - 2   CARD16  ncrtcs
> + 2   CARD16  noutputs
>   2   INT16   x
>   2   INT16   y
>   2   CARD16  width in pixels
>   2   CARD16  height in pixels
>   4   CARD32  width in millimeters
>   4   CARD32  height in millimeters
> - 4*n CRTCcrtcs
> + 4*n OUTPUT  outputs
>  └───
>  
>  A.2 Protocol Requests
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xrandr v4 2/5] xrandr: stricter --scale argument parsing

[walter harms]

Am 05.02.2018 11:24, schrieb Giuseppe Bilotta:
> On Mon, Feb 5, 2018 at 10:44 AM, walter harms <wha...@bfs.de> wrote:
>>
>> Am 05.02.2018 02:47, schrieb Giuseppe Bilotta:
>>>   {
>>>   double  sx, sy;
>>> + char junk;
>>>   if (!config_output) argerr ("%s must be used after --output\n", 
>>> argv[i]);
>>>   if (++i >= argc) argerr ("%s requires an argument\n", argv[i-1]);
>>> - if (sscanf (argv[i], "%lfx%lf", , ) != 2)
>>> + if (sscanf (argv[i], "%lfx%lf%c", , , ) != 2)
>>>   {
>>> - if (sscanf (argv[i], "%lf", ) != 1)
>>> + if (sscanf (argv[i], "%lf%c", , ) != 1)
>>>   argerr ("failed to parse '%s' as a scaling factor\n", 
>>> argv[i]);
>>>   sy = sx;
>>>   }
>>
>> can the scanf be converted to strtod ? there you get an endpointer by 
>> default.
> 
> I'm not a big fan of strtod because with it it's impossible to know if
> a conversion actually happened. xrandr --scale '  ' would actually be
> accepted (resulting in a scale value of 0), while the scanf catches
> it. For the same reason I use two sscanf instead of a single one
> (because that wouldn't be able to catch something like xrandr --scale
> 1j).
> 
> Of course there's also to be said that we could reject a scale factor
> of 0, regardless of whether it comes from a correct parsing of the
> string '0.0' or from the parse of an empty string (but of course then
> we couldn't customize the error message to differentiate between
> “incorrect parse” and “value out of range”).
> 

Ok, i understand. My guess is that a scale factor of 0 would be rejected
or interessting things will happen 

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xrandr v4 2/5] xrandr: stricter --scale argument parsing

[walter harms]

Am 05.02.2018 02:47, schrieb Giuseppe Bilotta:
> We used to accept something like --scale 2x3junk as a valid input
> (scaling x by 2 and y by 3), even though this isn't really a valid
> scaling factor.
> 
> Fix by making sure there is nothing after the parsed number(s).
> 
> Signed-off-by: Giuseppe Bilotta 
> ---
>  xrandr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xrandr.c b/xrandr.c
> index 1085a95..6a38cf2 100644
> --- a/xrandr.c
> +++ b/xrandr.c
> @@ -3014,11 +3014,12 @@ main (int argc, char **argv)
>   if (!strcmp ("--scale", argv[i]))
>   {
>   double  sx, sy;
> + char junk;
>   if (!config_output) argerr ("%s must be used after --output\n", 
> argv[i]);
>   if (++i >= argc) argerr ("%s requires an argument\n", argv[i-1]);
> - if (sscanf (argv[i], "%lfx%lf", , ) != 2)
> + if (sscanf (argv[i], "%lfx%lf%c", , , ) != 2)
>   {
> - if (sscanf (argv[i], "%lf", ) != 1)
> + if (sscanf (argv[i], "%lf%c", , ) != 1)
>   argerr ("failed to parse '%s' as a scaling factor\n", 
> argv[i]);
>   sy = sx;
>   }

can the scanf be converted to strtod ? there you get an endpointer by default.

re,
 wh


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-vesa 3/4] Use VBEFreeVBEInfo not free

[walter harms]

Am 31.01.2018 16:48, schrieb Adam Jackson:
> A VbeInfoBlock has substructure, just freeing the object will leak.
> Unfortunately VBEFreeVBEInfo does not check for NULL first so we have
> to.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=25029
> Signed-off-by: Adam Jackson 
> ---
>  src/vesa.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/vesa.c b/src/vesa.c
> index 3f5b81c..2d3c10d 100644
> --- a/src/vesa.c
> +++ b/src/vesa.c
> @@ -596,7 +596,8 @@ VESAFreeRec(ScrnInfoPtr pScrn)
>  }
>  #endif
>  free(pVesa->monitor);
> -free(pVesa->vbeInfo);
> +if (pVesa->vbeInfo)
> + VBEFreeVBEInfo(pVesa->vbeInfo);

since all free function accept NULL these days it may be more efficient to
teach VBEFreeVBEInfo to accept NULL.

just my 2 cents,
re,
 wh

>  free(pVesa->pal);
>  free(pVesa->savedPal);
>  free(pVesa->fonts);
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] os: Fix strtok/free crash in ComputeLocalClient

[walter harms]

Am 06.12.2017 12:16, schrieb Tomasz Śniatowski:
> Don't reuse cmd for strtok output to ensure the proper pointer is
> freed afterwards.
> 
> The code incorrectly assumed the pointer returned by strtok(cmd, ":")
> would always point to cmd. However, strtok(str, sep) != str if str
> begins with sep. This caused an invalid-free crash when running
> a program under X with a name beginning with a colon.
> 
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=104123
> 

Yes, strtok returns a pointer to its arguments.
btw: strtok is not thread-safe, could that be an issue with that function ?

re,
 wh

> Signed-off-by: Tomasz Śniatowski 
> ---
>  os/access.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/os/access.c b/os/access.c
> index 8828e0834..97246160c 100644
> --- a/os/access.c
> +++ b/os/access.c
> @@ -1137,12 +1137,12 @@ ComputeLocalClient(ClientPtr client)
>  /* Cut off any colon and whatever comes after it, see
>   * 
> https://lists.freedesktop.org/archives/xorg-devel/2015-December/048164.html
>   */
> -cmd = strtok(cmd, ":");
> +char *tok = strtok(cmd, ":");
>  
>  #if !defined(WIN32) || defined(__CYGWIN__)
> -ret = strcmp(basename(cmd), "ssh") != 0;
> +ret = strcmp(basename(tok), "ssh") != 0;
>  #else
> -ret = strcmp(cmd, "ssh") != 0;
> +ret = strcmp(tok, "ssh") != 0;
>  #endif
>  
>  free(cmd);
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Xstartup handle DISPLAY=(null) correctly

[walter harms]

at what circumstances can DISPLAY == '(null)' ?

looks more like a bug in an Xserver.

re,
 wh


Am 20.11.2017 06:40, schrieb xiaoguang wang:
> 
> 
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] xfree86: fix gamma compute when palette_size > 256

[walter harms]

Am 30.10.2017 07:33, schrieb Qiang Yu:
> palette_(red|green|blue)_size > crtc->gamma_size (=256)
> this may happen when screen has per RGB chanel > 8bit,
> i.e. 30bit depth screen 10bit per RGB.
> 
> Signed-off-by: Qiang Yu 
> ---
>  hw/xfree86/modes/xf86RandR12.c | 96 
> ++
>  1 file changed, 69 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
> index fe8770d..881c8a6 100644
> --- a/hw/xfree86/modes/xf86RandR12.c
> +++ b/hw/xfree86/modes/xf86RandR12.c
> @@ -1258,40 +1258,82 @@ xf86RandR12CrtcComputeGamma(xf86CrtcPtr crtc, LOCO 
> *palette,
>  
>  for (shift = 0; (gamma_size << shift) < (1 << 16); shift++);
>  
> -gamma_slots = crtc->gamma_size / palette_red_size;
> -for (i = 0; i < palette_red_size; i++) {
> -value = palette[i].red;
> -if (gamma_red)
> -value = gamma_red[value];
> -else
> -value <<= shift;
> +if (crtc->gamma_size >= palette_red_size) {

minor nitpick:

IMHO: if you use if (crtc->gamma_size < palette_red_size) and switch the
blocks it is more easy to read.

And since the if () are a bit large, did you consider to make it into
3 separate functions to improve readability ?

re,
 wh

> +gamma_slots = crtc->gamma_size / palette_red_size;
> +for (i = 0; i < palette_red_size; i++) {
> +value = palette[i].red;
> +if (gamma_red)
> +value = gamma_red[value];
> +else
> +value <<= shift;
>  
> -for (j = 0; j < gamma_slots; j++)
> -crtc->gamma_red[i * gamma_slots + j] = value;
> +for (j = 0; j < gamma_slots; j++)
> +crtc->gamma_red[i * gamma_slots + j] = value;
> +}
>  }
> +else {
> +gamma_slots = palette_red_size / crtc->gamma_size;
> +for (i = 0; i < crtc->gamma_size; i++) {
> +value = palette[i * gamma_slots].red;
> +if (gamma_red)
> +value = gamma_red[value];
> +else
> +value <<= shift;
>  
> -gamma_slots = crtc->gamma_size / palette_green_size;
> -for (i = 0; i < palette_green_size; i++) {
> -value = palette[i].green;
> -if (gamma_green)
> -value = gamma_green[value];
> -else
> -value <<= shift;
> +crtc->gamma_red[i] = value;
> +}
> +}
> +
> +if (crtc->gamma_size >= palette_green_size) {
> +gamma_slots = crtc->gamma_size / palette_green_size;
> +for (i = 0; i < palette_green_size; i++) {
> +value = palette[i].green;
> +if (gamma_green)
> +value = gamma_green[value];
> +else
> +value <<= shift;
> +
> +for (j = 0; j < gamma_slots; j++)
> +crtc->gamma_green[i * gamma_slots + j] = value;
> +}
> +}
> +else {
> +gamma_slots = palette_green_size / crtc->gamma_size;
> +for (i = 0; i < crtc->gamma_size; i++) {
> +value = palette[i * gamma_slots].green;
> +if (gamma_green)
> +value = gamma_green[value];
> +else
> +value <<= shift;
>  
> -for (j = 0; j < gamma_slots; j++)
> -crtc->gamma_green[i * gamma_slots + j] = value;
> +crtc->gamma_green[i] = value;
> +}
>  }
>  
> -gamma_slots = crtc->gamma_size / palette_blue_size;
> -for (i = 0; i < palette_blue_size; i++) {
> -value = palette[i].blue;
> -if (gamma_blue)
> -value = gamma_blue[value];
> -else
> -value <<= shift;
> +if (crtc->gamma_size >= palette_blue_size) {
> +gamma_slots = crtc->gamma_size / palette_blue_size;
> +for (i = 0; i < palette_blue_size; i++) {
> +value = palette[i].blue;
> +if (gamma_blue)
> +value = gamma_blue[value];
> +else
> +value <<= shift;
>  
> -for (j = 0; j < gamma_slots; j++)
> -crtc->gamma_blue[i * gamma_slots + j] = value;
> +for (j = 0; j < gamma_slots; j++)
> +crtc->gamma_blue[i * gamma_slots + j] = value;
> +}
> +}
> +else {
> +gamma_slots = palette_blue_size / crtc->gamma_size;
> +for (i = 0; i < crtc->gamma_size; i++) {
> +value = palette[i * gamma_slots].blue;
> +if (gamma_blue)
> +value = gamma_blue[value];
> +else
> +value <<= shift;
> +
> +crtc->gamma_blue[i] = value;
> +}
>  }
>  }
>  
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libXau 0/3] remove redundant null check on calling free()

[walter harms]

Am 28.10.2017 21:18, schrieb Daniel Martin:
> Am 28.10.2017 19:16 schrieb "walter harms" <wha...@bfs.de>:
> 
> After the last patch for libXau i checked the code
> with smatch and started to remove remove redundant
> 
> 
> One redundant "remove" here. ;-)
> 


aehm, yes totally absolutely gone :)

re,
 wh

> All 3 patches are
> Reviewed-by: Daniel Martin <consume.no...@gmail.com>
> 
> 
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libXau 3/3] AuRead.c: remove redundant null check on calling free()

[walter harms]

this removes simply unneeded code from XauReadAuth

Signed-off-by: Walter Harms <wha...@bfs.de>
---
 AuRead.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/AuRead.c b/AuRead.c
index 5d41f03..d48906b 100644
--- a/AuRead.c
+++ b/AuRead.c
@@ -77,25 +77,25 @@ XauReadAuth (FILE *auth_file)
 if (read_counted_string (_length, , auth_file) 
== 0)
return NULL;
 if (read_counted_string (_length, , auth_file) 
== 0) {
-   if (local.address) free (local.address);
+   free (local.address);
return NULL;
 }
 if (read_counted_string (_length, , auth_file) == 0) 
{
-   if (local.address) free (local.address);
-   if (local.number) free (local.number);
+   free (local.address);
+   free (local.number);
return NULL;
 }
 if (read_counted_string (_length, , auth_file) == 0) 
{
-   if (local.address) free (local.address);
-   if (local.number) free (local.number);
-   if (local.name) free (local.name);
+   free (local.address);
+   free (local.number);
+   free (local.name);
return NULL;
 }
 ret = (Xauth *) malloc (sizeof (Xauth));
 if (!ret) {
-   if (local.address) free (local.address);
-   if (local.number) free (local.number);
-   if (local.name) free (local.name);
+   free (local.address);
+   free (local.number);
+   free (local.name);
if (local.data) {
bzero (local.data, local.data_length);
free (local.data);
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libXau 2/3]Au FileName.c: remove redundant null check on calling free()

[walter harms]

remove redundant null check on calling free()
Signed-off-by: Walter Harms <wha...@bfs.de>
---
 AuFileName.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/AuFileName.c b/AuFileName.c
index 2946c80..4ccda9d 100644
--- a/AuFileName.c
+++ b/AuFileName.c
@@ -68,8 +68,7 @@ XauFileName (void)
 }
 size = strlen (name) + strlen([1]) + 2;
 if ((size > bsize) || (buf == NULL)) {
-   if (buf)
-   free (buf);
+   free (buf);
 assert(size > 0);
buf = malloc (size);
if (!buf) {
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libXau 1/3] AuDispose.c:remove redundant null check on calling free()

[walter harms]

redundant null check on auth->address calling free()
redundant null check on auth->number calling free()
redundant null check on auth->name calling free()

Signed-off-by: Walter Harms <wha...@bfs.de>
---
 AuDispose.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/AuDispose.c b/AuDispose.c
index 2a9b2f1..355224d 100644
--- a/AuDispose.c
+++ b/AuDispose.c
@@ -34,9 +34,9 @@ void
 XauDisposeAuth (Xauth *auth)
 {
 if (auth) {
-   if (auth->address) (void) free (auth->address);
-   if (auth->number) (void) free (auth->number);
-   if (auth->name) (void) free (auth->name);
+   free (auth->address);
+   free (auth->number);
+   free (auth->name);
if (auth->data) {
(void) bzero (auth->data, auth->data_length);
(void) free (auth->data);
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libXau 0/3] remove redundant null check on calling free()

[walter harms]

After the last patch for libXau i checked the code
with smatch and started to remove remove redundant
null checks. just some cleanup.

re,
 wh
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libXau] Avoid out of boundary read access

[walter harms]

Am 19.10.2017 22:18, schrieb Tobias Stoeckmann:
> If the environment variable HOME is empty, XauFileName triggers an
> out of boundary read access (name[1]). 

true but if HOME="" perhaps we could simply return

If HOME consists of a single
> character relative path, the output becomes unexpected, because
> "HOME=a" leads to "a.Xauthority" instead of "a/.Xauthority". Granted,
> a relative HOME path leads to trouble in general, the code should
> properly return "a/.Xauthority" nonetheless.

Why is that massage for slashDotXauthority done in the first place ?
if we drop it:
HOME  + "/.Xauthority" =
""+ "/.Xauthority" = "/.Xauthority"
"a"   + "/.Xauthority" = "a/.Xauthority"
"a/"  + "/.Xauthority" = "a//.Xauthority"

a "//" will be condensed to "/" by the system

did i miss something ?

re,
 wh

> 
> Signed-off-by: Tobias Stoeckmann 
> ---
>  AuFileName.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/AuFileName.c b/AuFileName.c
> index 37c8b62..2946c80 100644
> --- a/AuFileName.c
> +++ b/AuFileName.c
> @@ -85,6 +85,6 @@ XauFileName (void)
>   bsize = size;
>  }
>  snprintf (buf, bsize, "%s%s", name,
> -  slashDotXauthority + (name[1] == '\0' ? 1 : 0));
> +  slashDotXauthority + (name[0] == '/' && name[1] == '\0' ? 1 : 
> 0));
>  return buf;
>  }
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 13/13] make sure buffer is zero filled and report if, allocation failed

[walter harms]

 make sure buffer is zero filled and
 report if allocation failed

 Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/listenwk.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/listenwk.c b/src/listenwk.c
index 7517ea8..bc1da76 100644
--- a/src/listenwk.c
+++ b/src/listenwk.c
@@ -64,11 +64,14 @@ IceListenForWellKnownConnections (
return (0);
 }

-if ((listenObjs = malloc (transCount * sizeof (struct _IceListenObj))) == 
NULL)
+listenObjs = calloc (transCount, sizeof (struct _IceListenObj));
+if ( listenObjs == NULL)
 {
for (i = 0; i < transCount; i++)
_IceTransClose (transConns[i]);
free (transConns);
+
+strncpy (errorStringRet, "Malloc failed", errorLength);
return (0);
 }

-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 12/13] add check for malloc

[walter harms]

fix a potential null pointer deference error and

IceAllocScratch() do not report size when allocation failes

Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/misc.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/misc.c b/src/misc.c
index 87d6335..fdc671d 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -57,7 +57,10 @@ IceAllocScratch (
free (iceConn->scratch);

iceConn->scratch = malloc (size);
-   iceConn->scratch_size = size;
+   if ( !iceConn->scratch )
+iceConn->scratch_size = 0;
+   else
+iceConn->scratch_size = size;
 }

 return (iceConn->scratch);
@@ -415,12 +418,14 @@ _IceAddOpcodeMapping (
 )
 {
 if (hisOpcode <= 0 || hisOpcode > 255)
-{
return;
-}
-else if (iceConn->process_msg_info == NULL)
+
+if (iceConn->process_msg_info == NULL)
 {
iceConn->process_msg_info = malloc (sizeof (_IceProcessMsgInfo));
+   if ( ! iceConn->process_msg_info )
+ return;
+
iceConn->his_min_opcode = iceConn->his_max_opcode = hisOpcode;
 }
 else if (hisOpcode < iceConn->his_min_opcode)
@@ -433,6 +438,9 @@ _IceAddOpcodeMapping (
iceConn->process_msg_info = malloc (
newsize * sizeof (_IceProcessMsgInfo));

+   if ( ! iceConn->process_msg_info )
+ return;
+
memcpy (>process_msg_info[
iceConn->his_min_opcode - hisOpcode], oldVec,
oldsize * sizeof (_IceProcessMsgInfo));
@@ -460,6 +468,9 @@ _IceAddOpcodeMapping (
iceConn->process_msg_info = malloc (
newsize * sizeof (_IceProcessMsgInfo));

+   if ( ! iceConn->process_msg_info )
+ return;
+
memcpy (iceConn->process_msg_info, oldVec,
oldsize * sizeof (_IceProcessMsgInfo));

-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 11/13] add check for mall

[walter harms]

fix a potential null pointer deference error and
properly dispose the affected message

 Signed-off-by: Walter Harms <wha...@bfs.de>
---
 src/process.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/src/process.c b/src/process.c
index a9a8d08..89f0403 100644
--- a/src/process.c
+++ b/src/process.c
@@ -923,6 +923,14 @@ ProcessConnectionSetup (
 if ((hisAuthCount = message->authCount) > 0)
 {
hisAuthNames = malloc (hisAuthCount * sizeof (char *));
+
+   if (!hisAuthNames)
+   {
+   iceConn->connection_status = IceConnectRejected;
+   IceDisposeCompleteMessage (iceConn, pStart);
+   return (0);
+   }
+
EXTRACT_LISTOF_STRING (pData, swap, hisAuthCount, hisAuthNames);
 }

@@ -1058,6 +1066,13 @@ ProcessConnectionSetup (
iceConn->connect_to_me = setupInfo =
malloc (sizeof (_IceConnectToMeInfo));

+   if (!iceConn->connect_to_me)
+   {
+   iceConn->connection_status = IceConnectRejected;
+   IceDisposeCompleteMessage (iceConn, pStart);
+   return (0);
+   }
+
setupInfo->my_version_index = myVersionIndex;
setupInfo->his_version_index = hisVersionIndex;
setupInfo->his_vendor = vendor;
@@ -1961,6 +1976,12 @@ ProcessProtocolSetup (
 if ((hisAuthCount = message->authCount) > 0)
 {
hisAuthNames = malloc (hisAuthCount * sizeof (char *));
+   if (!hisAuthNames)
+   {
+   IceDisposeCompleteMessage (iceConn, pStart);
+   return (0);
+   }
+
EXTRACT_LISTOF_STRING (pData, swap, hisAuthCount, hisAuthNames);
 }

@@ -2091,6 +2112,12 @@ ProcessProtocolSetup (
iceConn->protosetup_to_me = setupInfo =
malloc (sizeof (_IceProtoSetupToMeInfo));

+   if (!iceConn->protosetup_to_me)
+   {
+   IceDisposeCompleteMessage (iceConn, pStart);
+   return (0);
+   }
+
setupInfo->his_opcode = hisOpcode;
setupInfo->my_opcode = myOpcode;
setupInfo->my_version_index = myVersionIndex;
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 10/13] add check for malloc

[walter harms]

From ea066aa04dd118187ca0289053bc4ca5caa0a4a8 Mon Sep 17 00:00:00 2001


fix a potential null pointer deference error
convert malloc() to calloc() to have valid
null pointers on error. so we can release
already allocated memory

Signed-off-by: Walter Harms <wha...@bfs.de>
---
 src/register.c | 53 ++---
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/src/register.c b/src/register.c
index 833714b..2417dd7 100644
--- a/src/register.c
+++ b/src/register.c
@@ -67,7 +67,9 @@ IceRegisterForProtocolSetup (

 if (i <= _IceLastMajorOpcode)
 {
-   p = _IceProtocols[i - 1].orig_client = malloc (sizeof(_IcePoProtocol));
+p = _IceProtocols[i - 1].orig_client = calloc 
(1,sizeof(_IcePoProtocol));
+   if (!p)
+ return (-1);
opcodeRet = i;
 }
 else if (_IceLastMajorOpcode == 255 ||
@@ -82,7 +84,9 @@ IceRegisterForProtocolSetup (
strdup(protocolName);

p = _IceProtocols[_IceLastMajorOpcode].orig_client =
-   malloc (sizeof (_IcePoProtocol));
+   calloc (1,sizeof (_IcePoProtocol));
+   if (!p)
+ return (-1);

_IceProtocols[_IceLastMajorOpcode].accept_client = NULL;

@@ -95,15 +99,20 @@ IceRegisterForProtocolSetup (
 p->version_count = versionCount;

 p->version_recs = malloc (versionCount * sizeof (IcePoVersionRec));
+if (!p->version_recs)
+goto out_of_memory;
+
 memcpy (p->version_recs, versionRecs,
versionCount * sizeof (IcePoVersionRec));

 if ((p->auth_count = authCount) > 0)
 {
p->auth_names = malloc (authCount * sizeof (char *));
-
+   if (!p->auth_names);
+goto out_of_memory;
p->auth_procs = malloc (authCount * sizeof (IcePoAuthProc));
-
+   if (!p->auth_names);
+goto out_of_memory;
for (i = 0; i < authCount; i++)
{
p->auth_names[i] = strdup(authNames[i]);
@@ -119,6 +128,15 @@ IceRegisterForProtocolSetup (
 p->io_error_proc = IOErrorProc;

 return (opcodeRet);
+
+out_of_memory:
+free(p->auth_procs);
+free(p->auth_names);
+free(p->version_recs);
+free(p->release);
+free(p->vendor);
+free(p);
+return (-1);
 }


@@ -163,7 +181,10 @@ IceRegisterForProtocolReply (
 if (i <= _IceLastMajorOpcode)
 {
p = _IceProtocols[i - 1].accept_client =
-   malloc (sizeof (_IcePaProtocol));
+ calloc (1,sizeof (_IcePaProtocol));
+   if (!p)
+ return (-1);
+
opcodeRet = i;
 }
 else if (_IceLastMajorOpcode == 255 ||
@@ -180,7 +201,9 @@ IceRegisterForProtocolReply (
_IceProtocols[_IceLastMajorOpcode].orig_client = NULL;

p = _IceProtocols[_IceLastMajorOpcode].accept_client =
-   malloc (sizeof (_IcePaProtocol));
+ calloc (1,sizeof (_IcePaProtocol));
+   if (!p)
+ return (-1);

opcodeRet = ++_IceLastMajorOpcode;
 }
@@ -191,6 +214,9 @@ IceRegisterForProtocolReply (
 p->version_count = versionCount;

 p->version_recs = malloc (versionCount * sizeof (IcePaVersionRec));
+if (!p->version_recs)
+goto out_of_memory;
+
 memcpy (p->version_recs, versionRecs,
versionCount * sizeof (IcePaVersionRec));

@@ -200,8 +226,12 @@ IceRegisterForProtocolReply (
 if ((p->auth_count = authCount) > 0)
 {
p->auth_names = malloc (authCount * sizeof (char *));
+   if (!p->auth_names);
+goto out_of_memory;

p->auth_procs = malloc (authCount * sizeof (IcePaAuthProc));
+   if (!p->auth_names);
+goto out_of_memory;

for (i = 0; i < authCount; i++)
{
@@ -220,5 +250,14 @@ IceRegisterForProtocolReply (
 p->io_error_proc = IOErrorProc;

 return (opcodeRet);
-}

+out_of_memory:
+free(p->auth_procs);
+free(p->auth_names);
+free(p->version_recs);
+free(p->release);
+free(p->vendor);
+free(p);
+return (-1);
+
+}
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 09/13] add check for malloc

[walter harms]

fix a potential null pointer deference error

Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/replywait.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/replywait.c b/src/replywait.c
index b25c351..91a8dd6 100644
--- a/src/replywait.c
+++ b/src/replywait.c
@@ -60,7 +60,8 @@ _IceAddReplyWait (
 }

 savedReplyWait = malloc (sizeof (_IceSavedReplyWait));
-
+if (!savedReplyWait)
+  return;
 savedReplyWait->reply_wait = replyWait;
 savedReplyWait->reply_ready = False;
 savedReplyWait->next = NULL;
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 08/13] add check for malloc

[walter harms]

add check for malloc

fix a potential null pointer deference error and
release already allocated memory

Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/setauth.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/src/setauth.c b/src/setauth.c
index 1017b02..1f3e5cc 100644
--- a/src/setauth.c
+++ b/src/setauth.c
@@ -102,6 +102,19 @@ IceSetPaAuthData (
 entries[i].auth_data_length;
_IcePaAuthDataEntries[j].auth_data = malloc (
 entries[i].auth_data_length);
+   if (! _IcePaAuthDataEntries[j].auth_data)
+ {
+   /*
+  we can not inform the user about a botched update
+  but we can release the memory we already have
+   */
+
+   free(_IcePaAuthDataEntries[j].protocol_name);
+   free(_IcePaAuthDataEntries[j].network_id);
+   free(_IcePaAuthDataEntries[j].auth_name);
+   _IcePaAuthDataEntries[j].auth_data_length = 0;
+   return;
+ }
memcpy (_IcePaAuthDataEntries[j].auth_data,
 entries[i].auth_data, entries[i].auth_data_length);
 }
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 07/13] add check for malloc

[walter harms]

 add check for malloc and a bit untangling
 Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/watch.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/watch.c b/src/watch.c
index abbc265..c60d0c1 100644
--- a/src/watch.c
+++ b/src/watch.c
@@ -48,7 +48,8 @@ IceAddConnectionWatch (
 _IceWatchProc  *newWatchProc;
 inti;

-if ((newWatchProc = malloc (sizeof (_IceWatchProc))) == NULL)
+newWatchProc = malloc (sizeof (_IceWatchProc));
+if ( !newWatchProc )
 {
return (0);
 }
@@ -75,7 +76,11 @@ IceAddConnectionWatch (
 {
_IceWatchedConnection *newWatchedConn =
malloc (sizeof (_IceWatchedConnection));
-
+   if (!newWatchedConn)
+ {
+   IceRemoveConnectionWatch(watchProc,clientData);
+   return (0);
+ }
newWatchedConn->iceConn = _IceConnectionObjs[i];
newWatchedConn->next = NULL;

@@ -86,6 +91,7 @@ IceAddConnectionWatch (
 }

 return (1);
+
 }


@@ -143,6 +149,9 @@ _IceConnectionOpened (
malloc (sizeof (_IceWatchedConnection));
_IceWatchedConnection *watchedConn;

+   if (!newWatchedConn)
+ return ;
+
watchedConn = watchProc->watched_connections;
while (watchedConn && watchedConn->next)
watchedConn = watchedConn->next;
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 06/13] check malloc return

[walter harms]

 check malloc return
 failed mallocs will cause segfaults, so add check
 also free already allocated memory

 Signed-off-by: Walter Harms <wha...@bfs.de>
---
 src/connect.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index 276a356..b61449e 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -193,8 +193,8 @@ IceOpenConnection (

 iceConn->connect_to_me = NULL;
 iceConn->protosetup_to_me = NULL;
-
-if ((iceConn->inbuf = iceConn->inbufptr = malloc (ICE_INBUFSIZE)) == NULL)
+iceConn->inbuf = iceConn->inbufptr = malloc (ICE_INBUFSIZE);
+if ( iceConn->inbuf == NULL)
 {
_IceFreeConnection (iceConn);
strncpy (errorStringRet, "Can't malloc", errorLength);
@@ -202,9 +202,10 @@ IceOpenConnection (
 }

 iceConn->inbufmax = iceConn->inbuf + ICE_INBUFSIZE;
-
-if ((iceConn->outbuf = iceConn->outbufptr = calloc (1, ICE_OUTBUFSIZE)) == 
NULL)
+iceConn->outbuf = iceConn->outbufptr = calloc (1, ICE_OUTBUFSIZE);
+if ( iceConn->outbuf == NULL)
 {
+free(iceConn->inbuf);
_IceFreeConnection (iceConn);
strncpy (errorStringRet, "Can't malloc", errorLength);
return (NULL);
@@ -225,6 +226,14 @@ IceOpenConnection (
 iceConn->connect_to_you = malloc (sizeof (_IceConnectToYouInfo));
 iceConn->connect_to_you->auth_active = 0;

+if (!iceConn->connect_to_you) {
+free(iceConn->outbuf);
+free(iceConn->inbuf);
+   _IceFreeConnection (iceConn);
+   strncpy (errorStringRet, "Can't malloc", errorLength);
+   return (NULL);
+}
+
 /*
  * Send our byte order.
  */
@@ -467,6 +476,8 @@ ConnectToPeer (char *networkIdsList, char 
**actualConnectionRet)
 else
 {
address = malloc (len + 1);
+   if (!address)
+return (NULL);
address_size = len;
 }

-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 02/13] make IceProtocolShutdown() more readable

[walter harms]

 make IceProtocolShutdown() more readable

---
 src/shutdown.c | 47 ---
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/src/shutdown.c b/src/shutdown.c
index 90e9ded..98376a7 100644
--- a/src/shutdown.c
+++ b/src/shutdown.c
@@ -40,45 +40,38 @@ IceProtocolShutdown (
int majorOpcode
 )
 {
+int i;
+
 if (iceConn->proto_ref_count == 0 || iceConn->process_msg_info == NULL ||
 majorOpcode < 1 || majorOpcode > _IceLastMajorOpcode)
 {
return (0);
 }
-else
-{
-   /*
-* Make sure this majorOpcode is really being used.
-*/
-
-   int i;
+
+
+/*
+ * Make sure this majorOpcode is really being used.
+ */

-   for (i = iceConn->his_min_opcode; i <= iceConn->his_max_opcode; i++)
-   {
-   if (iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].in_use &&
-iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].my_opcode == majorOpcode)
-   break;
-   }
+for (i = iceConn->his_min_opcode; i <= iceConn->his_max_opcode; i++)
+  {
+   int n=i - iceConn->his_min_opcode;
+   if (iceConn->process_msg_info[n].in_use &&
+   iceConn->process_msg_info[n].my_opcode == majorOpcode)
+ {

-   if (i > iceConn->his_max_opcode)
-   {
-   return (0);
-   }
-   else
-   {
/*
 * OK, we can shut down the protocol.
 */

-   iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].in_use = False;
-   iceConn->proto_ref_count--;
+ iceConn->process_msg_info[n].in_use = False;
+ iceConn->proto_ref_count--;
+ return (1);
+ }
+   
+  }

-   return (1);
-   }
-}
+return (0);
 }


-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 05/13] add errormessage and remove unneeded indention

[walter harms]

add errormessage and remove unneeded indention

Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/listen.c | 51 +--
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/src/listen.c b/src/listen.c
index 9a449ae..54aabcd 100644
--- a/src/listen.c
+++ b/src/listen.c
@@ -67,6 +67,8 @@ IceListenForConnections (
for (i = 0; i < transCount; i++)
_IceTransClose (transConns[i]);
free (transConns);
+strncpy (errorStringRet,
+   "Out of memmory", errorLength);
return (0);
 }

@@ -186,6 +188,7 @@ IceComposeNetworkIdList (
 char *list;
 int len = 0;
 int i;
+int doneCount = 0;

 if (count < 1 || listenObjs == NULL)
return (NULL);
@@ -197,39 +200,35 @@ IceComposeNetworkIdList (

 if (list == NULL)
return (NULL);
-else
-{
-   int doneCount = 0;

-   list[0] = '\0';
+list[0] = '\0';

+for (i = 0; i < count; i++)
+  {
+   if (_IceTransIsLocal (listenObjs[i]->trans_conn))
+ {
+   strcat (list, listenObjs[i]->network_id);
+   doneCount++;
+   if (doneCount < count)
+ strcat (list, ",");
+ }
+  }
+
+if (doneCount < count)
+  {
for (i = 0; i < count; i++)
-   {
-   if (_IceTransIsLocal (listenObjs[i]->trans_conn))
-   {
+ {
+   if (!_IceTransIsLocal (listenObjs[i]->trans_conn))
+ {
strcat (list, listenObjs[i]->network_id);
doneCount++;
if (doneCount < count)
-   strcat (list, ",");
-   }
-   }
+ strcat (list, ",");
+ }
+ }
+  }

-   if (doneCount < count)
-   {
-   for (i = 0; i < count; i++)
-   {
-   if (!_IceTransIsLocal (listenObjs[i]->trans_conn))
-   {
-   strcat (list, listenObjs[i]->network_id);
-   doneCount++;
-   if (doneCount < count)
-   strcat (list, ",");
-   }
-   }
-   }
-
-   return (list);
-}
+return (list);
 }


-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 02/13] make IceProtocolShutdown() more readable

[walter harms]

 make IceProtocolShutdown() more readable

 Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/shutdown.c | 47 ---
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/src/shutdown.c b/src/shutdown.c
index 90e9ded..98376a7 100644
--- a/src/shutdown.c
+++ b/src/shutdown.c
@@ -40,45 +40,38 @@ IceProtocolShutdown (
int majorOpcode
 )
 {
+int i;
+
 if (iceConn->proto_ref_count == 0 || iceConn->process_msg_info == NULL ||
 majorOpcode < 1 || majorOpcode > _IceLastMajorOpcode)
 {
return (0);
 }
-else
-{
-   /*
-* Make sure this majorOpcode is really being used.
-*/
-
-   int i;
+
+
+/*
+ * Make sure this majorOpcode is really being used.
+ */

-   for (i = iceConn->his_min_opcode; i <= iceConn->his_max_opcode; i++)
-   {
-   if (iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].in_use &&
-iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].my_opcode == majorOpcode)
-   break;
-   }
+for (i = iceConn->his_min_opcode; i <= iceConn->his_max_opcode; i++)
+  {
+   int n=i - iceConn->his_min_opcode;
+   if (iceConn->process_msg_info[n].in_use &&
+   iceConn->process_msg_info[n].my_opcode == majorOpcode)
+ {

-   if (i > iceConn->his_max_opcode)
-   {
-   return (0);
-   }
-   else
-   {
/*
 * OK, we can shut down the protocol.
 */

-   iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].in_use = False;
-   iceConn->proto_ref_count--;
+ iceConn->process_msg_info[n].in_use = False;
+ iceConn->proto_ref_count--;
+ return (1);
+ }
+   
+  }

-   return (1);
-   }
-}
+return (0);
 }


-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 04/13] check the return of malloc()

[walter harms]

check the return of malloc()

Signed-off-by: Walter Harms <wha...@bfs.de>
---
 src/protosetup.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/protosetup.c b/src/protosetup.c
index b6aece8..dbc136e 100644
--- a/src/protosetup.c
+++ b/src/protosetup.c
@@ -110,11 +110,17 @@ IceProtocolSetup (
 /*
  * Generate the message.
  */
+authCount = 0;
+authIndices = NULL;

 if (myProtocol->orig_client->auth_count > 0)
 {
authIndices = malloc (
myProtocol->orig_client->auth_count * sizeof (int));
+   if (! authIndices) {
+ strncpy (errorStringRet,"out of memory",errorLength);
+ return (IceProtocolSetupFailure);
+   }

_IceGetPoValidAuthIndices (myProtocol->protocol_name,
iceConn->connection_string,
@@ -123,11 +129,6 @@ IceProtocolSetup (
 , authIndices);

 }
-else
-{
-   authCount = 0;
-   authIndices = NULL;
-}

 extra = STRING_BYTES (myProtocol->protocol_name) +
 STRING_BYTES (myProtocol->orig_client->vendor) +
@@ -183,6 +184,12 @@ IceProtocolSetup (
 replyWait.reply = (IcePointer) 

 iceConn->protosetup_to_you = malloc (sizeof (_IceProtoSetupToYouInfo));
+if (! iceConn->protosetup_to_you) {
+  free(authIndices);
+  strncpy (errorStringRet,"out of memory",errorLength);
+  return (IceProtocolSetupFailure);
+}
+
 iceConn->protosetup_to_you->my_opcode = myOpcode;
 iceConn->protosetup_to_you->my_auth_count = authCount;
 iceConn->protosetup_to_you->auth_active = 0;
@@ -199,6 +206,8 @@ IceProtocolSetup (

if (ioErrorOccured)
{
+  free(iceConn->protosetup_to_you);
+  free(authIndices);
strncpy (errorStringRet,
"IO error occured doing Protocol Setup on connection",
errorLength);
@@ -240,6 +249,7 @@ IceProtocolSetup (
free (iceConn->protosetup_to_you->my_auth_indices);
free (iceConn->protosetup_to_you);
iceConn->protosetup_to_you = NULL;
+   free(authIndices);
}
 }

@@ -279,6 +289,5 @@ IceProtocolSetup (
   versionRec->process_msg_proc;

 return (IceProtocolSetupSuccess);
-

 }
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 03/13] save one indentlevel in IceProtocolSetup

[walter harms]

save one indentlevel in IceProtocolSetup by early check and
remove a lost free() check
Signed-off-by: Walter Harms <wha...@bfs.de>

---
 src/protosetup.c | 61 +++-
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/src/protosetup.c b/src/protosetup.c
index fc6010a..b6aece8 100644
--- a/src/protosetup.c
+++ b/src/protosetup.c
@@ -60,6 +60,7 @@ IceProtocolSetup (
 IcePoVersionRec*versionRec = NULL;
 intauthCount;
 int*authIndices;
+_IceProcessMsgInfo  *process_msg_info;

 if (errorStringRet && errorLength > 0)
*errorStringRet = '\0';
@@ -235,53 +236,49 @@ IceProtocolSetup (
free (reply.protocol_error.error_message);
}

-   if (iceConn->protosetup_to_you->my_auth_indices)
-   free (iceConn->protosetup_to_you->my_auth_indices);
+
+   free (iceConn->protosetup_to_you->my_auth_indices);
free (iceConn->protosetup_to_you);
iceConn->protosetup_to_you = NULL;
}
 }

-if (accepted)
-{
-   _IceProcessMsgInfo *process_msg_info;
+if (!accepted)
+   return (IceProtocolSetupFailure);

-   *majorVersionRet = versionRec->major_version;
-   *minorVersionRet = versionRec->minor_version;
-   *vendorRet = reply.protocol_reply.vendor;
-   *releaseRet = reply.protocol_reply.release;
+*majorVersionRet = versionRec->major_version;
+*minorVersionRet = versionRec->minor_version;
+*vendorRet = reply.protocol_reply.vendor;
+*releaseRet = reply.protocol_reply.release;


-   /*
-* Increase the reference count for the number of active protocols.
-*/
+/*
+ * Increase the reference count for the number of active protocols.
+ */

-   iceConn->proto_ref_count++;
+iceConn->proto_ref_count++;


-   /*
-* We may be using a different major opcode for this protocol
-* than the other client.  Whenever we get a message, we must
-* map to our own major opcode.
-*/
+/*
+ * We may be using a different major opcode for this protocol
+ * than the other client.  Whenever we get a message, we must
+ * map to our own major opcode.
+ */

-   hisOpcode = reply.protocol_reply.major_opcode;
+hisOpcode = reply.protocol_reply.major_opcode;

-   _IceAddOpcodeMapping (iceConn, hisOpcode, myOpcode);
+_IceAddOpcodeMapping (iceConn, hisOpcode, myOpcode);

-   process_msg_info = >process_msg_info[hisOpcode -
-   iceConn->his_min_opcode];
+process_msg_info = >process_msg_info[hisOpcode -
+ iceConn->his_min_opcode];

-   process_msg_info->client_data = clientData;
-   process_msg_info->accept_flag = 0;
+process_msg_info->client_data = clientData;
+process_msg_info->accept_flag = 0;

-   process_msg_info->process_msg_proc.orig_client =
-   versionRec->process_msg_proc;
+process_msg_info->process_msg_proc.orig_client =
+  versionRec->process_msg_proc;
+
+return (IceProtocolSetupSuccess);
+

-   return (IceProtocolSetupSuccess);
-}
-else
-{
-   return (IceProtocolSetupFailure);
-}
 }
-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 00/13] clean up patches for LibICE

[walter harms]

Hi List,
this is a bunch of chean up patches for the
Inter Client Exchange library for X11.

there are no functional changes just clean up
like malloc checks, release of memory in error case etc.

re,
 wh

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 01/13] free() can handle NULL so remove the check

[walter harms]

From f6d630eda04e02713da059519f56dd2dd75a703c Mon Sep 17 00:00:00 2001


free() can handle NULL so remove the check

---
 src/authutil.c | 24 
 src/misc.c |  3 +--
 src/process.c  | 41 +++--
 src/shutdown.c | 44 +++-
 4 files changed, 39 insertions(+), 73 deletions(-)

diff --git a/src/authutil.c b/src/authutil.c
index ca0504a..d7bcef9 100644
--- a/src/authutil.c
+++ b/src/authutil.c
@@ -111,8 +111,8 @@ IceAuthFileName (void)

 if (size > bsize)
 {
-   if (buf)
-   free (buf);
+
+   free (buf);
buf = malloc (size);
if (!buf) {
bsize = 0;
@@ -266,11 +266,11 @@ IceReadAuthFileEntry (

  bad:

-if (local.protocol_name) free (local.protocol_name);
-if (local.protocol_data) free (local.protocol_data);
-if (local.network_id) free (local.network_id);
-if (local.auth_name) free (local.auth_name);
-if (local.auth_data) free (local.auth_data);
+free (local.protocol_name);
+free (local.protocol_data);
+free (local.network_id);
+free (local.auth_name);
+free (local.auth_data);

 return (NULL);
 }
@@ -284,11 +284,11 @@ IceFreeAuthFileEntry (
 {
 if (auth)
 {
-   if (auth->protocol_name) free (auth->protocol_name);
-   if (auth->protocol_data) free (auth->protocol_data);
-   if (auth->network_id) free (auth->network_id);
-   if (auth->auth_name) free (auth->auth_name);
-   if (auth->auth_data) free (auth->auth_data);
+   free (auth->protocol_name);
+   free (auth->protocol_data);
+   free (auth->network_id);
+   free (auth->auth_name);
+   free (auth->auth_data);
free (auth);
 }
 }
diff --git a/src/misc.c b/src/misc.c
index d2e9150..87d6335 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -54,8 +54,7 @@ IceAllocScratch (
 {
 if (!iceConn->scratch || size > iceConn->scratch_size)
 {
-   if (iceConn->scratch)
-   free (iceConn->scratch);
+   free (iceConn->scratch);

iceConn->scratch = malloc (size);
iceConn->scratch_size = size;
diff --git a/src/process.c b/src/process.c
index 4100a83..a9a8d08 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1026,8 +1026,7 @@ ProcessConnectionSetup (
iceConn->connection_status = IceConnectRejected;
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}

if (iceConn->connection_status == IceConnectRejected)
@@ -1080,8 +1079,7 @@ ProcessConnectionSetup (
if (authData && authDataLen > 0)
free (authData);

-   if (errorString)
-   free (errorString);
+   free (errorString);
 }

 if (accept_setup_now)
@@ -1369,8 +1367,7 @@ ProcessAuthReply (
status = IcePaAuthAccepted;
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}

if (status != IcePaAuthAccepted)
@@ -1444,8 +1441,7 @@ ProcessAuthReply (
status = IcePaAuthAccepted;
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}

if (status == IcePaAuthRejected)
@@ -1559,18 +1555,15 @@ ProcessAuthReply (
_IceErrorSetupFailed (iceConn, ICE_ProtocolSetup,
failureReason);

-   if (failureReason)
-   free (failureReason);
+   free (failureReason);
}
}


if (free_setup_info)
{
-   if (iceConn->protosetup_to_me->his_vendor)
-   free (iceConn->protosetup_to_me->his_vendor);
-   if (iceConn->protosetup_to_me->his_release)
-   free (iceConn->protosetup_to_me->his_release);
+   free (iceConn->protosetup_to_me->his_vendor);
+   free (iceConn->protosetup_to_me->his_release);
free (iceConn->protosetup_to_me);
iceConn->protosetup_to_me = NULL;
}
@@ -1587,8 +1580,8 @@ ProcessAuthReply (
 if (authData && authDataLen > 0)
free (authData);

-if (errorString)
-   free (errorString);
+
+free (errorString);

 IceDisposeCompleteMessage (iceConn, replyData);
 return (0);
@@ -2071,8 +2064,7 @@ ProcessProtocolSetup (
ICE_ProtocolSetup, "None of the authentication protocols 
specified are supported and host-based authentication failed");
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}
 }
 else
@@ -2118,8 +2110,8 @@ ProcessProtocolSetup (
if (authData && authDataLen > 0)
free (authData);

-   if (errorString)
-   free (errorString);
+
+   free (errorString);
 }

 if (accept_setup_now)
@@ -2202,16 +2194,13 @@ ProcessProtocolSetup (

_IceErrorSetupFailed (iceConn, 

Re: [PATCH libICE 1/2] free() can handle NULL so remove the check

[walter harms]

Am 12.09.2017 10:58, schrieb Eric Engestrom:
> On Friday, 2017-09-08 19:59:17 +0200, walter harms wrote:
>> free() can handle NULL so remove the check
> 
> Did you use a cocci script [1] to generate this?
> If so, can you add it to the commit message?
> 

no, i used smatch, to be fair i did not mention it.

re,
 wh


> Regardless, I double-checked it and it looks good to me:
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
> 
> [1] perhaps something like this?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/ifnullfree.cocci
> 
>>
>> Signed-off-by: Walter Harms <wha...@bfs.de>
>> ---
>>  src/authutil.c | 24 
>>  src/misc.c |  3 +--
>>  src/process.c  | 41 +++--
>>  src/shutdown.c | 44 +++-
>>  4 files changed, 39 insertions(+), 73 deletions(-)
>>
>> diff --git a/src/authutil.c b/src/authutil.c
>> index ca0504a..d7bcef9 100644
>> --- a/src/authutil.c
>> +++ b/src/authutil.c
>> @@ -111,8 +111,8 @@ IceAuthFileName (void)
>>
>>  if (size > bsize)
>>  {
>> -if (buf)
>> -free (buf);
>> +
>> +free (buf);
>>  buf = malloc (size);
>>  if (!buf) {
>>  bsize = 0;
>> @@ -266,11 +266,11 @@ IceReadAuthFileEntry (
>>
>>   bad:
>>
>> -if (local.protocol_name) free (local.protocol_name);
>> -if (local.protocol_data) free (local.protocol_data);
>> -if (local.network_id) free (local.network_id);
>> -if (local.auth_name) free (local.auth_name);
>> -if (local.auth_data) free (local.auth_data);
>> +free (local.protocol_name);
>> +free (local.protocol_data);
>> +free (local.network_id);
>> +free (local.auth_name);
>> +free (local.auth_data);
>>
>>  return (NULL);
>>  }
>> @@ -284,11 +284,11 @@ IceFreeAuthFileEntry (
>>  {
>>  if (auth)
>>  {
>> -if (auth->protocol_name) free (auth->protocol_name);
>> -if (auth->protocol_data) free (auth->protocol_data);
>> -if (auth->network_id) free (auth->network_id);
>> -if (auth->auth_name) free (auth->auth_name);
>> -if (auth->auth_data) free (auth->auth_data);
>> +free (auth->protocol_name);
>> +free (auth->protocol_data);
>> +free (auth->network_id);
>> +free (auth->auth_name);
>> +free (auth->auth_data);
>>  free (auth);
>>  }
>>  }
>> diff --git a/src/misc.c b/src/misc.c
>> index d2e9150..87d6335 100644
>> --- a/src/misc.c
>> +++ b/src/misc.c
>> @@ -54,8 +54,7 @@ IceAllocScratch (
>>  {
>>  if (!iceConn->scratch || size > iceConn->scratch_size)
>>  {
>> -if (iceConn->scratch)
>> -free (iceConn->scratch);
>> +free (iceConn->scratch);
>>
>>  iceConn->scratch = malloc (size);
>>  iceConn->scratch_size = size;
>> diff --git a/src/process.c b/src/process.c
>> index 4100a83..a9a8d08 100644
>> --- a/src/process.c
>> +++ b/src/process.c
>> @@ -1026,8 +1026,7 @@ ProcessConnectionSetup (
>>  iceConn->connection_status = IceConnectRejected;
>>  }
>>
>> -if (hostname)
>> -free (hostname);
>> +free (hostname);
>>  }
>>
>>  if (iceConn->connection_status == IceConnectRejected)
>> @@ -1080,8 +1079,7 @@ ProcessConnectionSetup (
>>  if (authData && authDataLen > 0)
>>  free (authData);
>>
>> -if (errorString)
>> -free (errorString);
>> +free (errorString);
>>  }
>>
>>  if (accept_setup_now)
>> @@ -1369,8 +1367,7 @@ ProcessAuthReply (
>>  status = IcePaAuthAccepted;
>>  }
>>
>> -if (hostname)
>> -free (hostname);
>> +free (hostname);
>>  }
>>
>>  if (status != IcePaAuthAccepted)
>> @@ -1444,8 +1441,7 @@ ProcessAuthReply (
>>  status = IcePaAuthAccepted;
>>  }
>>
>> -if (hostname)
>> -free (hostname);
>> +free (hostname);
>>  }
>>
>>  if (status == IcePaAuthRejected)
>> @@ -1559,18 +1555,15 @@ ProcessAuthReply (
>>  _IceErrorSetupFailed (iceConn, ICE_Protocol

Re: [PATCH libX11 1/3] Make _XCloseLC thread safe.

[walter harms]

Hi Jacek,
while it worked in the latest release of libX11 i was unable to
replicate the crash with the current git version.
Can you confirm my observation ?

re,
 wh


Am 14.08.2017 19:27, schrieb Jacek Caban:
> Hi,
> 
> Sure, a simple test case is attached. It replicates what Wine does when
> initializing new threads (see [1] for Wine reports).
> 
> Thanks,
> Jacek
> 
> [1] https://bugs.winehq.org/show_bug.cgi?id=35041
> 
> 
> On 14.08.2017 15:43, walter harms wrote:
>> hi,
>> can you provide a simple program does needs the patch to work ?
>>
>> re,
>>  wh
>>
>> Am 10.08.2017 23:04, schrieb Jacek Caban:
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=55678
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=68538
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=69088
>>> Signed-off-by: Jacek Caban <ja...@codeweavers.com>
>>> ---
>>>  src/xlibi18n/lcWrap.c | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/src/xlibi18n/lcWrap.c b/src/xlibi18n/lcWrap.c
>>> index 38242608..43b4d622 100644
>>> --- a/src/xlibi18n/lcWrap.c
>>> +++ b/src/xlibi18n/lcWrap.c
>>> @@ -324,6 +324,8 @@ _XCloseLC(
>>>  {
>>>  XLCdList cur, *prev;
>>>  +_XLockMutex(_Xi18n_lock);
>>> +
>>>  for (prev = _list; (cur = *prev); prev = >next) {
>>> if (cur->lcd == lcd) {
>>> if (--cur->ref_count < 1) {
>>> @@ -339,6 +341,8 @@ _XCloseLC(
>>> _XlcDeInitLoader();
>>> loader_list = NULL;
>>>  }
>>> +
>>> +_XUnlockMutex(_Xi18n_lock);
>>>  }
>>>   /*
> 
> 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11 1/3] Make _XCloseLC thread safe.

[walter harms]

Am 14.08.2017 19:27, schrieb Jacek Caban:
> Hi,
> 
> Sure, a simple test case is attached. It replicates what Wine does when
> initializing new threads (see [1] for Wine reports).
> 
> Thanks,
> Jacek
> 
> [1] https://bugs.winehq.org/show_bug.cgi?id=35041
> 

i was able to get a double free bug. Is that what is expected ?
(the bugreport shows several backtraces, i want to make sure).

re,
 wh

> 
> On 14.08.2017 15:43, walter harms wrote:
>> hi,
>> can you provide a simple program does needs the patch to work ?
>>
>> re,
>>  wh
>>
>> Am 10.08.2017 23:04, schrieb Jacek Caban:
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=55678
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=68538
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=69088
>>> Signed-off-by: Jacek Caban <ja...@codeweavers.com>
>>> ---
>>>  src/xlibi18n/lcWrap.c | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/src/xlibi18n/lcWrap.c b/src/xlibi18n/lcWrap.c
>>> index 38242608..43b4d622 100644
>>> --- a/src/xlibi18n/lcWrap.c
>>> +++ b/src/xlibi18n/lcWrap.c
>>> @@ -324,6 +324,8 @@ _XCloseLC(
>>>  {
>>>  XLCdList cur, *prev;
>>>  +_XLockMutex(_Xi18n_lock);
>>> +
>>>  for (prev = _list; (cur = *prev); prev = >next) {
>>> if (cur->lcd == lcd) {
>>> if (--cur->ref_count < 1) {
>>> @@ -339,6 +341,8 @@ _XCloseLC(
>>> _XlcDeInitLoader();
>>> loader_list = NULL;
>>>  }
>>> +
>>> +_XUnlockMutex(_Xi18n_lock);
>>>  }
>>>   /*
> 
> 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 2/2] make IceProtocolShutdown() more readable

[walter harms]

I found IceProtocolShutdown() hard to read only to find that was
it does it aktually very simple. So i rearranged the code to make
it more readable.

Signed-off-by: Walter Harms <wha...@bfs.de>
---
 src/shutdown.c | 47 ---
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/src/shutdown.c b/src/shutdown.c
index 90e9ded..98376a7 100644
--- a/src/shutdown.c
+++ b/src/shutdown.c
@@ -40,45 +40,38 @@ IceProtocolShutdown (
int majorOpcode
 )
 {
+int i;
+
 if (iceConn->proto_ref_count == 0 || iceConn->process_msg_info == NULL ||
 majorOpcode < 1 || majorOpcode > _IceLastMajorOpcode)
 {
return (0);
 }
-else
-{
-   /*
-* Make sure this majorOpcode is really being used.
-*/
-
-   int i;
+
+
+/*
+ * Make sure this majorOpcode is really being used.
+ */

-   for (i = iceConn->his_min_opcode; i <= iceConn->his_max_opcode; i++)
-   {
-   if (iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].in_use &&
-iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].my_opcode == majorOpcode)
-   break;
-   }
+for (i = iceConn->his_min_opcode; i <= iceConn->his_max_opcode; i++)
+  {
+   int n=i - iceConn->his_min_opcode;
+   if (iceConn->process_msg_info[n].in_use &&
+   iceConn->process_msg_info[n].my_opcode == majorOpcode)
+ {

-   if (i > iceConn->his_max_opcode)
-   {
-   return (0);
-   }
-   else
-   {
/*
 * OK, we can shut down the protocol.
 */

-   iceConn->process_msg_info[
-   i - iceConn->his_min_opcode].in_use = False;
-   iceConn->proto_ref_count--;
+ iceConn->process_msg_info[n].in_use = False;
+ iceConn->proto_ref_count--;
+ return (1);
+ }
+   
+  }

-   return (1);
-   }
-}
+return (0);
 }


-- 
2.1.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH libICE 1/2] free() can handle NULL so remove the check

[walter harms]

free() can handle NULL so remove the check

Signed-off-by: Walter Harms <wha...@bfs.de>
---
 src/authutil.c | 24 
 src/misc.c |  3 +--
 src/process.c  | 41 +++--
 src/shutdown.c | 44 +++-
 4 files changed, 39 insertions(+), 73 deletions(-)

diff --git a/src/authutil.c b/src/authutil.c
index ca0504a..d7bcef9 100644
--- a/src/authutil.c
+++ b/src/authutil.c
@@ -111,8 +111,8 @@ IceAuthFileName (void)

 if (size > bsize)
 {
-   if (buf)
-   free (buf);
+
+   free (buf);
buf = malloc (size);
if (!buf) {
bsize = 0;
@@ -266,11 +266,11 @@ IceReadAuthFileEntry (

  bad:

-if (local.protocol_name) free (local.protocol_name);
-if (local.protocol_data) free (local.protocol_data);
-if (local.network_id) free (local.network_id);
-if (local.auth_name) free (local.auth_name);
-if (local.auth_data) free (local.auth_data);
+free (local.protocol_name);
+free (local.protocol_data);
+free (local.network_id);
+free (local.auth_name);
+free (local.auth_data);

 return (NULL);
 }
@@ -284,11 +284,11 @@ IceFreeAuthFileEntry (
 {
 if (auth)
 {
-   if (auth->protocol_name) free (auth->protocol_name);
-   if (auth->protocol_data) free (auth->protocol_data);
-   if (auth->network_id) free (auth->network_id);
-   if (auth->auth_name) free (auth->auth_name);
-   if (auth->auth_data) free (auth->auth_data);
+   free (auth->protocol_name);
+   free (auth->protocol_data);
+   free (auth->network_id);
+   free (auth->auth_name);
+   free (auth->auth_data);
free (auth);
 }
 }
diff --git a/src/misc.c b/src/misc.c
index d2e9150..87d6335 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -54,8 +54,7 @@ IceAllocScratch (
 {
 if (!iceConn->scratch || size > iceConn->scratch_size)
 {
-   if (iceConn->scratch)
-   free (iceConn->scratch);
+   free (iceConn->scratch);

iceConn->scratch = malloc (size);
iceConn->scratch_size = size;
diff --git a/src/process.c b/src/process.c
index 4100a83..a9a8d08 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1026,8 +1026,7 @@ ProcessConnectionSetup (
iceConn->connection_status = IceConnectRejected;
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}

if (iceConn->connection_status == IceConnectRejected)
@@ -1080,8 +1079,7 @@ ProcessConnectionSetup (
if (authData && authDataLen > 0)
free (authData);

-   if (errorString)
-   free (errorString);
+   free (errorString);
 }

 if (accept_setup_now)
@@ -1369,8 +1367,7 @@ ProcessAuthReply (
status = IcePaAuthAccepted;
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}

if (status != IcePaAuthAccepted)
@@ -1444,8 +1441,7 @@ ProcessAuthReply (
status = IcePaAuthAccepted;
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}

if (status == IcePaAuthRejected)
@@ -1559,18 +1555,15 @@ ProcessAuthReply (
_IceErrorSetupFailed (iceConn, ICE_ProtocolSetup,
failureReason);

-   if (failureReason)
-   free (failureReason);
+   free (failureReason);
}
}


if (free_setup_info)
{
-   if (iceConn->protosetup_to_me->his_vendor)
-   free (iceConn->protosetup_to_me->his_vendor);
-   if (iceConn->protosetup_to_me->his_release)
-   free (iceConn->protosetup_to_me->his_release);
+   free (iceConn->protosetup_to_me->his_vendor);
+   free (iceConn->protosetup_to_me->his_release);
free (iceConn->protosetup_to_me);
iceConn->protosetup_to_me = NULL;
}
@@ -1587,8 +1580,8 @@ ProcessAuthReply (
 if (authData && authDataLen > 0)
free (authData);

-if (errorString)
-   free (errorString);
+
+free (errorString);

 IceDisposeCompleteMessage (iceConn, replyData);
 return (0);
@@ -2071,8 +2064,7 @@ ProcessProtocolSetup (
ICE_ProtocolSetup, "None of the authentication protocols 
specified are supported and host-based authentication failed");
}

-   if (hostname)
-   free (hostname);
+   free (hostname);
}
 }
 else
@@ -2118,8 +2110,8 @@ ProcessProtocolSetup (
if (authData && authDataLen > 0)
free (authData);

-   if (errorString)
-   free (errorString);
+
+ 

Re: [PATCHv2 5/8] edid-decode: add new CTA-861-G VIC codes

[walter harms]

Am 08.09.2017 12:32, schrieb Hans Verkuil:
> From: Hans Verkuil 
> 
> The CTA-861-G standard (successor to CEA-861-F) adds new VIC codes.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  edid-decode.c | 93 
> +++
>  1 file changed, 81 insertions(+), 12 deletions(-)
> 
> diff --git a/edid-decode.c b/edid-decode.c
> index 4534d58f..074cb821 100644
> --- a/edid-decode.c
> +++ b/edid-decode.c
> @@ -1113,10 +1113,12 @@ cea_audio_block(unsigned char *x)
>  }
>  }
>  
> -static struct {
> +struct edid_cea_mode {
>  const char *name;
>  int refresh, hor_freq_hz, pixclk_khz;
> -} edid_cea_modes[] = {
> +};
> +
> +static struct edid_cea_mode edid_cea_modes1[] = {
>  /* VIC 1 */
>  {"640x480@60Hz 4:3", 60, 31469, 25175},
>  {"720x480@60Hz 4:3", 60, 31469, 27000},
> @@ -1235,14 +1237,80 @@ static struct {
>  {"3840x2160@30Hz 64:27", 30, 67500, 297000},
>  {"3840x2160@50Hz 64:27", 50, 112500, 594000},
>  {"3840x2160@60Hz 64:27", 60, 135000, 594000},
> +{"1280x720@48Hz 16:9", 48, 36000, 9},
> +{"1280x720@48Hz 64:27", 48, 36000, 9},
> +{"1680x720@48Hz 64:27", 48, 36000, 99000},
> +/* VIC 111 */
> +{"1920x1080@48Hz 16:9", 48, 54000, 148500},
> +{"1920x1080@48Hz 64:27", 48, 54000, 148500},
> +{"2560x1080@48Hz 64:27", 48, 52800, 198000},
> +{"3840x2160@48Hz 16:9", 48, 108000, 594000},
> +{"4096x2160@48Hz 256:135", 48, 108000, 594000},
> +{"3840x2160@48Hz 64:27", 48, 108000, 594000},
> +{"3840x2160@100Hz 16:9", 100, 225000, 1188000},
> +{"3840x2160@120Hz 16:9", 120, 27, 1188000},
> +{"3840x2160@100Hz 64:27", 100, 225000, 1188000},
> +{"3840x2160@120Hz 64:27", 120, 27, 1188000},
> +/* VIC 121 */
> +{"5120x2160@24Hz 64:27", 24, 52800, 396000},
> +{"5120x2160@25Hz 64:27", 25, 55000, 396000},
> +{"5120x2160@30Hz 64:27", 30, 66000, 396000},
> +{"5120x2160@48Hz 64:27", 48, 118800, 742500},
> +{"5120x2160@50Hz 64:27", 50, 112500, 742500},
> +{"5120x2160@60Hz 64:27", 60, 135000, 742500},
> +{"5120x2160@100Hz 64:27", 100, 225000, 1485000},
> +};
> +
> +static struct edid_cea_mode edid_cea_modes2[] = {
> +/* VIC 193 */
> +{"5120x2160@120Hz 64:27", 120, 27, 1485000},
> +{"7680x4320@24Hz 16:9", 24, 108000, 1188000},
> +{"7680x4320@25Hz 16:9", 25, 11, 1188000},
> +{"7680x4320@30Hz 16:9", 30, 132000, 1188000},
> +{"7680x4320@48Hz 16:9", 48, 216000, 2376000},
> +{"7680x4320@50Hz 16:9", 50, 22, 2376000},
> +{"7680x4320@60Hz 16:9", 60, 264000, 2376000},
> +{"7680x4320@100Hz 16:9", 100, 45, 4752000},
> +/* VIC 201 */
> +{"7680x4320@120Hz 16:9", 120, 54, 4752000},
> +{"7680x4320@24Hz 64:27", 24, 108000, 1188000},
> +{"7680x4320@25Hz 64:27", 25, 11, 1188000},
> +{"7680x4320@30Hz 64:27", 30, 132000, 1188000},
> +{"7680x4320@48Hz 64:27", 48, 216000, 2376000},
> +{"7680x4320@50Hz 64:27", 50, 22, 2376000},
> +{"7680x4320@60Hz 64:27", 60, 264000, 2376000},
> +{"7680x4320@100Hz 64:27", 100, 45, 4752000},
> +{"7680x4320@120Hz 64:27", 120, 54, 4752000},
> +{"10240x4320@24Hz 64:27", 24, 118800, 1485000},
> +/* VIC 211 */
> +{"10240x4320@25Hz 64:27", 25, 11, 1485000},
> +{"10240x4320@30Hz 64:27", 30, 135000, 1485000},
> +{"10240x4320@48Hz 64:27", 48, 237600, 297},
> +{"10240x4320@50Hz 64:27", 50, 22, 297},
> +{"10240x4320@60Hz 64:27", 60, 27, 297},
> +{"10240x4320@100Hz 64:27", 100, 45, 594},
> +{"10240x4320@120Hz 64:27", 120, 54, 594},
> +{"4096x2160@100Hz 256:135", 100, 225000, 1188000},
> +{"4096x2160@120Hz 256:135", 120, 27, 1188000},
>  };
>  
> +static const struct edid_cea_mode *
> +vic_to_mode(unsigned char vic)
> +{
> +if (vic > 0 && vic <= ARRAY_SIZE(edid_cea_modes1))
> + return edid_cea_modes1 + vic - 1;
> +if (vic >= 193 && vic <= ARRAY_SIZE(edid_cea_modes2) + 193)
> + return edid_cea_modes2 + vic - 193;
> +return NULL;
> +}
> +


I do not see details yet, but maybe you can simply the code by
returning a dummy struct edid_cea_mode [0] =
 {"Unknown mode 0:0", 0, 0, 0}


>  static void
>  cea_svd(unsigned char *x, int n, int for_ycbcr420)
>  {
>  int i;
>  
>  for (i = 0; i < n; i++)  {
> + const struct edid_cea_mode *vicmode = NULL;
>   unsigned char svd = x[i];
>   unsigned char native;
>   unsigned char vic;
> @@ -1261,7 +1329,8 @@ cea_svd(unsigned char *x, int n, int for_ycbcr420)
>   native = svd & 0x80;
>   }
>  
> - if (vic > 0 && vic <= ARRAY_SIZE(edid_cea_modes)) {
> + vicmode = vic_to_mode(vic);
> + if (vicmode) {
>   switch (vic) {
>   case 95:
>   supported_hdmi_vic_vsb_codes |= 1 << 0;
> @@ -1276,13 +1345,13 @@ cea_svd(unsigned char *x, int n, int for_ycbcr420)
>   

Re: [PATCHv2 4/8] edid-decode: add support for Room/Speaker data blocks

[walter harms]

Am 08.09.2017 12:32, schrieb Hans Verkuil:
> From: Hans Verkuil 
> 
> Support the Room Configuration Data Block and the Speaker Location
> Data Block.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  edid-decode.c | 112 
> --
>  1 file changed, 109 insertions(+), 3 deletions(-)
> 
> diff --git a/edid-decode.c b/edid-decode.c
> index 8d47a5fc..4534d58f 100644
> --- a/edid-decode.c
> +++ b/edid-decode.c
> @@ -1614,7 +1614,7 @@ static struct field *vcdb_fields[] = {
>  _scan,
>  };
>  
> -static const char *sadb_map[] = {
> +static const char *speaker_map[] = {
>  "FL/FR - Front Left/Right",
>  "LFE - Low Frequency Effects",
>  "FC - Front Center",
> @@ -1648,13 +1648,111 @@ cea_sadb(unsigned char *x)
>  
>   printf("Speaker map:\n");
>  
> - for (i = 0; i < ARRAY_SIZE(sadb_map); i++) {
> + for (i = 0; i < ARRAY_SIZE(speaker_map); i++) {
>   if ((sad >> i) & 1)
> - printf("  %s\n", sadb_map[i]);
> + printf("  %s\n", speaker_map[i]);
>   }
>  }
>  }
>  
> +static float
> +decode_uchar_as_float(unsigned char x)
> +{
> +signed char s = (signed char)x;
> +
> +return s / 64.0;
> +}
> +
> +static void
> +cea_rcdb(unsigned char *x)
> +{
> +int length = x[0] & 0x1f;
> +uint32_t spm = ((x[5] << 16) | (x[4] << 8) | x[3]);
> +int i;
> +
> +if (length < 4)
> + return;
> +
> +if (x[2] & 0x40)
> + printf("Speaker count: %d\n", (x[2] & 0x1f) + 1);
> +
> +printf("Speaker Presence Mask:\n");
> +for (i = 0; i < ARRAY_SIZE(speaker_map); i++) {
> + if ((spm >> i) & 1)
> + printf("  %s\n", speaker_map[i]);
> +}
> +if ((x[2] & 0x20) && length >= 7) {
> + printf("Xmax: %d dm\n", x[6]);
> + printf("Ymax: %d dm\n", x[7]);
> + printf("Zmax: %d dm\n", x[8]);

length >= 7)  ... x[8]);
off by one ?

> +}
> +if ((x[2] & 0x80) && length >= 10) {
> + printf("DisplayX: %.3f * Xmax\n", decode_uchar_as_float(x[9]));
> + printf("DisplayY: %.3f * Ymax\n", decode_uchar_as_float(x[10]));
> + printf("DisplayZ: %.3f * Zmax\n", decode_uchar_as_float(x[11]));

off by one ?

> +}
> +}
> +
> +static const char *speaker_location[] = {
> +"FL - Front Left",
> +"FR - Front Right",
> +"FC - Front Center",
> +"LFE1 - Low Frequency Effects 1",
> +"BL - Back Left",
> +"BR - Back Right",
> +"FLC - Front Left of Center",
> +"FRC - Front Right of Center",
> +"BC - Back Center",
> +"LFE2 - Low Frequency Effects 2",
> +"SiL - Side Left",
> +"SiR - Side Right",
> +"TpFL - Top Front Left",
> +"TpFH - Top Front Right",
> +"TpFC - Top Front Center",
> +"TpC - Top Center",
> +"TpBL - Top Back Left",
> +"TpBR - Top Back Right",
> +"TpSiL - Top Side Left",
> +"TpSiR - Top Side Right",
> +"TpBC - Top Back Center",
> +"BtFC - Bottom Front Center",
> +"BtFL - Bottom Front Left",
> +"BtBR - Bottom Front Right",
> +"FLW - Front Left Wide",
> +"FRW - Front Right Wide",
> +"LS - Left Surround",
> +"RS - Right Surround",
> +};
> +
> +static void
> +cea_sldb(unsigned char *x)
> +{
> +int length = x[0] & 0x1f;
> +
> +if (!length)
> + return;
> +
> +x += 2;
> +length--;
> +
> +while (length >= 2) {
> + printf("Channel: %d (%sactive)\n", x[0] & 0x1f,
> +(x[0] & 0x20) ? "" : "not ");
> + if ((x[1] & 0x1f) < ARRAY_SIZE(speaker_location))
> + printf("  Speaker: %s\n", speaker_location[x[1] & 0x1f]);
> + if (length >= 5 && (x[0] & 0x40)) {
> + printf("  X: %.3f * Xmax\n", decode_uchar_as_float(x[2]));
> + printf("  Y: %.3f * Ymax\n", decode_uchar_as_float(x[3]));
> + printf("  Z: %.3f * Zmax\n", decode_uchar_as_float(x[4]));
> + length -= 3;
> + x += 3;
> + }
> +
> + length -= 2;
> + x += 2;
> +}
> +}
> +
>  static void
>  cea_vcdb(unsigned char *x)
>  {
> @@ -1812,6 +1910,14 @@ cea_block(unsigned char *x)
>   case 0x12:
>   printf("HDMI audio data block\n");
>   break;
> + case 0x13:
> + printf("Room configuration data block\n");
> + cea_rcdb(x);
> + break;
> + case 0x14:
> + printf("Speaker location data block\n");
> + cea_sldb(x);
> + break;
>   case 0x20:
>   printf("InfoFrame data block\n");
>   break;
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 2/8] edid-decode: update Speaker Allocation data block

[walter harms]

Am 07.09.2017 20:03, schrieb Hans Verkuil:
> From: Hans Verkuil 
> 
> More bits are now in use, implement support for those.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  edid-decode.c | 38 ++
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/edid-decode.c b/edid-decode.c
> index 26550423..9b6c297e 100644
> --- a/edid-decode.c
> +++ b/edid-decode.c
> @@ -1615,17 +1615,25 @@ static struct field *vcdb_fields[] = {
>  };
>  
>  static const char *sadb_map[] = {
> -"FL/FR",
> -"LFE",
> -"FC",
> -"RL/RR",
> -"RC",
> -"FLC/FRC",
> -"RLC/RRC",
> -"FLW/FRW",
> -"FLH/FRH",
> -"TC",
> -"FCH",
> +"FL/FR - Front Left/Right",
> +"LFE - Low Frequency Effects",
> +"FC - Front Center",
> +"BL/BR - Back Left/Right",
> +"BC - Back Center",
> +"FLC/FRC - Front Left/Right of Center",
> +"RLC/RRC - Rear Left/Right of Center",
> +"FLW/FRW - Front Left/Right Wide",
> +"TpFL/TpFH - Top Front Left/Right",
> +"TpC - Top Center",
> +"LS/RS - Left/Right Surround",
> +"LFE2 - Low Frequency Effects 2",
> +"TpBC - Top Back Center",
> +"SiL/SiR - Side Left/Right",
> +"TpSiL/TpSiR - Top Side Left/Right",
> +"TpBL/TpBR - Top Back Left/Right",
> +"BtFC - Bottom Front Center",
> +"BtFL/BtBR - Bottom Front Left/Right",
> +"TpLS/TpRS - Top Left/Right Surround",
>  };
>  
>  static void
> @@ -1635,16 +1643,14 @@ cea_sadb(unsigned char *x)
>  int i;
>  
>  if (length >= 3) {
> - uint16_t sad = ((x[2] << 8) | x[1]);
> + uint32_t sad = ((x[3] << 16) | (x[2] << 8) | x[1]);
>  
> - printf("Speaker map:");
> + printf("Speaker map:\n");
>  
>   for (i = 0; i < ARRAY_SIZE(sadb_map); i++) {
>   if ((sad >> i) & 1)
> - printf(" %s", sadb_map[i]);
> + printf("  %s\n", sadb_map[i]);
>   }

just a remark:
 if (sad & 1)

 sad >>=1 ;
 if (sad == 0) break;

to avoid testing empty bits ..

re,
 wh

> -
> - printf("\n");
>  }
>  }
>  
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 3/8] edid-decode: add support for Room/Speaker data blocks

[walter harms]

Am 07.09.2017 20:03, schrieb Hans Verkuil:
> From: Hans Verkuil 
> 
> Support the Room Configuration Data Block and the Speaker Location
> Data Block.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  edid-decode.c | 79 
> ---
>  1 file changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/edid-decode.c b/edid-decode.c
> index 9b6c297e..bdeb0c49 100644
> --- a/edid-decode.c
> +++ b/edid-decode.c
> @@ -1614,7 +1614,7 @@ static struct field *vcdb_fields[] = {
>  _scan,
>  };
>  
> -static const char *sadb_map[] = {
> +static const char *speaker_map[] = {
>  "FL/FR - Front Left/Right",
>  "LFE - Low Frequency Effects",
>  "FC - Front Center",
> @@ -1647,13 +1647,78 @@ cea_sadb(unsigned char *x)
>  
>   printf("Speaker map:\n");
>  
> - for (i = 0; i < ARRAY_SIZE(sadb_map); i++) {
> + for (i = 0; i < ARRAY_SIZE(speaker_map); i++) {
>   if ((sad >> i) & 1)
> - printf("  %s\n", sadb_map[i]);
> + printf("  %s\n", speaker_map[i]);
>   }
>  }
>  }
>  
> +static float
> +uchar_to_float(unsigned char x)
> +{
> +signed char s = (signed char)x;
> +
> +return s / 64.0;
> +}


The Name is a bit unfortunate as there is more than a type change
to be fair i have no better name, to_64flt() ?

re,
 wh

> +
> +static void
> +cea_rcdb(unsigned char *x)
> +{
> +int length = x[0] & 0x1f;
> +uint32_t spm = ((x[5] << 16) | (x[4] << 8) | x[3]);
> +int i;
> +
> +if (length < 12)
> + return;
> +
> +if (x[2] & 0x40)
> + printf("Speaker count: %d\n", x[2] & 0x1f);
> +
> +printf("Speaker Presence Mask:\n");
> +for (i = 0; i < ARRAY_SIZE(speaker_map); i++) {
> + if ((spm >> i) & 1)
> + printf("  %s\n", speaker_map[i]);
> +}
> +if (x[2] & 0x20) {
> + printf("Xmax: %d dm\n", x[6]);
> + printf("Ymax: %d dm\n", x[7]);
> + printf("Zmax: %d dm\n", x[8]);
> +}
> +if (x[2] & 0x80) {
> + printf("DisplayX: %.3f * Xmax\n", uchar_to_float(x[9]));
> + printf("DisplayY: %.3f * Ymax\n", uchar_to_float(x[10]));
> + printf("DisplayZ: %.3f * Zmax\n", uchar_to_float(x[11]));
> +}
> +}
> +
> +static void
> +cea_sldb(unsigned char *x)
> +{
> +int length = x[0] & 0x1f;
> +
> +if (length < 2)
> + return;
> +
> +x += 2;
> +length -= 2;
> +
> +while (length >= 2) {
> + printf("Channel: %d (%sactive)\n", x[0] & 0x1f,
> +(x[0] & 0x20) ? "" : "not ");
> + if ((x[1] & 0x1f) < ARRAY_SIZE(speaker_map))
> + printf("Speaker: %s\n", speaker_map[x[1] & 0x1f]);
> + if (length >= 5 && (x[0] & 0x40)) {
> + printf("X: %.3f * Xmax\n", uchar_to_float(x[2]));
> + printf("Y: %.3f * Ymax\n", uchar_to_float(x[3]));
> + printf("Z: %.3f * Zmax\n", uchar_to_float(x[4]));
> + length -= 3;
> + }
> +
> + length -= 2;
> +}
> +}
> +
>  static void
>  cea_vcdb(unsigned char *x)
>  {
> @@ -1811,6 +1876,14 @@ cea_block(unsigned char *x)
>   case 0x12:
>   printf("HDMI audio data block\n");
>   break;
> + case 0x13:
> + printf("Room configuration data block\n");
> + cea_rcdb(x);
> + break;
> + case 0x14:
> + printf("Speaker location data block\n");
> + cea_sldb(x);
> + break;
>   case 0x20:
>   printf("InfoFrame data block\n");
>   break;
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel