Re: [PATCH weston] Revert "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

On Wed, Jul 13, 2016 at 02:27:17PM -0700, Yong Bakos wrote:
> On Jul 13, 2016, at 2:10 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > On Wed, Jul 13, 2016 at 01:27:29PM -0700, Bryce Harrington wrote:
> >> The reduction in range limits does have an effect for color values,
> >> which are expressed as hexadecimal values from 0x to
> >> 0x.  By limiting the range to INT_MAX, color values of
> >> 0x8000 and up are in fact lost.
> >> 
> >> This reverts commit 6351fb08c2e302f8696b2022830e5317e7219c39.
> >> 
> >> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > 
> > Here are some alternative solutions:
> > 
> > a.  Use strtoll instead of strtol
> > 
> > b.  Add weston_config_section_get_hex()
> > 
> > c.  Add weston_config_section_get_color(), that supports both
> >hexadecimal and color names (ala rgb.txt or similar)
> > 
> > I'm leaning towards (b), and sounds like Peter was thinking along those
> > lines.  Splitting hexadecimal handling out of get_uint() would also
> > allow specifying base-10, and would allow stronger type checking
> > generally.
> > 
> > We could also do both (a) and (b).  But I don't know whether we actually
> > need the full range for unsigned ints other than for hexadecimal values.
> > I also don't know if there's any difference in overhead between strtoll
> > vs. strtol.
> > 
> > Option (c) I think would be spiffy but I'm skeptical that using color
> > names would really be all that useful in practice, and it would require
> > extra overhead for loading and/or maintaining a color map database of
> > some sort.
> > 
> > So, (b) seems to me to be the right way to go.  Other opinions?
> 
> If I understand correctly I would:
> 
> - apply the recent reversion of 6351fb08
> - do (c) but just keep it to hex values
> - reapply 6351fb08 and use (c) at appropriate call sites

Patches posted for the above suggested approach.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 2/2] Re-apply "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

[With hexadecimal color values now handled via their own routine,
re-introduce the negative unsigned numbers fix.]

strtoul() has a side effect that when given a string representing a
negative number, it returns a negated version as the value, and does not
flag an error.  IOW, strtoul("-42", ) sets val to 42.  This could
potentially result in unintended surprise behaviors, such as if one were
to inadvertantly set a config param to -1 expecting that to disable it,
but with the result of setting the param to 1 instead.

Catch this by using strtol() and then manually check for the negative
value.  This logic is modelled after Wayland's strtouint().

Note that this change unfortunately reduces the range of parseable
numbers from [0,UINT_MAX] to [0,INT_MAX].  The current users of
weston_config_section_get_uint() are anticipating numbers far smaller
than either of these limits, so the change is believed to have no impact
in practice.

Also add a test case for negative numbers that catches this error
condition.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
---
 shared/config-parser.c | 12 +++-
 tests/config-parser-test.c | 31 +++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index f040380..4183551 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -186,6 +186,7 @@ weston_config_section_get_uint(struct weston_config_section 
*section,
   const char *key,
   uint32_t *value, uint32_t default_value)
 {
+   long int ret;
struct weston_config_entry *entry;
char *end;
 
@@ -197,13 +198,22 @@ weston_config_section_get_uint(struct 
weston_config_section *section,
}
 
errno = 0;
-   *value = strtoul(entry->value, , 0);
+   ret = strtol(entry->value, , 0);
if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
}
 
+   /* check range */
+   if (ret < 0 || ret > INT_MAX) {
+   *value = default_value;
+   errno = ERANGE;
+   return -1;
+   }
+
+   *value = ret;
+
return 0;
 }
 
diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index f9495b3..115f5ff 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -117,6 +117,7 @@ static struct zuc_fixture config_test_t1 = {
"# more comments\n"
"number=5252\n"
"zero=0\n"
+   "negative=-42\n"
"flag=false\n"
"color-none=0x00\n"
"color-low=0x11223344\n"
@@ -541,6 +542,36 @@ ZUC_TEST_F(config_test_t1, test024, data)
ZUC_ASSERT_EQ(EINVAL, errno);
 }
 
+ZUC_TEST_F(config_test_t1, test025, data)
+{
+   int r;
+   int32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_int(section, "negative", , 600);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(-42, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test026, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_uint(section, "negative", , 600);
+
+   ZUC_ASSERT_EQ(-1, r);
+   ZUC_ASSERT_EQ(600, n);
+   ZUC_ASSERT_EQ(ERANGE, errno);
+}
+
 ZUC_TEST_F(config_test_t2, doesnt_parse, data)
 {
struct weston_config *config = data;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 1/2] config-parser: Add weston_config_section_get_color

[Bryce Harrington]

Previously weston_config_section_get_uint was serving dual purpose for
parsing both unsigned decimal integer values (ids, counts, seconds,
etc.)  and hexadecimal values (colors), by relying on strtoul's
auto-detection mechanism.

However, this usage is unable to catch certain kinds of error
conditions, such as specifying a negative number where an unsigned
should be used.  And for colors in particular, it would misparse hex
values if the leading 0x was omitted.  E.g. "background-color="
would render a near-black background (effectively 0x05f5e0ff) instead of
medium grey, and "background-color=" would be treated as an
error rather than white.  "background-color=0x01234567",
"background-color=01234567", and "background-color=1234567" each
resulted in the value being parsed as hexadecimal, octal, and decimal
respectively, resulting in colors 0x01234567, 0x00053977, and 0x0012d687
being displayed.

This new routine forces hexadecimal to be used in all cases when parsing
color values, so "0x01234567", "01234567", and "1234567" all result in
the same color value, "" is grey, and "" is white.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 clients/desktop-shell.c|  8 ++--
 clients/ivi-shell-user-interface.c |  2 +-
 shared/config-parser.c | 27 +
 shared/config-parser.h |  4 ++
 tests/config-parser-test.c | 80 ++
 5 files changed, 116 insertions(+), 5 deletions(-)

diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
index f5e0ba2..18b8f9e 100644
--- a/clients/desktop-shell.c
+++ b/clients/desktop-shell.c
@@ -571,8 +571,8 @@ panel_create(struct desktop *desktop)
free (clock_format_option);
 
s = weston_config_get_section(desktop->config, "shell", NULL, NULL);
-   weston_config_section_get_uint(s, "panel-color",
-  >color, 0xaa00);
+   weston_config_section_get_color(s, "panel-color",
+   >color, 0xaa00);
 
panel_add_launchers(panel, desktop);
 
@@ -1068,8 +1068,8 @@ background_create(struct desktop *desktop)
s = weston_config_get_section(desktop->config, "shell", NULL, NULL);
weston_config_section_get_string(s, "background-image",
 >image, NULL);
-   weston_config_section_get_uint(s, "background-color",
-  >color, 0);
+   weston_config_section_get_color(s, "background-color",
+   >color, 0x);
 
weston_config_section_get_string(s, "background-type",
 , "tile");
diff --git a/clients/ivi-shell-user-interface.c 
b/clients/ivi-shell-user-interface.c
index 06b79a2..465def1 100644
--- a/clients/ivi-shell-user-interface.c
+++ b/clients/ivi-shell-user-interface.c
@@ -1143,7 +1143,7 @@ hmi_homescreen_setting_create(void)
weston_config_section_get_uint(
shellSection, "home-id", >home.id, 1007);
 
-   weston_config_section_get_uint(
+   weston_config_section_get_color(
shellSection, "workspace-background-color",
>workspace_background.color, 0x9900);
 
diff --git a/shared/config-parser.c b/shared/config-parser.c
index 1e08759..f040380 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -209,6 +209,33 @@ weston_config_section_get_uint(struct 
weston_config_section *section,
 
 WL_EXPORT
 int
+weston_config_section_get_color(struct weston_config_section *section,
+   const char *key,
+   uint32_t *color, uint32_t default_color)
+{
+   struct weston_config_entry *entry;
+   char *end;
+
+   entry = config_section_get_entry(section, key);
+   if (entry == NULL) {
+   *color = default_color;
+   errno = ENOENT;
+   return -1;
+   }
+
+   errno = 0;
+   *color = strtoul(entry->value, , 16);
+   if (errno != 0 || end == entry->value || *end != '\0') {
+   *color = default_color;
+   errno = EINVAL;
+   return -1;
+   }
+
+   return 0;
+}
+
+WL_EXPORT
+int
 weston_config_section_get_double(struct weston_config_section *section,
 const char *key,
 double *value, double default_value)
diff --git a/shared/config-parser.h b/shared/config-parser.h
index b8462a7..17ef5d7 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -85,6 +85,10 @@ weston_config_section_get_uint(struct weston_config_section 
*secti

Re: [PATCH wayland-protocols v5 3/4] tablet: restrict the cursor surface to one per tool

[Bryce Harrington]

On Mon, Jul 11, 2016 at 05:13:35PM +0200, Carlos Garnacho wrote:
> From: Peter Hutterer <peter.hutte...@who-t.net>
> 
> The initial approach was to allow one surface to be re-used between tools,
> seats and even used together as wl_pointer cursor surface. This has a few
> drawbacks, most of which are related to managing the surface correctly in the
> compositor. For example, the same cursor surface could have two different
> hotspots. Animated cursors should animate independently rather than update at
> the same time.
> 
> Furthermore: a client cannot know when a surface will cease being used as a
> cursor surface. The basic assumption of "after focus out" is an implementation
> detail in the compositor and unless the client unsets the cursor it is not
> guaranteed that the surface is released. This again makes sharing a surface
> less obvious - you cannot know if the wl_pointer surface is still in use when
> you set it for a new wp_tablet_tool.
> 
> Avoid these headaches (and push some of them to the client) by simply
> restricting a wl_surface to be assigned to a single tool. For the 99% use case
> where we have one tablet with two tools (pen + eraser) this means we merely
> get two extra surfaces, and the two don't usually share the same cursor shape
> anyway. If sharing is absolutely necessary, a client may still opt to share
> the underlying wl_buffer.
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> Reviewed-by: Jason Gerecke <jason.gere...@wacom.com>
> Reviewed-by: Carlos Garnacho <carl...@gnome.org>

Makes sense; overloading behaviors and trying to do too many things at
once often leads to oddball corner cases.

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

> ---
>  unstable/tablet/tablet-unstable-v2.xml | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/unstable/tablet/tablet-unstable-v2.xml 
> b/unstable/tablet/tablet-unstable-v2.xml
> index de9217b..77b185c 100644
> --- a/unstable/tablet/tablet-unstable-v2.xml
> +++ b/unstable/tablet/tablet-unstable-v2.xml
> @@ -225,13 +225,11 @@
>   and pending input regions become undefined, and the wl_surface is
>   unmapped.
>  
> - This request gives the surface the role of a cursor. The role
> - assigned by this request is the same as assigned by
> - wl_pointer.set_cursor meaning the same surface can be
> - used both as a wl_pointer cursor and a wp_tablet cursor. If the
> - surface already has another role, it raises a protocol error.
> - The surface may be used on multiple tablets and across multiple
> - seats.
> + This request gives the surface the role of a wp_tablet_tool cursor. A
> + surface may only ever be used as the cursor surface for one
> + wp_tablet_tool. If the surface already has another role or has
> + previously been used as cursor surface for a different tool, a
> + protocol error is raised.
>
>
> allow-null="true"/>
> -- 
> 2.7.4
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v5 2/4] tablet: change all degree values from int to wl_fixed

[Bryce Harrington]

On Mon, Jul 11, 2016 at 05:13:34PM +0200, Carlos Garnacho wrote:
> From: Peter Hutterer <peter.hutte...@who-t.net>
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> Reviewed-by: Jason Gerecke <jason.gere...@wacom.com>
> Reviewed-by: Carlos Garnacho <carl...@gnome.org>

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

> ---
>  unstable/tablet/tablet-unstable-v2.xml | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/unstable/tablet/tablet-unstable-v2.xml 
> b/unstable/tablet/tablet-unstable-v2.xml
> index 81e9835..de9217b 100644
> --- a/unstable/tablet/tablet-unstable-v2.xml
> +++ b/unstable/tablet/tablet-unstable-v2.xml
> @@ -478,21 +478,21 @@
>  
>
>   Sent whenever one or both of the tilt axes on a tool change. Each tilt
> - value is in 0.01 of a degree, relative to the z-axis of the tablet.
> + value is in degrees, relative to the z-axis of the tablet.
>   The angle is positive when the top of a tool tilts along the
>   positive x or y axis.
>
> -  
> -  
> +  
> +  
>  
>  
>  
>
>   Sent whenever the z-rotation axis on the tool changes. The
> - rotation value is in 0.01 of a degree clockwise from the tool's
> + rotation value is in degrees clockwise from the tool's
>   logical neutral position.
>
> -  
> +  
>  
>  
>  
> @@ -510,10 +510,10 @@
>
>   Sent whenever the wheel on the tool emits an event. This event
>   contains two values for the same axis change. The degrees value is
> - in 0.01 of a degree in the same orientation as the
> - wl_pointer.vertical_scroll axis. The clicks value is in discrete
> - logical clicks of the mouse wheel. This value may be zero if the
> - movement of the wheel was less than one logical click.
> + in the same orientation as the wl_pointer.vertical_scroll axis. The
> + clicks value is in discrete logical clicks of the mouse wheel. This
> + value may be zero if the movement of the wheel was less
> + than one logical click.
>  
>   Clients should choose either value and avoid mixing degrees and
>   clicks. The compositor may accumulate values smaller than a logical
> @@ -521,7 +521,7 @@
>   Thus, wl_tablet_tool.wheel events with non-zero clicks values may
>   have different degrees values.
>
> -  
> +  
>
>  
>  
> -- 
> 2.7.4
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v5 1/4] tablet: add v2 of the tablet protocol

[Bryce Harrington]

On Mon, Jul 11, 2016 at 05:13:33PM +0200, Carlos Garnacho wrote:
> From: Peter Hutterer <peter.hutte...@who-t.net>
> 
> This is a straightforward copy/paste with a _v1 -> _v2 rename. No functional
> changes otherwise.
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> Reviewed-by: Jason Gerecke <jason.gere...@wacom.com>
> Reviewed-by: Carlos Garnacho <carl...@gnome.org>

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

> ---
>  Makefile.am|   1 +
>  unstable/tablet/tablet-unstable-v2.xml | 641 
> +
>  2 files changed, 642 insertions(+)
>  create mode 100644 unstable/tablet/tablet-unstable-v2.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 71d2632..9e2a029 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -8,6 +8,7 @@ unstable_protocols =  
> \
>   unstable/relative-pointer/relative-pointer-unstable-v1.xml  
> \
>   unstable/pointer-constraints/pointer-constraints-unstable-v1.xml
> \
>   unstable/tablet/tablet-unstable-v1.xml  
> \
> + unstable/tablet/tablet-unstable-v2.xml  
> \
>   $(NULL)
>  
>  stable_protocols =   
> \
> diff --git a/unstable/tablet/tablet-unstable-v2.xml 
> b/unstable/tablet/tablet-unstable-v2.xml
> new file mode 100644
> index 000..81e9835
> --- /dev/null
> +++ b/unstable/tablet/tablet-unstable-v2.xml
> @@ -0,0 +1,641 @@
> +
> +
> +
> +  
> +Copyright 2014 © Stephen "Lyude" Chandler Paul
> +Copyright 2015-2016 © Red Hat, Inc.
> +
> +Permission is hereby granted, free of charge, to any person
> +obtaining a copy of this software and associated documentation files
> +(the "Software"), to deal in the Software without restriction,
> +including without limitation the rights to use, copy, modify, merge,
> +publish, distribute, sublicense, and/or sell copies of the Software,
> +and to permit persons to whom the Software is furnished to do so,
> +subject to the following conditions:
> +
> +The above copyright notice and this permission notice (including the
> +next paragraph) shall be included in all copies or substantial
> +portions of the Software.
> +
> +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> +NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> +BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> +ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> +CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> +SOFTWARE.
> +  
> +
> +  
> +This description provides a high-level overview of the interplay between
> +the interfaces defined this protocol. For details, see the protocol
> +specification.
> +
> +More than one tablet may exist, and device-specifics matter. Tablets are
> +not represented by a single virtual device like wl_pointer. A client
> +binds to the tablet manager object which is just a proxy object. From
> +that, the client requests wp_tablet_manager.get_tablet_seat(wl_seat)
> +and that returns the actual interface that has all the tablets. With
> +this indirection, we can avoid merging wp_tablet into the actual Wayland
> +protocol, a long-term benefit.
> +
> +The wp_tablet_seat sends a "tablet added" event for each tablet
> +connected. That event is followed by descriptive events about the
> +hardware; currently that includes events for name, vid/pid and
> +a wp_tablet.path event that describes a local path. This path can be
> +used to uniquely identify a tablet or get more information through
> +libwacom. Emulated or nested tablets can skip any of those, e.g. a
> +virtual tablet may not have a vid/pid. The sequence of descriptive
> +events is terminated by a wp_tablet.done event to signal that a client
> +may now finalize any initialization for that tablet.
> +
> +Events from tablets require a tool in proximity. Tools are also managed
> +by the tablet seat; a "tool added" event is sent whenever a tool is new
> +to the compositor. That event is followed by a number of descriptive
> +events about the hardware; currently that includes capabilities,
> +hardware id and serial number, and tool type. Similar to the tablet
> 

Re: [PATCH weston] Revert "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

On Wed, Jul 13, 2016 at 02:12:43PM -0700, Yong Bakos wrote:
> On Jul 13, 2016, at 1:27 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > The reduction in range limits does have an effect for color values,
> > which are expressed as hexadecimal values from 0x to
> > 0x.  By limiting the range to INT_MAX, color values of
> > 0x8000 and up are in fact lost.
> > 
> > This reverts commit 6351fb08c2e302f8696b2022830e5317e7219c39.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> Reviewed-by: Yong Bakos <yba...@humanoriented.com>
> Tested-by: Yong Bakos <yba...@humanoriented.com>

Thanks, Derek also acked it as looking ok on irc.

Pushed:
   a8d987d..03793e3  master -> master

 
> yong
> 
> 
> > ---
> > shared/config-parser.c | 12 +---
> > tests/config-parser-test.c | 31 ---
> > 2 files changed, 1 insertion(+), 42 deletions(-)
> > 
> > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > index 4c67220..1e08759 100644
> > --- a/shared/config-parser.c
> > +++ b/shared/config-parser.c
> > @@ -186,7 +186,6 @@ weston_config_section_get_uint(struct 
> > weston_config_section *section,
> >const char *key,
> >uint32_t *value, uint32_t default_value)
> > {
> > -   long int ret;
> > struct weston_config_entry *entry;
> > char *end;
> > 
> > @@ -198,22 +197,13 @@ weston_config_section_get_uint(struct 
> > weston_config_section *section,
> > }
> > 
> > errno = 0;
> > -   ret = strtol(entry->value, , 0);
> > +   *value = strtoul(entry->value, , 0);
> > if (errno != 0 || end == entry->value || *end != '\0') {
> > *value = default_value;
> > errno = EINVAL;
> > return -1;
> > }
> > 
> > -   /* check range */
> > -   if (ret < 0 || ret > INT_MAX) {
> > -   *value = default_value;
> > -   errno = ERANGE;
> > -   return -1;
> > -   }
> > -
> > -   *value = ret;
> > -
> > return 0;
> > }
> > 
> > diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> > index f88e89b..735da4e 100644
> > --- a/tests/config-parser-test.c
> > +++ b/tests/config-parser-test.c
> > @@ -117,7 +117,6 @@ static struct zuc_fixture config_test_t1 = {
> > "# more comments\n"
> > "number=5252\n"
> > "zero=0\n"
> > -   "negative=-42\n"
> > "flag=false\n"
> > "\n"
> > "[stuff]\n"
> > @@ -462,36 +461,6 @@ ZUC_TEST_F(config_test_t1, test019, data)
> > ZUC_ASSERT_EQ(0, errno);
> > }
> > 
> > -ZUC_TEST_F(config_test_t1, test020, data)
> > -{
> > -   int r;
> > -   int32_t n;
> > -   struct weston_config_section *section;
> > -   struct weston_config *config = data;
> > -
> > -   section = weston_config_get_section(config, "bar", NULL, NULL);
> > -   r = weston_config_section_get_int(section, "negative", , 600);
> > -
> > -   ZUC_ASSERT_EQ(0, r);
> > -   ZUC_ASSERT_EQ(-42, n);
> > -   ZUC_ASSERT_EQ(0, errno);
> > -}
> > -
> > -ZUC_TEST_F(config_test_t1, test021, data)
> > -{
> > -   int r;
> > -   uint32_t n;
> > -   struct weston_config_section *section;
> > -   struct weston_config *config = data;
> > -
> > -   section = weston_config_get_section(config, "bar", NULL, NULL);
> > -   r = weston_config_section_get_uint(section, "negative", , 600);
> > -
> > -   ZUC_ASSERT_EQ(-1, r);
> > -   ZUC_ASSERT_EQ(600, n);
> > -   ZUC_ASSERT_EQ(ERANGE, errno);
> > -}
> > -
> > ZUC_TEST_F(config_test_t2, doesnt_parse, data)
> > {
> > struct weston_config *config = data;
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] Revert "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

On Wed, Jul 13, 2016 at 01:27:29PM -0700, Bryce Harrington wrote:
> The reduction in range limits does have an effect for color values,
> which are expressed as hexadecimal values from 0x to
> 0x.  By limiting the range to INT_MAX, color values of
> 0x8000 and up are in fact lost.
> 
> This reverts commit 6351fb08c2e302f8696b2022830e5317e7219c39.
> 
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

Here are some alternative solutions:

a.  Use strtoll instead of strtol

b.  Add weston_config_section_get_hex()

c.  Add weston_config_section_get_color(), that supports both
hexadecimal and color names (ala rgb.txt or similar)

I'm leaning towards (b), and sounds like Peter was thinking along those
lines.  Splitting hexadecimal handling out of get_uint() would also
allow specifying base-10, and would allow stronger type checking
generally.

We could also do both (a) and (b).  But I don't know whether we actually
need the full range for unsigned ints other than for hexadecimal values.
I also don't know if there's any difference in overhead between strtoll
vs. strtol.

Option (c) I think would be spiffy but I'm skeptical that using color
names would really be all that useful in practice, and it would require
extra overhead for loading and/or maintaining a color map database of
some sort.

So, (b) seems to me to be the right way to go.  Other opinions?

Bryce

> ---
>  shared/config-parser.c | 12 +---
>  tests/config-parser-test.c | 31 ---
>  2 files changed, 1 insertion(+), 42 deletions(-)
> 
> diff --git a/shared/config-parser.c b/shared/config-parser.c
> index 4c67220..1e08759 100644
> --- a/shared/config-parser.c
> +++ b/shared/config-parser.c
> @@ -186,7 +186,6 @@ weston_config_section_get_uint(struct 
> weston_config_section *section,
>  const char *key,
>  uint32_t *value, uint32_t default_value)
>  {
> - long int ret;
>   struct weston_config_entry *entry;
>   char *end;
>  
> @@ -198,22 +197,13 @@ weston_config_section_get_uint(struct 
> weston_config_section *section,
>   }
>  
>   errno = 0;
> - ret = strtol(entry->value, , 0);
> + *value = strtoul(entry->value, , 0);
>   if (errno != 0 || end == entry->value || *end != '\0') {
>   *value = default_value;
>   errno = EINVAL;
>   return -1;
>   }
>  
> - /* check range */
> - if (ret < 0 || ret > INT_MAX) {
> - *value = default_value;
> - errno = ERANGE;
> - return -1;
> - }
> -
> - *value = ret;
> -
>   return 0;
>  }
>  
> diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> index f88e89b..735da4e 100644
> --- a/tests/config-parser-test.c
> +++ b/tests/config-parser-test.c
> @@ -117,7 +117,6 @@ static struct zuc_fixture config_test_t1 = {
>   "# more comments\n"
>   "number=5252\n"
>   "zero=0\n"
> - "negative=-42\n"
>   "flag=false\n"
>   "\n"
>   "[stuff]\n"
> @@ -462,36 +461,6 @@ ZUC_TEST_F(config_test_t1, test019, data)
>   ZUC_ASSERT_EQ(0, errno);
>  }
>  
> -ZUC_TEST_F(config_test_t1, test020, data)
> -{
> - int r;
> - int32_t n;
> - struct weston_config_section *section;
> - struct weston_config *config = data;
> -
> - section = weston_config_get_section(config, "bar", NULL, NULL);
> - r = weston_config_section_get_int(section, "negative", , 600);
> -
> - ZUC_ASSERT_EQ(0, r);
> - ZUC_ASSERT_EQ(-42, n);
> - ZUC_ASSERT_EQ(0, errno);
> -}
> -
> -ZUC_TEST_F(config_test_t1, test021, data)
> -{
> - int r;
> - uint32_t n;
> - struct weston_config_section *section;
> - struct weston_config *config = data;
> -
> - section = weston_config_get_section(config, "bar", NULL, NULL);
> - r = weston_config_section_get_uint(section, "negative", , 600);
> -
> - ZUC_ASSERT_EQ(-1, r);
> - ZUC_ASSERT_EQ(600, n);
> - ZUC_ASSERT_EQ(ERANGE, errno);
> -}
> -
>  ZUC_TEST_F(config_test_t2, doesnt_parse, data)
>  {
>   struct weston_config *config = data;
> -- 
> 1.9.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] Revert "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

The reduction in range limits does have an effect for color values,
which are expressed as hexadecimal values from 0x to
0x.  By limiting the range to INT_MAX, color values of
0x8000 and up are in fact lost.

This reverts commit 6351fb08c2e302f8696b2022830e5317e7219c39.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 shared/config-parser.c | 12 +---
 tests/config-parser-test.c | 31 ---
 2 files changed, 1 insertion(+), 42 deletions(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index 4c67220..1e08759 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -186,7 +186,6 @@ weston_config_section_get_uint(struct weston_config_section 
*section,
   const char *key,
   uint32_t *value, uint32_t default_value)
 {
-   long int ret;
struct weston_config_entry *entry;
char *end;
 
@@ -198,22 +197,13 @@ weston_config_section_get_uint(struct 
weston_config_section *section,
}
 
errno = 0;
-   ret = strtol(entry->value, , 0);
+   *value = strtoul(entry->value, , 0);
if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
}
 
-   /* check range */
-   if (ret < 0 || ret > INT_MAX) {
-   *value = default_value;
-   errno = ERANGE;
-   return -1;
-   }
-
-   *value = ret;
-
return 0;
 }
 
diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index f88e89b..735da4e 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -117,7 +117,6 @@ static struct zuc_fixture config_test_t1 = {
"# more comments\n"
"number=5252\n"
"zero=0\n"
-   "negative=-42\n"
"flag=false\n"
"\n"
"[stuff]\n"
@@ -462,36 +461,6 @@ ZUC_TEST_F(config_test_t1, test019, data)
ZUC_ASSERT_EQ(0, errno);
 }
 
-ZUC_TEST_F(config_test_t1, test020, data)
-{
-   int r;
-   int32_t n;
-   struct weston_config_section *section;
-   struct weston_config *config = data;
-
-   section = weston_config_get_section(config, "bar", NULL, NULL);
-   r = weston_config_section_get_int(section, "negative", , 600);
-
-   ZUC_ASSERT_EQ(0, r);
-   ZUC_ASSERT_EQ(-42, n);
-   ZUC_ASSERT_EQ(0, errno);
-}
-
-ZUC_TEST_F(config_test_t1, test021, data)
-{
-   int r;
-   uint32_t n;
-   struct weston_config_section *section;
-   struct weston_config *config = data;
-
-   section = weston_config_get_section(config, "bar", NULL, NULL);
-   r = weston_config_section_get_uint(section, "negative", , 600);
-
-   ZUC_ASSERT_EQ(-1, r);
-   ZUC_ASSERT_EQ(600, n);
-   ZUC_ASSERT_EQ(ERANGE, errno);
-}
-
 ZUC_TEST_F(config_test_t2, doesnt_parse, data)
 {
struct weston_config *config = data;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 0/3] Standardize error checking for strtol calls

[Bryce Harrington]

Make error handling of most strtol calls consistent with each other.

The first patch fixes various cases that are straightforward refactors.
The second and third patch cover cases where some idiosyncracies need
highlighted in the commit messages.  All three patches can be squashed
if desired.

Note that intentional omissions are the strtol() calls in the terminal.c
and multi-resource.c clients, because those usages make use of
strtol()'s return of the unparsed portion of the string.  Those could
also use improvement of error handling but will be different enough to
need special attention.

Bryce Harrington (3):
  Standardize error checking for strtol calls
  xwayland: Improve error checking for strtol call
  option-parser: Improve error checking for strtol call

 compositor/main.c   |  3 ++-
 compositor/systemd-notify.c |  2 +-
 libweston/compositor.c  |  3 ++-
 libweston/libbacklight.c| 10 +-
 shared/option-parser.c  | 11 +--
 xwayland/launcher.c |  3 ++-
 6 files changed, 25 insertions(+), 7 deletions(-)

-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 3/3] option-parser: Improve error checking for strtol call

[Bryce Harrington]

Make the error checking consistent with other strtol() calls.

Note that since strtol(nptr, ) sets endptr == nptr if there were
no digits, this catches the case where the string was blank, so there's
no need to test *value != '\0'.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 shared/option-parser.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/shared/option-parser.c b/shared/option-parser.c
index 33355b8..fb4a342 100644
--- a/shared/option-parser.c
+++ b/shared/option-parser.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "config-parser.h"
 
@@ -40,11 +41,17 @@ handle_option(const struct weston_option *option, char 
*value)
 
switch (option->type) {
case WESTON_OPTION_INTEGER:
+   errno = 0;
* (int32_t *) option->data = strtol(value, , 10);
-   return *value && !*p;
+   if (errno != 0 || p == value || *p != '\0')
+   return 0;
+   return 1;
case WESTON_OPTION_UNSIGNED_INTEGER:
+   errno = 0;
* (uint32_t *) option->data = strtoul(value, , 10);
-   return *value && !*p;
+   if (errno != 0 || p == value || *p != '\0')
+   return 0;
+   return 1;
case WESTON_OPTION_STRING:
* (char **) option->data = strdup(value);
return 1;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 2/3] xwayland: Improve error checking for strtol call

[Bryce Harrington]

This updates the error checking for the strtol() call in xwayland's
create_lockfile to match other cases.  C.f. cbc05378 and other recent
patches.

A notable difference here is that the existing error checking was
verifying that exactly 10 digits were being read from the lock file,
but the fact that it's 10 digits is just an implementation detail for
how we're writing it.  The pid could be a shorter number of digits, and
would just be space-padded on the left.

This change allows the file to contain any number of digits, but it
can't be blank, all of the digits must be numeric, and the resulting
number must be within the accepted range.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 xwayland/launcher.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xwayland/launcher.c b/xwayland/launcher.c
index b7aee3b..5b81fef 100644
--- a/xwayland/launcher.c
+++ b/xwayland/launcher.c
@@ -164,8 +164,9 @@ create_lockfile(int display, char *lockfile, size_t lsize)
return -1;
}
 
+   errno = 0;
other = strtol(pid, , 10);
-   if (end != pid + 10) {
+   if (errno != 0 || end == pid || *end != '\0') {
weston_log("can't parse lock file %s\n",
lockfile);
close(fd);
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 1/3] Standardize error checking for strtol calls

[Bryce Harrington]

This tightens up the strtol() error checking in several places where it
is used for parsing environment variables, and in the backlight
interface that is reading numbers from files under /sys/class/backlight.
All of these uses are expecting strings containing decimal numbers and
nothing else, so the error checking can all be tightened up and made
consistent with other strtol() calls.

This follows the error checking style used in Wayland
(c.f. wayland-client.c and scanner.c) and c.f. commit cbc05378.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 compositor/main.c   |  3 ++-
 compositor/systemd-notify.c |  2 +-
 libweston/compositor.c  |  3 ++-
 libweston/libbacklight.c| 10 +-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/compositor/main.c b/compositor/main.c
index 1f75ae0..27be973 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -1684,8 +1684,9 @@ int main(int argc, char *argv[])
server_socket = getenv("WAYLAND_SERVER_SOCKET");
if (server_socket) {
weston_log("Running with single client\n");
+   errno = 0;
fd = strtol(server_socket, , 10);
-   if (*end != '\0')
+   if (errno != 0 || end == server_socket || *end != '\0')
fd = -1;
} else {
fd = -1;
diff --git a/compositor/systemd-notify.c b/compositor/systemd-notify.c
index 9fbd5ee..6104124 100644
--- a/compositor/systemd-notify.c
+++ b/compositor/systemd-notify.c
@@ -146,7 +146,7 @@ module_init(struct weston_compositor *compositor,
 
errno = 0;
watchdog_time_conv = strtol(watchdog_time_env, , 10);
-   if ((errno != 0) || (*tail != '\0'))
+   if (errno != 0 || tail == watchdog_time_env || *tail != '\0')
return 0;
 
/* Convert 'WATCHDOG_USEC' to milliseconds and notify
diff --git a/libweston/compositor.c b/libweston/compositor.c
index 96eeb17..67f334c 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -4617,8 +4617,9 @@ weston_environment_get_fd(const char *env)
e = getenv(env);
if (!e)
return -1;
+   errno = 0;
fd = strtol(e, , 10);
-   if (*end != '\0')
+   if (errno != 0 || end == e || *end != '\0')
return -1;
 
flags = fcntl(fd, F_GETFD);
diff --git a/libweston/libbacklight.c b/libweston/libbacklight.c
index 722d66f..f1205e8 100644
--- a/libweston/libbacklight.c
+++ b/libweston/libbacklight.c
@@ -47,6 +47,7 @@ static long backlight_get(struct backlight *backlight, char 
*node)
 {
char buffer[100];
char *path;
+   char *end;
int fd;
long value, ret;
 
@@ -64,8 +65,15 @@ static long backlight_get(struct backlight *backlight, char 
*node)
goto out;
}
 
-   value = strtol(buffer, NULL, 10);
+   errno = 0;
+   value = strtol(buffer, , 10);
+   if (errno != 0 || end == buffer || *end != '\0') {
+   ret = -1;
+   goto out;
+   }
+
ret = value;
+
 out:
if (fd >= 0)
close(fd);
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] Require base-10 for strtol() calls

[Bryce Harrington]

On Tue, Jul 12, 2016 at 06:49:06PM -0700, Yong Bakos wrote:
> On Jul 12, 2016, at 4:51 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > The third arg to strtol() specifies the base to assume for the number.
> > When 0 is passed, as is currently done in option-parser.c, hexadecimal
> > and octal numbers are permitted and automatically detected and
> > converted.
> > 
> > This change is an expansion of f6051cbab84c0e577473b67f0585c0f329eb80fe
> > to cover the remaining strtol() calls in Weston, where the routine is
> > being used to read fds and pids - which are always expressed in base-10.
> 
> I believe you missed two calls, see:
> 
> config-parser.c: weston_config_section_get_uint
> systemd-notify.c: module_init

Thanks for checking and catching.  I intentionally skipped changing
weston_config_section_get_uint() since is used to parse some color
values which are expressed as hexadecimal so forcing base-10 would break
those.  See my email to Peter.

But you're right that the strtol call in system-notify.c needs this fix
too.

Bryce


> > It also changes the calls in config-parser, used by
> > weston_config_section_get_int(), which in turn is being used to read
> > scales, sizes, times, rates, and delays; these are all expressed in
> > base-10 numbers only.
> > 
> > The benefit of limiting this to base-10 is to eliminate surprises when
> > parsing numbers from the command line.  Also, by making the code
> > consistent with other usages of strtol, it may make it possible to
> > factor out the common code in the future.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> > compositor/main.c  | 2 +-
> > libweston/compositor.c | 2 +-
> > shared/config-parser.c | 2 +-
> > xwayland/launcher.c| 2 +-
> > 4 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/compositor/main.c b/compositor/main.c
> > index 6cf9194..8400d70 100644
> > --- a/compositor/main.c
> > +++ b/compositor/main.c
> > @@ -1684,7 +1684,7 @@ int main(int argc, char *argv[])
> > server_socket = getenv("WAYLAND_SERVER_SOCKET");
> > if (server_socket) {
> > weston_log("Running with single client\n");
> > -   fd = strtol(server_socket, , 0);
> > +   fd = strtol(server_socket, , 10);
> > if (*end != '\0')
> > fd = -1;
> > } else {
> > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > index 771f1c9..96eeb17 100644
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c
> > @@ -4617,7 +4617,7 @@ weston_environment_get_fd(const char *env)
> > e = getenv(env);
> > if (!e)
> > return -1;
> > -   fd = strtol(e, , 0);
> > +   fd = strtol(e, , 10);
> > if (*end != '\0')
> > return -1;
> > 
> > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > index 247e880..4c67220 100644
> > --- a/shared/config-parser.c
> > +++ b/shared/config-parser.c
> > @@ -170,7 +170,7 @@ weston_config_section_get_int(struct 
> > weston_config_section *section,
> > }
> > 
> > errno = 0;
> > -   *value = strtol(entry->value, , 0);
> > +   *value = strtol(entry->value, , 10);
> > if (errno != 0 || end == entry->value || *end != '\0') {
> > *value = default_value;
> > errno = EINVAL;
> > diff --git a/xwayland/launcher.c b/xwayland/launcher.c
> > index 614ef5b..b7aee3b 100644
> > --- a/xwayland/launcher.c
> > +++ b/xwayland/launcher.c
> > @@ -164,7 +164,7 @@ create_lockfile(int display, char *lockfile, size_t 
> > lsize)
> > return -1;
> > }
> > 
> > -   other = strtol(pid, , 0);
> > +   other = strtol(pid, , 10);
> > if (end != pid + 10) {
> > weston_log("can't parse lock file %s\n",
> > lockfile);
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] Include space in 'if ('

[Bryce Harrington]

On Wed, Jul 13, 2016 at 10:23:42AM +1000, Peter Hutterer wrote:
> On Tue, Jul 12, 2016 at 04:59:05PM -0700, Bryce Harrington wrote:
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> 
> Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>
> but seriously, imo you should push things like this directly, I'm not sure
> how many eyeballs whitespace changes really need :)

Yeah you're right, I debated it.  ;-)
Thanks for the r-b, will push with the other patch.

Bryce
 
> Cheers,
>Peter
> 
> >  compositor/main.c | 2 +-
> >  ivi-shell/ivi-layout-transition.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/compositor/main.c b/compositor/main.c
> > index 8400d70..1f75ae0 100644
> > --- a/compositor/main.c
> > +++ b/compositor/main.c
> > @@ -1499,7 +1499,7 @@ load_wayland_backend(struct weston_compositor *c,
> > int ret = 0;
> >  
> > ret = load_wayland_backend_config(c, argc, argv, wc, );
> > -   if(ret < 0) {
> > +   if (ret < 0) {
> > weston_wayland_backend_config_release();
> > return ret;
> > }
> > diff --git a/ivi-shell/ivi-layout-transition.c 
> > b/ivi-shell/ivi-layout-transition.c
> > index 04b62a5..4913db4 100644
> > --- a/ivi-shell/ivi-layout-transition.c
> > +++ b/ivi-shell/ivi-layout-transition.c
> > @@ -439,7 +439,7 @@ ivi_layout_transition_move_resize_view(struct 
> > ivi_layout_surface *surface,
> > transition_move_resize_view_destroy,
> > duration);
> >  
> > -   if(transition && layout_transition_register(transition))
> > +   if (transition && layout_transition_register(transition))
> > return;
> > layout_transition_destroy(transition);
> >  }
> > -- 
> > 1.9.1
>  
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] Include space in 'if ('

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 compositor/main.c | 2 +-
 ivi-shell/ivi-layout-transition.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/compositor/main.c b/compositor/main.c
index 8400d70..1f75ae0 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -1499,7 +1499,7 @@ load_wayland_backend(struct weston_compositor *c,
int ret = 0;
 
ret = load_wayland_backend_config(c, argc, argv, wc, );
-   if(ret < 0) {
+   if (ret < 0) {
weston_wayland_backend_config_release();
return ret;
}
diff --git a/ivi-shell/ivi-layout-transition.c 
b/ivi-shell/ivi-layout-transition.c
index 04b62a5..4913db4 100644
--- a/ivi-shell/ivi-layout-transition.c
+++ b/ivi-shell/ivi-layout-transition.c
@@ -439,7 +439,7 @@ ivi_layout_transition_move_resize_view(struct 
ivi_layout_surface *surface,
transition_move_resize_view_destroy,
duration);
 
-   if(transition && layout_transition_register(transition))
+   if (transition && layout_transition_register(transition))
return;
layout_transition_destroy(transition);
 }
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] Require base-10 for strtol() calls

[Bryce Harrington]

The third arg to strtol() specifies the base to assume for the number.
When 0 is passed, as is currently done in option-parser.c, hexadecimal
and octal numbers are permitted and automatically detected and
converted.

This change is an expansion of f6051cbab84c0e577473b67f0585c0f329eb80fe
to cover the remaining strtol() calls in Weston, where the routine is
being used to read fds and pids - which are always expressed in base-10.
It also changes the calls in config-parser, used by
weston_config_section_get_int(), which in turn is being used to read
scales, sizes, times, rates, and delays; these are all expressed in
base-10 numbers only.

The benefit of limiting this to base-10 is to eliminate surprises when
parsing numbers from the command line.  Also, by making the code
consistent with other usages of strtol, it may make it possible to
factor out the common code in the future.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 compositor/main.c  | 2 +-
 libweston/compositor.c | 2 +-
 shared/config-parser.c | 2 +-
 xwayland/launcher.c| 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/compositor/main.c b/compositor/main.c
index 6cf9194..8400d70 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -1684,7 +1684,7 @@ int main(int argc, char *argv[])
server_socket = getenv("WAYLAND_SERVER_SOCKET");
if (server_socket) {
weston_log("Running with single client\n");
-   fd = strtol(server_socket, , 0);
+   fd = strtol(server_socket, , 10);
if (*end != '\0')
fd = -1;
} else {
diff --git a/libweston/compositor.c b/libweston/compositor.c
index 771f1c9..96eeb17 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -4617,7 +4617,7 @@ weston_environment_get_fd(const char *env)
e = getenv(env);
if (!e)
return -1;
-   fd = strtol(e, , 0);
+   fd = strtol(e, , 10);
if (*end != '\0')
return -1;
 
diff --git a/shared/config-parser.c b/shared/config-parser.c
index 247e880..4c67220 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -170,7 +170,7 @@ weston_config_section_get_int(struct weston_config_section 
*section,
}
 
errno = 0;
-   *value = strtol(entry->value, , 0);
+   *value = strtol(entry->value, , 10);
if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
diff --git a/xwayland/launcher.c b/xwayland/launcher.c
index 614ef5b..b7aee3b 100644
--- a/xwayland/launcher.c
+++ b/xwayland/launcher.c
@@ -164,7 +164,7 @@ create_lockfile(int display, char *lockfile, size_t lsize)
return -1;
}
 
-   other = strtol(pid, , 0);
+   other = strtol(pid, , 10);
if (end != pid + 10) {
weston_log("can't parse lock file %s\n",
lockfile);
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] config-parser: Catch negative numbers assigned to unsigned config values

[Bryce Harrington]

On Tue, Jul 12, 2016 at 01:17:20PM +0100, Eric Engestrom wrote:
> On Mon, Jul 11, 2016 at 05:55:15PM -0700, Bryce Harrington wrote:
> > strtoul() has a side effect that when given a string representing a
> > negative number, it returns a negated version as the value, and does not
> > flag an error.  IOW, strtoul("-42", ) sets val to 42.  This could
> > potentially result in unintended surprise behaviors, such as if one were
> > to inadvertantly set a config param to -1 expecting that to disable it,
> > but with the result of setting the param to 1 instead.
> > 
> > Catch this by using strtol() and then manually check for the negative
> > value.  This logic is modelled after Wayland's strtouint().
> > 
> > Note that this change unfortunately reduces the range of parseable
> > numbers from [0,UINT_MAX] to [0,INT_MAX].  The current users of
> > weston_config_section_get_uint() are anticipating numbers far smaller
> > than either of these limits, so the change is believed to have no impact
> > in practice.
> > 
> > Also add a test case for negative numbers that catches this error
> > condition.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> Looks good to me.
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Thanks, pushed:
   5ba41eb..6351fb0  master -> master
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] config-parser: Catch negative numbers assigned to unsigned config values

[Bryce Harrington]

On Tue, Jul 12, 2016 at 10:58:05AM -0700, Bill Spitzak wrote:
> I tested this and at least for libc on linux it returns 0x1-n, ie
> "-1" is 0x.
> 
> This is actually pretty useful when the unsigned value is bitflags or you
> want to guarantee you typed in the largest number possible. I am not sure
> you really want to disable it, especially if it prevents entry for 1/2 the
> possible numbers.

Sure, but nothing using this routine requires either of those
capabilities.

Bryce

> On Tue, Jul 12, 2016 at 5:17 AM, Eric Engestrom <eric.engest...@imgtec.com>
> wrote:
> 
> > On Mon, Jul 11, 2016 at 05:55:15PM -0700, Bryce Harrington wrote:
> > > strtoul() has a side effect that when given a string representing a
> > > negative number, it returns a negated version as the value, and does not
> > > flag an error.  IOW, strtoul("-42", ) sets val to 42.  This could
> > > potentially result in unintended surprise behaviors, such as if one were
> > > to inadvertantly set a config param to -1 expecting that to disable it,
> > > but with the result of setting the param to 1 instead.
> > >
> > > Catch this by using strtol() and then manually check for the negative
> > > value.  This logic is modelled after Wayland's strtouint().
> > >
> > > Note that this change unfortunately reduces the range of parseable
> > > numbers from [0,UINT_MAX] to [0,INT_MAX].  The current users of
> > > weston_config_section_get_uint() are anticipating numbers far smaller
> > > than either of these limits, so the change is believed to have no impact
> > > in practice.
> > >
> > > Also add a test case for negative numbers that catches this error
> > > condition.
> > >
> > > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> >
> > Looks good to me.
> > Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] rdp: Check for non-digits and errno in strtol call

[Bryce Harrington]

On Tue, Jul 12, 2016 at 08:12:37AM -0700, Yong Bakos wrote:
> On Jul 11, 2016, at 5:02 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > Improve error checking for situations like RDP_FD=42foo, or where the
> > provided number is out of range.
> > 
> > Suggestion by Yong Bakos.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> The condition addresses all failure cases, so this is
> Reviewed-by: Yong Bakos <yba...@humanoriented.com>
> 
> yong

Thanks Yong and Eric, I've pushed with your R-b's:
   7fc000c..5ba41eb  master -> master


> > ---
> > libweston/compositor-rdp.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
> > index 79f0687..53c7124 100644
> > --- a/libweston/compositor-rdp.c
> > +++ b/libweston/compositor-rdp.c
> > @@ -1263,7 +1263,8 @@ rdp_backend_create(struct weston_compositor 
> > *compositor,
> > }
> > 
> > fd = strtoul(fd_str, _tail, 10);
> > -   if (fd_tail == fd_str || rdp_peer_init(freerdp_peer_new(fd), b))
> > +   if (errno != 0 || fd_tail == fd_str || *fd_tail != '\0'
> > +   || rdp_peer_init(freerdp_peer_new(fd), b))
> > goto err_output;
> > }
> > 
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] config-parser: Catch negative numbers assigned to unsigned config values

[Bryce Harrington]

strtoul() has a side effect that when given a string representing a
negative number, it returns a negated version as the value, and does not
flag an error.  IOW, strtoul("-42", ) sets val to 42.  This could
potentially result in unintended surprise behaviors, such as if one were
to inadvertantly set a config param to -1 expecting that to disable it,
but with the result of setting the param to 1 instead.

Catch this by using strtol() and then manually check for the negative
value.  This logic is modelled after Wayland's strtouint().

Note that this change unfortunately reduces the range of parseable
numbers from [0,UINT_MAX] to [0,INT_MAX].  The current users of
weston_config_section_get_uint() are anticipating numbers far smaller
than either of these limits, so the change is believed to have no impact
in practice.

Also add a test case for negative numbers that catches this error
condition.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 shared/config-parser.c | 12 +++-
 tests/config-parser-test.c | 31 +++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index 151ae9c..247e880 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -186,6 +186,7 @@ weston_config_section_get_uint(struct weston_config_section 
*section,
   const char *key,
   uint32_t *value, uint32_t default_value)
 {
+   long int ret;
struct weston_config_entry *entry;
char *end;
 
@@ -197,13 +198,22 @@ weston_config_section_get_uint(struct 
weston_config_section *section,
}
 
errno = 0;
-   *value = strtoul(entry->value, , 0);
+   ret = strtol(entry->value, , 0);
if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
}
 
+   /* check range */
+   if (ret < 0 || ret > INT_MAX) {
+   *value = default_value;
+   errno = ERANGE;
+   return -1;
+   }
+
+   *value = ret;
+
return 0;
 }
 
diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index 735da4e..f88e89b 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -117,6 +117,7 @@ static struct zuc_fixture config_test_t1 = {
"# more comments\n"
"number=5252\n"
"zero=0\n"
+   "negative=-42\n"
"flag=false\n"
"\n"
"[stuff]\n"
@@ -461,6 +462,36 @@ ZUC_TEST_F(config_test_t1, test019, data)
ZUC_ASSERT_EQ(0, errno);
 }
 
+ZUC_TEST_F(config_test_t1, test020, data)
+{
+   int r;
+   int32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_int(section, "negative", , 600);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(-42, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test021, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_uint(section, "negative", , 600);
+
+   ZUC_ASSERT_EQ(-1, r);
+   ZUC_ASSERT_EQ(600, n);
+   ZUC_ASSERT_EQ(ERANGE, errno);
+}
+
 ZUC_TEST_F(config_test_t2, doesnt_parse, data)
 {
struct weston_config *config = data;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] rdp: Check for non-digits and errno in strtol call

[Bryce Harrington]

Improve error checking for situations like RDP_FD=42foo, or where the
provided number is out of range.

Suggestion by Yong Bakos.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 libweston/compositor-rdp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
index 79f0687..53c7124 100644
--- a/libweston/compositor-rdp.c
+++ b/libweston/compositor-rdp.c
@@ -1263,7 +1263,8 @@ rdp_backend_create(struct weston_compositor *compositor,
}
 
fd = strtoul(fd_str, _tail, 10);
-   if (fd_tail == fd_str || rdp_peer_init(freerdp_peer_new(fd), b))
+   if (errno != 0 || fd_tail == fd_str || *fd_tail != '\0'
+   || rdp_peer_init(freerdp_peer_new(fd), b))
goto err_output;
}
 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] scanner: Improve documentation for strtouint()

[Bryce Harrington]

On Fri, Jul 08, 2016 at 07:38:13PM -0700, Yong Bakos wrote:
> On Jul 8, 2016, at 4:51 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > From: Bryce Harrington <br...@bryceharrington.org>
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
> 
> A definite improvement. However, a couple questions inline below.
> 
> 
> > ---
> > src/scanner.c | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/scanner.c b/src/scanner.c
> > index 4708cae..a266da1 100644
> > --- a/src/scanner.c
> > +++ b/src/scanner.c
> > @@ -576,8 +576,17 @@ free_interface(struct interface *interface)
> > free(interface);
> > }
> > 
> > -/* convert string to unsigned integer,
> > - * in the case of error, return -1 */
> > +/** Convert string to unsigned integer
> 
> Should this be a standard comment block instead of a doc-comment?
> Since doxygen will not generate public documentation for scanner.c,
> and since this is an internal function, maybe this should just be a
> regular comment block.

Alright.

> > + *
> > + * Parses a non-negative base-10 number from the given string.  If the
> > + * specified string is blank, contains non-numerical characters, is out
> > + * of range, or results in a negative number, -1 is returned to indicate
> > + * an error..
> 
> Nit: single period here.
> 
> 
> > + *
> > + * This routine does not modify errno, nor sets errno on error.
> > + *
> 
> This feels a little misleading as written, since it does potentially
> modify errno temporarily as an implementation detail. Perhaps "Upon error,
> this routine doe not modify or set errno" would be a little more clear.

Hmm, you're right it's a bit awkward.  I'll use your text for now, we'll
get another go at the phrasing if/when the routine is moved to being a
shared utility routine.

> Other than that, this is
> 
> Reviewed-by: Yong Bakos <yba...@humanoriented.com>

Thanks, pushed with your suggested edits.

   1cda73f..c88ec7e  master -> master


> Regards,
> yong
> 
> 
> > + * \returns -1 on error, or a non-negative integer on success
> > + */
> > static int
> > strtouint(const char *str)
> > {
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] tests: Require base 10 for the string specifying the number of open fd's

[Bryce Harrington]

On Fri, Jul 08, 2016 at 07:39:33PM -0700, Yong Bakos wrote:
> On Jul 8, 2016, at 7:00 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > The third arg to strtol() specifies the base to assume for the number.
> > When 0 is passed, as is currently done in wayland-client.c, hexadecimal
> > and octal numbers are permitted and automatically detected and
> > converted.
> > 
> > exec-fd-leak-checker's single argument is the count of file descriptors
> > it should expect to be open.  We should expect this to be specified only
> > as a decimal number, there's no reason why one would want to use octal
> > or hexadecimal for that.
> > 
> > Suggested by Yong Bakos.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> Reviewed-by: Yong Bakos <yba...@humanoriented.com>
> 
> yong

Thanks, pushed.

Bryce
> 
> > ---
> > tests/exec-fd-leak-checker.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/exec-fd-leak-checker.c b/tests/exec-fd-leak-checker.c
> > index 0c69da3..5f3b395 100644
> > --- a/tests/exec-fd-leak-checker.c
> > +++ b/tests/exec-fd-leak-checker.c
> > @@ -37,7 +37,7 @@ parse_count(const char *str, int *value)
> > long v;
> > 
> > errno = 0;
> > -   v = strtol(str, , 0);
> > +   v = strtol(str, , 10);
> > if ((errno == ERANGE && (v == LONG_MAX || v == LONG_MIN)) ||
> > (errno != 0 && v == 0) ||
> > (end == str) ||
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] rdp: Check for non-numeric value in RDP_FD env var

[Bryce Harrington]

On Fri, Jul 08, 2016 at 08:11:26PM -0700, Yong Bakos wrote:
> On Jul 8, 2016, at 6:54 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > strtoul(nptr, endptr, ...) will set *endptr to nptr in the case of where
> > no digits were read from the string, and return 0.  Running with
> > RDP_FD=foo would thus result in fd=0 being specified to
> > freerdp_peer_new(), which is unlikely to be the user's intent.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> > libweston/compositor-rdp.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
> > index d74dd5e..79f0687 100644
> > --- a/libweston/compositor-rdp.c
> > +++ b/libweston/compositor-rdp.c
> > @@ -1209,6 +1209,7 @@ rdp_backend_create(struct weston_compositor 
> > *compositor,
> > {
> > struct rdp_backend *b;
> > char *fd_str;
> > +   char *fd_tail;
> > int fd;
> > 
> > b = zalloc(sizeof *b);
> > @@ -1261,8 +1262,8 @@ rdp_backend_create(struct weston_compositor 
> > *compositor,
> > goto err_output;
> > }
> > 
> > -   fd = strtoul(fd_str, NULL, 10);
> > -   if (rdp_peer_init(freerdp_peer_new(fd), b))
> > +   fd = strtoul(fd_str, _tail, 10);
> > +   if (fd_tail == fd_str || rdp_peer_init(freerdp_peer_new(fd), b))
> 
> While you're at it, maybe this deserves to be a little more robust. When
> RDP_FD=42foo, fd_tail will != fd_str and fd will be 42. Something akin to
> the idiom:
> 
> if (fd_tail == fd_str || *fd_tail != '\0' || errno == ERANGE || rdp_peer...

Yeah, good points.  I'll catch this in followup, I think this specific
fix is good on its own, but agree there's more cleanup needed in this
call.  For now I'll push as is.

> Regards,
> yong

Bryce

P.S. Apologies...  When I pushed the patches I thought I'd applied the
patches with your R-b from patchwork but evidently they were my local
copies, so missed your R-b's.  :-/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Bryce Harrington]

On Mon, Jul 11, 2016 at 09:28:16AM +1000, Peter Hutterer wrote:
> On Thu, Jul 07, 2016 at 02:24:39PM -0700, Bryce Harrington wrote:
> > On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > > Check errno, which is set of over/underflow, out of range, etc.  Also
> > > check for empty strings (the usages covered in this patch already also
> > > cover the case where there are non-digits present).  Set errno to 0
> > > before making the strto*l call in case of pre-existing errors
> > > (i.e. ENOTTY when running under the testsuite).
> > > 
> > > This follows the error checking style used in Wayland
> > > (c.f. wayland-client.c and scanner.c).
> > > 
> > > In tests, also check errno, and add testcases for parsing '0'.
> > > 
> > > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > 
> > If this approach looks ok to folks, there are other irregular uses of
> > strto*l I'll clean up as well subsequently.
> 
> it'd be best to have a safe_strtol helper function in a weston-util.h
> header and just enforce use of that. because most of the time all we care
> about is "has it parsed and what's the result", you can get that with a
> bool safe_strtol(const char *string, long *result);
> 
> or safe_atoi, or whatever you want to call it.

I've pushed this specific cleanup with Eric's R-b but will follow up
with a proposal for this helper function later.  I think there may be a
few more cleanups to do first.

Bryce
 
> Cheers,
>Peter
> 
> 
> 
> > 
> > > ---
> > >  shared/config-parser.c |  6 --
> > >  tests/config-parser-test.c | 34 ++
> > >  2 files changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > > index 2256469..151ae9c 100644
> > > --- a/shared/config-parser.c
> > > +++ b/shared/config-parser.c
> > > @@ -169,8 +169,9 @@ weston_config_section_get_int(struct 
> > > weston_config_section *section,
> > >   return -1;
> > >   }
> > >  
> > > + errno = 0;
> > >   *value = strtol(entry->value, , 0);
> > > - if (*end != '\0') {
> > > + if (errno != 0 || end == entry->value || *end != '\0') {
> > >   *value = default_value;
> > >   errno = EINVAL;
> > >   return -1;
> > > @@ -195,8 +196,9 @@ weston_config_section_get_uint(struct 
> > > weston_config_section *section,
> > >   return -1;
> > >   }
> > >  
> > > + errno = 0;
> > >   *value = strtoul(entry->value, , 0);
> > > - if (*end != '\0') {
> > > + if (errno != 0 || end == entry->value || *end != '\0') {
> > >   *value = default_value;
> > >   errno = EINVAL;
> > >   return -1;
> > > diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> > > index 5dcafc4..735da4e 100644
> > > --- a/tests/config-parser-test.c
> > > +++ b/tests/config-parser-test.c
> > > @@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
> > >   "[bar]\n"
> > >   "# more comments\n"
> > >   "number=5252\n"
> > > + "zero=0\n"
> > >   "flag=false\n"
> > >   "\n"
> > >   "[stuff]\n"
> > > @@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
> > >  
> > >   ZUC_ASSERT_EQ(0, r);
> > >   ZUC_ASSERT_EQ(5252, n);
> > > + ZUC_ASSERT_EQ(0, errno);
> > >  }
> > >  
> > >  ZUC_TEST_F(config_test_t1, test007, data)
> > > @@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
> > >  
> > >   section = weston_config_get_section(config, "bar", NULL, NULL);
> > >   r = weston_config_section_get_uint(section, "number", , 600);
> > > +
> > >   ZUC_ASSERT_EQ(0, r);
> > >   ZUC_ASSERT_EQ(5252, u);
> > > + ZUC_ASSERT_EQ(0, errno);
> > >  }
> > >  
> > >  ZUC_TEST_F(config_test_t1, test009, data)
> > > @@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
> > >   ZUC_ASSERT_EQ(5, i);
> > >  }
> > >  
> > > +ZUC_TEST_F(config_test_t1, test018, data)
> > > +{
> > > + int r;
> > > + int32_t n;
> > > + struct weston_config_section *section;
> > > + struct weston_config *config = data;
> > > +
> > > + section = w

Re: [PATCH weston] Remove a wrong closing “extern "C"” in shared/xalloc.c

[Bryce Harrington]

On Mon, Jul 11, 2016 at 05:31:59PM +0100, Emmanuel Gil Peyrot wrote:
> Signed-off-by: Emmanuel Gil Peyrot 

Thanks, pushed:
   cbc0537..7fc000c  master -> master

> ---
>  shared/xalloc.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/shared/xalloc.c b/shared/xalloc.c
> index 4bf8a3e..9bf5245 100644
> --- a/shared/xalloc.c
> +++ b/shared/xalloc.c
> @@ -47,8 +47,3 @@ fail_on_null(void *p, size_t size, char *file, int32_t line)
>  
>   return p;
>  }
> -
> -
> -#ifdef  __cplusplus
> -}
> -#endif
> -- 
> 2.9.0
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] Remove a wrong closing “extern "C"” in shared/xalloc.c

[Bryce Harrington]

On Mon, Jul 11, 2016 at 05:31:59PM +0100, Emmanuel Gil Peyrot wrote:
> Signed-off-by: Emmanuel Gil Peyrot <emmanuel.pey...@collabora.com>

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

> ---
>  shared/xalloc.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/shared/xalloc.c b/shared/xalloc.c
> index 4bf8a3e..9bf5245 100644
> --- a/shared/xalloc.c
> +++ b/shared/xalloc.c
> @@ -47,8 +47,3 @@ fail_on_null(void *p, size_t size, char *file, int32_t line)
>  
>   return p;
>  }
> -
> -
> -#ifdef  __cplusplus
> -}
> -#endif
> -- 
> 2.9.0
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Bryce Harrington]

On Mon, Jul 11, 2016 at 09:39:00AM -0700, Bill Spitzak wrote:
> On Sun, Jul 10, 2016 at 4:28 PM, Peter Hutterer <peter.hutte...@who-t.net>
> wrote:
> 
> > On Thu, Jul 07, 2016 at 02:24:39PM -0700, Bryce Harrington wrote:
> > > On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > > > Check errno, which is set of over/underflow, out of range, etc.  Also
> > > > check for empty strings (the usages covered in this patch already also
> > > > cover the case where there are non-digits present).  Set errno to 0
> > > > before making the strto*l call in case of pre-existing errors
> > > > (i.e. ENOTTY when running under the testsuite).
> > > >
> > > > This follows the error checking style used in Wayland
> > > > (c.f. wayland-client.c and scanner.c).
> > > >
> > > > In tests, also check errno, and add testcases for parsing '0'.
> > > >
> > > > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > >
> > > If this approach looks ok to folks, there are other irregular uses of
> > > strto*l I'll clean up as well subsequently.
> >
> > it'd be best to have a safe_strtol helper function in a weston-util.h
> > header and just enforce use of that. because most of the time all we care
> > about is "has it parsed and what's the result", you can get that with a
> > bool safe_strtol(const char *string, long *result);
> >
> > or safe_atoi, or whatever you want to call it.
> >
> 
> The problem is that it is a pain to have to pass a reference to the output
> variable, which discourages use and why everybody keeps calling strtol. A
> function that returns a value is much more usable and easy to read in the
> source code.
> 
> To report the error setting errno will work. Programs can check this after
> return. Returning a bool, or passing a pointer to a bool, really does not
> help, the program can ignore that just as easily as ignoring errno, and you
> have made it harder to call the function and thus discouraged it's use.

Hmm, I get your point, but checking bool error code returns is way more
common than checking for errno, and I would think less likely to be
ignored.  As far as convenience goes... well error checking is annoying
in general, so it's a bit of a wash, but checking errno with a value
return is going to require slightly more typing than checking return
codes with a value passed by reference.  Using errno also places a
requirement for including errno.h on the caller.

But to me, those points are both really minor.  The real benefits to
relying on errno here would be: 1) the errors are more expressive
(e.g. ERANGE or EINVAL rather than just a simple true/false), and 2)
errno can be checked after making a sequence of conversion calls to
check if any of them failed, rather than checking the return code on
each individually.

On the first point, most strtol/strtoul calls in Weston don't seem to
care much why it failed.  For the second point, there is indeed a
sequence of calls in the config parser, where checking errno just once
after all calls were made might make for more concise code, but if there
*is* an error, then for debugging purposes I'm guessing folks would want
to know what line in the config file errored, in which case we'd be back
to needing to check errors after each conversion.

So, considering all these points it seems like the trade-off favors
something more like what Peter suggests - pass the value by reference
and indicate success or failure with a bool return value.  We could also
set errno in addition, in case the caller might want to know more
details about why the failure occurred, but since none of the callers
seem to care to that level of detail maybe it'd be better to just leave
errno unchanged, as Wayland's strtouint() does; a caller that *did* care
about the exact errno can always just do their own direct strtol call.

> Have the function log errors to stderr if you want to make it "impossible"
> to ignore errors.

Hmm, not sure on this.  From the software's point of view, I would argue
that logging messages to stderr and continuing on with processing is
indeed ignoring the errors.  Also, for general purpose utility functions
like this, it seems better to leave decisions about error logging to the
caller, because it'll have more context over what the parsing is trying
to do.  Some things might be best sent to weston_log() rather than
stderr for example.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] option-parser: Require integer option string values to be base-10

[Bryce Harrington]

On Fri, Jul 08, 2016 at 06:47:19PM -0700, Yong Bakos wrote:
> On Jul 8, 2016, at 5:52 PM, Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > The third arg to strtol() specifies the base to assume for the number.
> > When 0 is passed, as is currently done in option-parser.c, hexadecimal
> > and octal numbers are permitted and automatically detected and
> > converted.
> > 
> > In weston and the weston clients and tests using option-parser.c, the
> > options are all things that can be expected to be specified in base 10:
> > widths, heights, counts, scales, font sizes, ports, ttys, connectors,
> > etc.  The subsurfaces client uses two modes, limited to values 0 and 1
> > only.  The zuc testsuite has a --random parameter for specifying a seed,
> > which is the only option where using hexadecimal or octal numbers might
> > conceivably happen.
> > 
> > The benefit of limiting this to base-10 is to eliminate surprises when
> > parsing numbers from the command line.  Also, by making the code
> > consistent with other usages of strtol/strtoul, it may make it possible
> > to factor out the common code in the future.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> I can't think of any drawbacks, other than when some future option has,
> say, a hex color value. But that's "YAGNI" for now, and would be easy to
> change later if need be.

Right, that's my thinking too.  And if we did have a hex color option,
and someone did --color=336633, you'd want to force that to be handled as
hexidecimal and not permit it to accidentally autoconvert as decimal,
anyway.

> Reviewed-by: Yong Bakos <yba...@humanoriented.com>

Thanks,
Bryce

> Regards,
> yong
> 
> 
> > ---
> > shared/option-parser.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/shared/option-parser.c b/shared/option-parser.c
> > index d5fee8e..33355b8 100644
> > --- a/shared/option-parser.c
> > +++ b/shared/option-parser.c
> > @@ -40,10 +40,10 @@ handle_option(const struct weston_option *option, char 
> > *value)
> > 
> > switch (option->type) {
> > case WESTON_OPTION_INTEGER:
> > -   * (int32_t *) option->data = strtol(value, , 0);
> > +   * (int32_t *) option->data = strtol(value, , 10);
> > return *value && !*p;
> > case WESTON_OPTION_UNSIGNED_INTEGER:
> > -   * (uint32_t *) option->data = strtoul(value, , 0);
> > +   * (uint32_t *) option->data = strtoul(value, , 10);
> > return *value && !*p;
> > case WESTON_OPTION_STRING:
> > * (char **) option->data = strdup(value);
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] tests: Require base 10 for the string specifying the number of open fd's

[Bryce Harrington]

The third arg to strtol() specifies the base to assume for the number.
When 0 is passed, as is currently done in wayland-client.c, hexadecimal
and octal numbers are permitted and automatically detected and
converted.

exec-fd-leak-checker's single argument is the count of file descriptors
it should expect to be open.  We should expect this to be specified only
as a decimal number, there's no reason why one would want to use octal
or hexadecimal for that.

Suggested by Yong Bakos.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 tests/exec-fd-leak-checker.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/exec-fd-leak-checker.c b/tests/exec-fd-leak-checker.c
index 0c69da3..5f3b395 100644
--- a/tests/exec-fd-leak-checker.c
+++ b/tests/exec-fd-leak-checker.c
@@ -37,7 +37,7 @@ parse_count(const char *str, int *value)
long v;
 
errno = 0;
-   v = strtol(str, , 0);
+   v = strtol(str, , 10);
if ((errno == ERANGE && (v == LONG_MAX || v == LONG_MIN)) ||
(errno != 0 && v == 0) ||
(end == str) ||
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] rdp: Check for non-numeric value in RDP_FD env var

[Bryce Harrington]

strtoul(nptr, endptr, ...) will set *endptr to nptr in the case of where
no digits were read from the string, and return 0.  Running with
RDP_FD=foo would thus result in fd=0 being specified to
freerdp_peer_new(), which is unlikely to be the user's intent.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 libweston/compositor-rdp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
index d74dd5e..79f0687 100644
--- a/libweston/compositor-rdp.c
+++ b/libweston/compositor-rdp.c
@@ -1209,6 +1209,7 @@ rdp_backend_create(struct weston_compositor *compositor,
 {
struct rdp_backend *b;
char *fd_str;
+   char *fd_tail;
int fd;
 
b = zalloc(sizeof *b);
@@ -1261,8 +1262,8 @@ rdp_backend_create(struct weston_compositor *compositor,
goto err_output;
}
 
-   fd = strtoul(fd_str, NULL, 10);
-   if (rdp_peer_init(freerdp_peer_new(fd), b))
+   fd = strtoul(fd_str, _tail, 10);
+   if (fd_tail == fd_str || rdp_peer_init(freerdp_peer_new(fd), b))
goto err_output;
}
 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] multi-resource: Check for no digits in time description

[Bryce Harrington]

strtoul(nptr, endptr, ...) will set *endptr to nptr in the case of where
no digits were read from the string.  E.g. "foo:bar" should trigger an
error, instead of being read as "0:0" and allowed through.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 clients/multi-resource.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/clients/multi-resource.c b/clients/multi-resource.c
index c283022..2fca7c3 100644
--- a/clients/multi-resource.c
+++ b/clients/multi-resource.c
@@ -434,12 +434,13 @@ create_device(struct display *display, const char 
*time_desc, int type)
 
errno = 0;
start_time = strtoul(time_desc, , 10);
-   if (errno)
+   if (errno || tail == time_desc)
goto error;
 
if (*tail == ':') {
-   end_time = strtoul(tail + 1, , 10);
-   if (errno || *tail != '\0')
+   time_desc = tail + 1;
+   end_time = strtoul(time_desc, , 10);
+   if (errno || tail == time_desc || *tail != '\0')
goto error;
} else if (*tail != '\0') {
goto error;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] option-parser: Require integer option string values to be base-10

[Bryce Harrington]

The third arg to strtol() specifies the base to assume for the number.
When 0 is passed, as is currently done in option-parser.c, hexadecimal
and octal numbers are permitted and automatically detected and
converted.

In weston and the weston clients and tests using option-parser.c, the
options are all things that can be expected to be specified in base 10:
widths, heights, counts, scales, font sizes, ports, ttys, connectors,
etc.  The subsurfaces client uses two modes, limited to values 0 and 1
only.  The zuc testsuite has a --random parameter for specifying a seed,
which is the only option where using hexadecimal or octal numbers might
conceivably happen.

The benefit of limiting this to base-10 is to eliminate surprises when
parsing numbers from the command line.  Also, by making the code
consistent with other usages of strtol/strtoul, it may make it possible
to factor out the common code in the future.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 shared/option-parser.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/shared/option-parser.c b/shared/option-parser.c
index d5fee8e..33355b8 100644
--- a/shared/option-parser.c
+++ b/shared/option-parser.c
@@ -40,10 +40,10 @@ handle_option(const struct weston_option *option, char 
*value)
 
switch (option->type) {
case WESTON_OPTION_INTEGER:
-   * (int32_t *) option->data = strtol(value, , 0);
+   * (int32_t *) option->data = strtol(value, , 10);
return *value && !*p;
case WESTON_OPTION_UNSIGNED_INTEGER:
-   * (uint32_t *) option->data = strtoul(value, , 0);
+   * (uint32_t *) option->data = strtoul(value, , 10);
return *value && !*p;
case WESTON_OPTION_STRING:
* (char **) option->data = strdup(value);
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] scanner: Improve documentation for strtouint()

[Bryce Harrington]

From: Bryce Harrington <br...@bryceharrington.org>

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
---
 src/scanner.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/scanner.c b/src/scanner.c
index 4708cae..a266da1 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -576,8 +576,17 @@ free_interface(struct interface *interface)
free(interface);
 }
 
-/* convert string to unsigned integer,
- * in the case of error, return -1 */
+/** Convert string to unsigned integer
+ *
+ * Parses a non-negative base-10 number from the given string.  If the
+ * specified string is blank, contains non-numerical characters, is out
+ * of range, or results in a negative number, -1 is returned to indicate
+ * an error..
+ *
+ * This routine does not modify errno, nor sets errno on error.
+ *
+ * \returns -1 on error, or a non-negative integer on success
+ */
 static int
 strtouint(const char *str)
 {
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] wayland-client: Require base 10 for WAYLAND_SOCKET, explicitly

[Bryce Harrington]

The third arg to strtol() specifies the base to assume for the number.
When 0 is passed, as is currently done in wayland-client.c, hexadecimal
and octal numbers are permitted and automatically detected and
converted.

I can find no indication that we would ever expect use of hexadecimal or
octal for socket fd's.  So be explicit about what base we're assuming
here and avoid any potential surprises.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 src/wayland-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 03c087a..3d7361e 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -1006,7 +1006,7 @@ wl_display_connect(const char *name)
if (connection) {
int prev_errno = errno;
errno = 0;
-   fd = strtol(connection, , 0);
+   fd = strtol(connection, , 10);
if (errno != 0 || connection == end || *end != '\0')
return NULL;
errno = prev_errno;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Bryce Harrington]

On Fri, Jul 08, 2016 at 11:33:56AM +0100, Eric Engestrom wrote:
> On Fri, Jul 08, 2016 at 10:26:43AM +0100, Eric Engestrom wrote:
> > On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > > + errno = 0;
> > >   *value = strtol(entry->value, , 0);
> > > - if (*end != '\0') {
> > > + if (errno != 0 || end == entry->value || *end != '\0') {
> > 
> > Isn't the empty string case already covered by `*end != '\0'` ?
> 
> No, it's not: I misread that.
> I just re-read the patch, now that I'm a bit more awake, and my r-b
> still stands :)
> 
> > Either way, the duplicate test wouldn't hurt, so:
> > Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Thanks again, pushed.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols] Fix grammar for 'an X*'

[Bryce Harrington]

On Fri, Jul 08, 2016 at 10:20:22AM +0100, Eric Engestrom wrote:
> On Thu, Jul 07, 2016 at 10:56:12AM -0700, Bryce Harrington wrote:
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> This is the last of it, right? ^^
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Thanks Eric and Yong, pushed.
 
> > ---
> >  unstable/input-method/input-method-unstable-v1.xml | 2 +-
> >  unstable/text-input/text-input-unstable-v1.xml | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/unstable/input-method/input-method-unstable-v1.xml 
> > b/unstable/input-method/input-method-unstable-v1.xml
> > index 274eac8..c1b2b59 100644
> > --- a/unstable/input-method/input-method-unstable-v1.xml
> > +++ b/unstable/input-method/input-method-unstable-v1.xml
> > @@ -145,7 +145,7 @@
> > Notify when a key event was sent. Key events should not be used for
> > normal text input operations, which should be done with commit_string,
> > delete_surrounding_text, etc. The key event follows the wl_keyboard key
> > -   event convention. Sym is a XKB keysym, state a wl_keyboard key_state.
> > +   event convention. Sym is an XKB keysym, state a wl_keyboard key_state.
> >
> >
> >
> > diff --git a/unstable/text-input/text-input-unstable-v1.xml 
> > b/unstable/text-input/text-input-unstable-v1.xml
> > index 1d00a52..d37ffcf 100644
> > --- a/unstable/text-input/text-input-unstable-v1.xml
> > +++ b/unstable/text-input/text-input-unstable-v1.xml
> > @@ -328,7 +328,7 @@
> > Notify when a key event was sent. Key events should not be used
> > for normal text input operations, which should be done with
> > commit_string, delete_surrounding_text, etc. The key event follows
> > -   the wl_keyboard key event convention. Sym is a XKB keysym, state a
> > +   the wl_keyboard key event convention. Sym is an XKB keysym, state a
> > wl_keyboard key_state. Modifiers are a mask for effective modifiers
> > (where the modifier indices are set by the modifiers_map event)
> >
> > -- 
> > 1.9.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Bryce Harrington]

On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> Check errno, which is set of over/underflow, out of range, etc.  Also
> check for empty strings (the usages covered in this patch already also
> cover the case where there are non-digits present).  Set errno to 0
> before making the strto*l call in case of pre-existing errors
> (i.e. ENOTTY when running under the testsuite).
> 
> This follows the error checking style used in Wayland
> (c.f. wayland-client.c and scanner.c).
> 
> In tests, also check errno, and add testcases for parsing '0'.
> 
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

If this approach looks ok to folks, there are other irregular uses of
strto*l I'll clean up as well subsequently.

> ---
>  shared/config-parser.c |  6 --
>  tests/config-parser-test.c | 34 ++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/shared/config-parser.c b/shared/config-parser.c
> index 2256469..151ae9c 100644
> --- a/shared/config-parser.c
> +++ b/shared/config-parser.c
> @@ -169,8 +169,9 @@ weston_config_section_get_int(struct 
> weston_config_section *section,
>   return -1;
>   }
>  
> + errno = 0;
>   *value = strtol(entry->value, , 0);
> - if (*end != '\0') {
> + if (errno != 0 || end == entry->value || *end != '\0') {
>   *value = default_value;
>   errno = EINVAL;
>   return -1;
> @@ -195,8 +196,9 @@ weston_config_section_get_uint(struct 
> weston_config_section *section,
>   return -1;
>   }
>  
> + errno = 0;
>   *value = strtoul(entry->value, , 0);
> - if (*end != '\0') {
> + if (errno != 0 || end == entry->value || *end != '\0') {
>   *value = default_value;
>   errno = EINVAL;
>   return -1;
> diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> index 5dcafc4..735da4e 100644
> --- a/tests/config-parser-test.c
> +++ b/tests/config-parser-test.c
> @@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
>   "[bar]\n"
>   "# more comments\n"
>   "number=5252\n"
> + "zero=0\n"
>   "flag=false\n"
>   "\n"
>   "[stuff]\n"
> @@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
>  
>   ZUC_ASSERT_EQ(0, r);
>   ZUC_ASSERT_EQ(5252, n);
> + ZUC_ASSERT_EQ(0, errno);
>  }
>  
>  ZUC_TEST_F(config_test_t1, test007, data)
> @@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
>  
>   section = weston_config_get_section(config, "bar", NULL, NULL);
>   r = weston_config_section_get_uint(section, "number", , 600);
> +
>   ZUC_ASSERT_EQ(0, r);
>   ZUC_ASSERT_EQ(5252, u);
> + ZUC_ASSERT_EQ(0, errno);
>  }
>  
>  ZUC_TEST_F(config_test_t1, test009, data)
> @@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
>   ZUC_ASSERT_EQ(5, i);
>  }
>  
> +ZUC_TEST_F(config_test_t1, test018, data)
> +{
> + int r;
> + int32_t n;
> + struct weston_config_section *section;
> + struct weston_config *config = data;
> +
> + section = weston_config_get_section(config, "bar", NULL, NULL);
> + r = weston_config_section_get_int(section, "zero", , 600);
> +
> + ZUC_ASSERT_EQ(0, r);
> + ZUC_ASSERT_EQ(0, n);
> + ZUC_ASSERT_EQ(0, errno);
> +}
> +
> +ZUC_TEST_F(config_test_t1, test019, data)
> +{
> + int r;
> + uint32_t n;
> + struct weston_config_section *section;
> + struct weston_config *config = data;
> +
> + section = weston_config_get_section(config, "bar", NULL, NULL);
> + r = weston_config_section_get_uint(section, "zero", , 600);
> +
> + ZUC_ASSERT_EQ(0, r);
> + ZUC_ASSERT_EQ(0, n);
> + ZUC_ASSERT_EQ(0, errno);
> +}
> +
>  ZUC_TEST_F(config_test_t2, doesnt_parse, data)
>  {
>   struct weston_config *config = data;
> -- 
> 1.9.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Bryce Harrington]

Check errno, which is set of over/underflow, out of range, etc.  Also
check for empty strings (the usages covered in this patch already also
cover the case where there are non-digits present).  Set errno to 0
before making the strto*l call in case of pre-existing errors
(i.e. ENOTTY when running under the testsuite).

This follows the error checking style used in Wayland
(c.f. wayland-client.c and scanner.c).

In tests, also check errno, and add testcases for parsing '0'.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 shared/config-parser.c |  6 --
 tests/config-parser-test.c | 34 ++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index 2256469..151ae9c 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -169,8 +169,9 @@ weston_config_section_get_int(struct weston_config_section 
*section,
return -1;
}
 
+   errno = 0;
*value = strtol(entry->value, , 0);
-   if (*end != '\0') {
+   if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
@@ -195,8 +196,9 @@ weston_config_section_get_uint(struct weston_config_section 
*section,
return -1;
}
 
+   errno = 0;
*value = strtoul(entry->value, , 0);
-   if (*end != '\0') {
+   if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index 5dcafc4..735da4e 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
"[bar]\n"
"# more comments\n"
"number=5252\n"
+   "zero=0\n"
"flag=false\n"
"\n"
"[stuff]\n"
@@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
 
ZUC_ASSERT_EQ(0, r);
ZUC_ASSERT_EQ(5252, n);
+   ZUC_ASSERT_EQ(0, errno);
 }
 
 ZUC_TEST_F(config_test_t1, test007, data)
@@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
 
section = weston_config_get_section(config, "bar", NULL, NULL);
r = weston_config_section_get_uint(section, "number", , 600);
+
ZUC_ASSERT_EQ(0, r);
ZUC_ASSERT_EQ(5252, u);
+   ZUC_ASSERT_EQ(0, errno);
 }
 
 ZUC_TEST_F(config_test_t1, test009, data)
@@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
ZUC_ASSERT_EQ(5, i);
 }
 
+ZUC_TEST_F(config_test_t1, test018, data)
+{
+   int r;
+   int32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_int(section, "zero", , 600);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test019, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_uint(section, "zero", , 600);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
 ZUC_TEST_F(config_test_t2, doesnt_parse, data)
 {
struct weston_config *config = data;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland-protocols] Fix grammar for 'an X*'

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 unstable/input-method/input-method-unstable-v1.xml | 2 +-
 unstable/text-input/text-input-unstable-v1.xml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/unstable/input-method/input-method-unstable-v1.xml 
b/unstable/input-method/input-method-unstable-v1.xml
index 274eac8..c1b2b59 100644
--- a/unstable/input-method/input-method-unstable-v1.xml
+++ b/unstable/input-method/input-method-unstable-v1.xml
@@ -145,7 +145,7 @@
Notify when a key event was sent. Key events should not be used for
normal text input operations, which should be done with commit_string,
delete_surrounding_text, etc. The key event follows the wl_keyboard key
-   event convention. Sym is a XKB keysym, state a wl_keyboard key_state.
+   event convention. Sym is an XKB keysym, state a wl_keyboard key_state.
   
   
   
diff --git a/unstable/text-input/text-input-unstable-v1.xml 
b/unstable/text-input/text-input-unstable-v1.xml
index 1d00a52..d37ffcf 100644
--- a/unstable/text-input/text-input-unstable-v1.xml
+++ b/unstable/text-input/text-input-unstable-v1.xml
@@ -328,7 +328,7 @@
Notify when a key event was sent. Key events should not be used
for normal text input operations, which should be done with
commit_string, delete_surrounding_text, etc. The key event follows
-   the wl_keyboard key event convention. Sym is a XKB keysym, state a
+   the wl_keyboard key event convention. Sym is an XKB keysym, state a
wl_keyboard key_state. Modifiers are a mask for effective modifiers
(where the modifier indices are set by the modifiers_map event)
   
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] xwayland: Grammar fixes

[Bryce Harrington]

On Thu, Jul 07, 2016 at 01:16:38PM +0100, Eric Engestrom wrote:
> On Wed, Jul 06, 2016 at 03:25:19PM -0700, Bryce Harrington wrote:
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> >  xwayland/xwayland-api.h | 16 
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xwayland/xwayland-api.h b/xwayland/xwayland-api.h
> > index 4be87e5..9a16147 100644
> > --- a/xwayland/xwayland-api.h
> > +++ b/xwayland/xwayland-api.h
> > @@ -63,8 +63,8 @@ struct weston_xwayland_api {
> >  
> > /** Listen for X connections.
> >  *
> > -* This function tells the Xwayland module to start create a X socket
> > -* and to listen for client connections. When one such connection is
> > +* This function tells the Xwayland module to begin creating a X socket
> > +* and start listening for client connections. When one such connection 
> > is
> >  * detected the given \a spawn_func callback will be called to start
> >  * the Xwayland process.
> >  *
> 
> "an X socket", maybe?

Sure, I'll change that one too.  I wondered about that myself.
 
> Either way, your changes are good:
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Thanks very much for the R-b's Eric.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] dmabuf: Fix grammar in a comment

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 libweston/linux-dmabuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libweston/linux-dmabuf.c b/libweston/linux-dmabuf.c
index 78e77a2..a91ae85 100644
--- a/libweston/linux-dmabuf.c
+++ b/libweston/linux-dmabuf.c
@@ -471,7 +471,7 @@ linux_dmabuf_setup(struct weston_compositor *compositor)
  * In any case, the options are to either composite garbage or nothing,
  * or disconnect the client. This is a helper function for the latter.
  *
- * The error is sent as a INVALID_OBJECT error on the client's wl_display.
+ * The error is sent as an INVALID_OBJECT error on the client's wl_display.
  *
  * \param buffer The linux_dmabuf_buffer that is unusable.
  * \param msg A custom error message attached to the protocol error.
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] xwayland: Grammar fixes

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 xwayland/xwayland-api.h | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xwayland/xwayland-api.h b/xwayland/xwayland-api.h
index 4be87e5..9a16147 100644
--- a/xwayland/xwayland-api.h
+++ b/xwayland/xwayland-api.h
@@ -45,16 +45,16 @@ typedef pid_t
 
 /** The libweston Xwayland API
  *
- * This API allows to control the Xwayland libweston module.
+ * This API allows control of the Xwayland libweston module.
  * The module must be loaded at runtime with \a 
weston_compositor_load_xwayland,
  * after which the API can be retrieved by using \a weston_xwayland_get_api.
  */
 struct weston_xwayland_api {
/** Retrieve the Xwayland context object.
 *
-* Note that this function does not create a new object, but returns
-* always the same object per compositor instance.
-* This function cannot fail, if this API object is valid.
+* Note that this function does not create a new object, but always
+* returns the same object per compositor instance.
+* This function cannot fail while this API object is valid.
 *
 * \param compositor The compositor instance.
 */
@@ -63,8 +63,8 @@ struct weston_xwayland_api {
 
/** Listen for X connections.
 *
-* This function tells the Xwayland module to start create a X socket
-* and to listen for client connections. When one such connection is
+* This function tells the Xwayland module to begin creating a X socket
+* and start listening for client connections. When one such connection 
is
 * detected the given \a spawn_func callback will be called to start
 * the Xwayland process.
 *
@@ -79,7 +79,7 @@ struct weston_xwayland_api {
(*listen)(struct weston_xwayland *xwayland, void *user_data,
  weston_xwayland_spawn_xserver_func_t spawn_func);
 
-   /** Notify the Xwayland module the Xwayland server is loaded.
+   /** Notify the Xwayland module that the Xwayland server is loaded.
 *
 * After the Xwayland server process has been spawned it will notify
 * the parent that is has finished the initialization by sending a
@@ -96,7 +96,7 @@ struct weston_xwayland_api {
(*xserver_loaded)(struct weston_xwayland *xwayland,
  struct wl_client *client, int wm_fd);
 
-   /** Notify the Xwayland module the Xwayland server has exited.
+   /** Notify the Xwayland module that the Xwayland server has exited.
 *
 * Whenever the Xwayland server process quits this function should be
 * called.
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] xwayland: Cleanup error message on spawn failure Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 compositor/xwayland.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compositor/xwayland.c b/compositor/xwayland.c
index 9fa7600..9881cd9 100644
--- a/compositor/xwayland.c
+++ b/compositor/xwayland.c
@@ -146,7 +146,7 @@ spawn_xserver(void *user_data, const char *display, int 
abstract_fd, int unix_fd
break;
 
case -1:
-   weston_log( "failed to fork\n");
+   weston_log("Failed to fork to spawn xserver process\n");
break;
}
 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] xwayland: Include missing config.h

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 compositor/xwayland.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/compositor/xwayland.c b/compositor/xwayland.c
index 26eda20..9fa7600 100644
--- a/compositor/xwayland.c
+++ b/compositor/xwayland.c
@@ -24,6 +24,8 @@
  * SOFTWARE.
  */
 
+#include "config.h"
+
 #include 
 #include 
 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] terminal: Fix segmentation fault when processing DCH ANSI escape code

[Bryce Harrington]

On Sat, Jul 02, 2016 at 12:35:21AM +, aa...@correspondwith.me wrote:
> Fix a segfault where terminal parses a DCH escape code and the cursor is at 
> the end of the line, causing terminal_shift_line to memmove memory it doesn't 
> own.
> ---
>  clients/terminal.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/clients/terminal.c b/clients/terminal.c
> index 6257cb7..7a0da29 100644
> --- a/clients/terminal.c
> +++ b/clients/terminal.c
> @@ -724,13 +724,16 @@ terminal_shift_line(struct terminal *terminal, int d)
>  
>   if (d < 0) {
>   d = 0 - d;
> - memmove([terminal->column],
> - [terminal->column + d],
> - (terminal->width - terminal->column - d) * sizeof(union 
> utf8_char));
> - memmove(_row[terminal->column], _row[terminal->column 
> + d],
> - (terminal->width - terminal->column - d) * 
> sizeof(struct attr));
> - memset([terminal->width - d], 0, d * sizeof(union 
> utf8_char));
> - attr_init(_row[terminal->width - d], terminal->curr_attr, 
> d);
> +
> + if(terminal->width - terminal->column) {

style nit - space needed between the if and (

I might go with a != rather than a - since really all you care about is
whether the two parameters differ.  That would be keeping stylistically
consistent with the surrounding code in terminal.c which appears to
prefer boolean operations in the conditionals.

Although I notice we're checking d, that is already a calculation of
terminal->width and terminal->column, which makes me wonder if the
conditionals ought to be tweaked further rather than adding another if
statement.

> + memmove([terminal->column],
> + [terminal->column + d],
> + (terminal->width - terminal->column - d) * 
> sizeof(union utf8_char));
> + memmove(_row[terminal->column], 
> _row[terminal->column + d],
> + (terminal->width - terminal->column - d) * 
> sizeof(struct attr));
> + memset([terminal->width - d], 0, d * sizeof(union 
> utf8_char));
> + attr_init(_row[terminal->width - d], 
> terminal->curr_attr, d);
> + }

This does though leave the question of what the behavior should be if
the column number is equal to the width.  This just skips the lineshift,
right?  For DCH that's probably the right behavior, but can other codes
trigger this situation, and if so would they require different handling
here?

Thanks for reporting this; if the above refactoring doesn't sound like
work you want to do, that's fine - let me know and I'll look further
into it myself later on.

Bryce

>   } else {
>   memmove([terminal->column + d], [terminal->column],
>   (terminal->width - terminal->column - d) * sizeof(union 
> utf8_char));
> -- 
> 2.9.0
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Free software license of weston1.9

[Bryce Harrington]

On Thu, Jun 30, 2016 at 11:59:58AM +0800, Jonas Ådahl wrote:
> On Thu, Jun 30, 2016 at 04:43:29AM +0200, Armin Krezović wrote:
> > On 30.06.2016 04:19, 袁嘉伟 wrote:
> > > Hi, All:
> > > 
> > > 
> > > Who does know that the free software license of weston 1.9 is GPL or 
> > > LGPL?
> > > Thanks.
> > > 
> > > Best regards,
> > > Anthenony
> > > 
> > > 
> > > 
> > > ___
> > > wayland-devel mailing list
> > > wayland-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > > 
> > 
> > It's neither of those. It's "MIT Expat" license
> > 
> > https://cgit.freedesktop.org/wayland/weston/tree/COPYING
> > 
> 
> While the source code is distributed under the MIT Expat license, be
> aware that the content of the data/ folder (a set of PNG files) have the
> Creative Commons Attribution-Share Alike 3.0 license.

No, only some cursors and one of the terminal icons are CC-SA-3.0, the
rest are either explicitly MIT Expat or a close variant; the SVG icons
are dual licensed so can be used flexibly either way.

Also despite the license diversity of the data dir, none of the licenses
impose restrictions on distribution or work combination like the GPL
does, just for modification+redistribution of the images themselves, so
I would be surprised any of this would create any sort of problem for
anyone trying to use weston for any purpose.

Bryce

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-web 1/1] html: Remove trailing whitespace

[Bryce Harrington]

On Tue, Jun 28, 2016 at 08:52:33AM -0500, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> The html files contain trailing whitespace, which is an annoyance when making
> a change: editors configured to remove trailing whitespace upon save end up
> cluttering the diff beyond the intent of the author.
> 
> Remove trailing whitespace from all html files, so the annoyance described
> above no longer occurs.
> 
> Signed-off-by: Yong Bakos <yba...@humanoriented.com>

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>
> ---
>  architecture.html | 12 +-
>  building.html | 12 +-
>  efl.html  | 10 
>  extras.html   | 14 +--
>  faq.html  | 10 
>  index.html| 12 +-
>  mesa-configure.html   | 18 +++---
>  qt5.html  | 10 
>  releases-archive.html | 14 +--
>  releases.html | 10 
>  reviewing.html| 66 
> +--
>  screenshots.html  | 12 +-
>  toolkits.html | 10 
>  ubuntu12.04.html  |  2 +-
>  xserver.html  | 12 +-
>  15 files changed, 112 insertions(+), 112 deletions(-)
> 
> diff --git a/architecture.html b/architecture.html
> index 0552e90..1acd957 100644
> --- a/architecture.html
> +++ b/architecture.html
> @@ -1,11 +1,11 @@
> - "http://www.w3.org/TR/html4/strict.dtd;>
> -
> + "http://www.w3.org/TR/html4/strict.dtd;>
> +
> 
> -
> -
> +
> +
>  
>  
> -Wayland
> +Wayland
>  
> 
>  
> @@ -264,4 +264,4 @@ point where the change it affects appears on screen.
>  
> 
>  
> -
> +
> diff --git a/building.html b/building.html
> index 1ff9ea2..1fe1245 100644
> --- a/building.html
> +++ b/building.html
> @@ -1,8 +1,8 @@
> - "http://www.w3.org/TR/html4/strict.dtd;>
> -
> + "http://www.w3.org/TR/html4/strict.dtd;>
> +
> 
> -
> -
> +
> +
>  
>  Building Weston
>  
> @@ -21,7 +21,7 @@ particular distros, such as the
>  
> 
>  
> -If you want an even current build, you can use
> +If you want an even current build, you can use
>  our  href="https://github.com/bryceharrington/wayland-build-tools/;>wayland-build-tools
>  scripts to check out and build all the git trees for your local system.
>  
> @@ -360,7 +360,7 @@ testing specific features in the wayland protocol: 
>'weston-view' does the same for pdf files
>'weston-resizor' demonstrates smooth window resizing
>  (use up and down keys)
> -  'weston-eventdemo' reports libtoytoolkit's
> +  'weston-eventdemo' reports libtoytoolkit's
>  events to console (see weston-eventdemo --help)
>  
> 
> diff --git a/efl.html b/efl.html
> index 2073098..024109a 100644
> --- a/efl.html
> +++ b/efl.html
> @@ -1,11 +1,11 @@
> - "http://www.w3.org/TR/html4/strict.dtd;>
> -
> + "http://www.w3.org/TR/html4/strict.dtd;>
> +
> 
> -  
> -
> +  
> +
>  
>  
> -Wayland
> +Wayland
>
> 
>
> diff --git a/extras.html b/extras.html
> index eba7133..14dfc79 100644
> --- a/extras.html
> +++ b/extras.html
> @@ -1,10 +1,10 @@
> - "http://www.w3.org/TR/html4/strict.dtd;>
> -
> + "http://www.w3.org/TR/html4/strict.dtd;>
> +
> 
> -
> -
> +
> +
>  
> -Extras that work with Wayland
> +Extras that work with Wayland
>  
> 
>  
> @@ -41,7 +41,7 @@ Wayland terminal emulator based on kmscon by the same 
> author.
> 
> 
>  
> -Java bindings by Jason Ekstrand's
> +Java bindings by Jason Ekstrand's
>  
> 
>  
> @@ -77,7 +77,7 @@ glmark2
>  ./waf configure --with-flavors=wayland-glesv2,wayland-gl --prefix=$WLD
>  ./waf
>  ./waf install
> -$WLD/bin/glmark2-es2-wayland
> +$WLD/bin/glmark2-es2-wayland
>  
> 
>  Debugging tools
> diff --git a/faq.html b/faq.html
> index edcd6df..9fd8731 100644
> --- a/faq.html
> +++ b/faq.html
> @@ -1,11 +1,11 @@
> - "http://www.w3.org/TR/html4/strict.dtd;>
> -
> + "http://www.w3.org/TR/html4/strict.dtd;>
> +
> 
> -
> -
> +
> +
>  
>  
> -Wayland
> +Wayland
>  
> 
>  
> diff --git a/index.html b/index.html
> index 75887e4..bf8da81 100644
> --- a/index.html
> +++ b/index.html
> @@ -1,10 +1,10 @@
> - "http://www.w3.org/TR/html4/strict.dtd;>
> -
> + "http://www.w3.org/TR/html4/strict.dtd;>
> +
> 
> -
> -
> +
> +
>  
> -Wayland
> +Wayland
>  
> 
>  
> @@ -60,4 +60,4 @@ emb

[PATCH weston 1/2] input: Rename weston_surface_activate to weston_seat_set_keyboard_focus

[Bryce Harrington]

The name suggests that it activates surfaces, but the code says it
rather just assigns keyboard focus.  Rename it for clarity, and so the
original function name could be used for something more appropriate
later.  Switch order of parameters since keyboard focus is a property of
the seat.  Update all callers as appropriate.

Change was asked for by pq, May 26, 2016:

 "This should be called weston_seat_set_keyboard_focus(seat, surface).
 Keyboard focus is a property of the seat."

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 desktop-shell/shell.c   | 2 +-
 fullscreen-shell/fullscreen-shell.c | 8 
 ivi-shell/ivi-shell.c   | 2 +-
 libweston/compositor.h  | 4 ++--
 libweston/input.c   | 4 ++--
 tests/weston-test.c | 4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index 825c6c3..c125d55 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -5168,7 +5168,7 @@ activate(struct desktop_shell *shell, struct 
weston_surface *es,
 * Leave fullscreen surfaces on unrelated outputs alone. */
lower_fullscreen_layer(shell, shsurf->output);
 
-   weston_surface_activate(es, seat);
+   weston_seat_set_keyboard_focus(seat, es);
 
state = ensure_focus_state(shell, seat);
if (state == NULL)
diff --git a/fullscreen-shell/fullscreen-shell.c 
b/fullscreen-shell/fullscreen-shell.c
index ecaebfa..1489258 100644
--- a/fullscreen-shell/fullscreen-shell.c
+++ b/fullscreen-shell/fullscreen-shell.c
@@ -89,7 +89,7 @@ pointer_focus_changed(struct wl_listener *listener, void 
*data)
struct weston_pointer *pointer = data;
 
if (pointer->focus && pointer->focus->surface->resource)
-   weston_surface_activate(pointer->focus->surface, pointer->seat);
+   weston_seat_set_keyboard_focus(pointer->seat, 
pointer->focus->surface);
 }
 
 static void
@@ -118,7 +118,7 @@ seat_caps_changed(struct wl_listener *l, void *data)
if (keyboard && keyboard->focus != NULL) {
wl_list_for_each(fsout, >shell->output_list, link) {
if (fsout->surface) {
-   weston_surface_activate(fsout->surface, seat);
+   weston_seat_set_keyboard_focus(seat, 
fsout->surface);
return;
}
}
@@ -704,7 +704,7 @@ fullscreen_shell_present_surface(struct wl_client *client,
weston_seat_get_keyboard(seat);
 
if (keyboard && !keyboard->focus)
-   weston_surface_activate(surface, seat);
+   weston_seat_set_keyboard_focus(seat, surface);
}
}
 }
@@ -755,7 +755,7 @@ fullscreen_shell_present_surface_for_mode(struct wl_client 
*client,
weston_seat_get_keyboard(seat);
 
if (keyboard && !keyboard->focus)
-   weston_surface_activate(surface, seat);
+   weston_seat_set_keyboard_focus(seat, surface);
}
 }
 
diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index 8bf3e80..090ee4d 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -445,7 +445,7 @@ activate_binding(struct weston_seat *seat,
if (get_ivi_shell_surface(main_surface) == NULL)
return;
 
-   weston_surface_activate(focus, seat);
+   weston_seat_set_keyboard_focus(seat, focus);
 }
 
 static void
diff --git a/libweston/compositor.h b/libweston/compositor.h
index 9e5155c..5701a05 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -1167,8 +1167,8 @@ int
 weston_spring_done(struct weston_spring *spring);
 
 void
-weston_surface_activate(struct weston_surface *surface,
-   struct weston_seat *seat);
+weston_seat_set_keyboard_focus(struct weston_seat *seat,
+  struct weston_surface *surface);
 void
 notify_motion(struct weston_seat *seat, uint32_t time,
  struct weston_pointer_motion_event *event);
diff --git a/libweston/input.c b/libweston/input.c
index 08378d1..e8c060e 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1297,8 +1297,8 @@ notify_motion_absolute(struct weston_seat *seat,
 }
 
 WL_EXPORT void
-weston_surface_activate(struct weston_surface *surface,
-   struct weston_seat *seat)
+weston_seat_set_keyboard_focus(struct weston_seat *seat,
+  struct weston_surface *surface)
 {
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
diff --git a/tests/weston-test.c b/tests/weston-test.c
index 1793744..27d045d 100644
--- a/t

[PATCH weston 2/2] input: Move weston_seat_set_keyboard_focus and document

[Bryce Harrington]

Place it with the other weston_seat functions.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 libweston/compositor.h |  7 ---
 libweston/input.c  | 34 +++---
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/libweston/compositor.h b/libweston/compositor.h
index 5701a05..49ef063 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -1167,9 +1167,6 @@ int
 weston_spring_done(struct weston_spring *spring);
 
 void
-weston_seat_set_keyboard_focus(struct weston_seat *seat,
-  struct weston_surface *surface);
-void
 notify_motion(struct weston_seat *seat, uint32_t time,
  struct weston_pointer_motion_event *event);
 void
@@ -1717,6 +1714,10 @@ weston_seat_get_pointer(struct weston_seat *seat);
 struct weston_touch *
 weston_seat_get_touch(struct weston_seat *seat);
 
+void
+weston_seat_set_keyboard_focus(struct weston_seat *seat,
+  struct weston_surface *surface);
+
 #ifdef  __cplusplus
 }
 #endif
diff --git a/libweston/input.c b/libweston/input.c
index e8c060e..8f46698 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1297,21 +1297,6 @@ notify_motion_absolute(struct weston_seat *seat,
 }
 
 WL_EXPORT void
-weston_seat_set_keyboard_focus(struct weston_seat *seat,
-  struct weston_surface *surface)
-{
-   struct weston_compositor *compositor = seat->compositor;
-   struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
-
-   if (keyboard) {
-   weston_keyboard_set_focus(keyboard, surface);
-   wl_data_device_set_keyboard_focus(seat);
-   }
-
-   wl_signal_emit(>activate_signal, surface);
-}
-
-WL_EXPORT void
 notify_button(struct weston_seat *seat, uint32_t time, int32_t button,
  enum wl_pointer_button_state state)
 {
@@ -2763,3 +2748,22 @@ weston_seat_get_touch(struct weston_seat *seat)
 
return NULL;
 }
+
+/** Sets the keyboard focus to the given surface
+ *
+ * \param seat The seat to query
+ */
+WL_EXPORT void
+weston_seat_set_keyboard_focus(struct weston_seat *seat,
+  struct weston_surface *surface)
+{
+   struct weston_compositor *compositor = seat->compositor;
+   struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
+
+   if (keyboard) {
+   weston_keyboard_set_focus(keyboard, surface);
+   wl_data_device_set_keyboard_focus(seat);
+   }
+
+   wl_signal_emit(>activate_signal, surface);
+}
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface

[Bryce Harrington]

On Tue, Jun 28, 2016 at 12:15:49PM +0300, Pekka Paalanen wrote:
> On Thu, 23 Jun 2016 17:32:50 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > On Thu, May 26, 2016 at 06:01:27PM +0300, Pekka Paalanen wrote:
> > > On Thu,  7 Apr 2016 16:44:17 -0700
> > > Bryce Harrington <br...@osg.samsung.com> wrote:
> > >   
> > > > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>  
> > > 
> > > Hi,
> > > 
> > > the commit message could use an explanation of what the 'bool active'
> > > will actually be useful for. What do we need the "is in active use" for?
> > > The comment in the code does not really tell me either. This ties in
> > > with my comments on patch 3.
> > > 
> > > The patch subject is misleading. There is no inhibition state tracking
> > > at all in this patch. Instead, this patch is about tracking surface
> > > activeness as defined by the shells.
> > >   
> > > > ---
> > > > v3: Rename inhibit_screensaving to inhibit_idling
> > > > 
> > > >  desktop-shell/shell.c   |  4 +++-
> > > >  fullscreen-shell/fullscreen-shell.c | 25 +++---
> > > >  ivi-shell/ivi-shell.c   |  4 +++-
> > > >  src/compositor.c| 42 
> > > > +
> > > >  src/compositor.h| 27 +---
> > > >  src/input.c | 15 -
> > > >  tests/weston-test.c |  8 +--
> > > >  7 files changed, 96 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > > > index 780902d..6e49076 100644
> > > > --- a/desktop-shell/shell.c
> > > > +++ b/desktop-shell/shell.c
> > > > @@ -5055,7 +5055,9 @@ activate(struct desktop_shell *shell, struct 
> > > > weston_surface *es,
> > > >  * Leave fullscreen surfaces on unrelated outputs alone. */
> > > > lower_fullscreen_layer(shell, shsurf->output);
> > > >  
> > > > -   weston_surface_activate(es, seat);
> > > > +   weston_surface_assign_keyboard(es, seat);
> > > > +   if (es != NULL)  
> > > 
> > > 'es' cannot be NULL.
> > >   
> > > > +   weston_surface_activate(es);
> > > >  
> > > > state = ensure_focus_state(shell, seat);
> > > > if (state == NULL)
> > > > diff --git a/fullscreen-shell/fullscreen-shell.c 
> > > > b/fullscreen-shell/fullscreen-shell.c
> > > > index abc4e84..e1f8a63 100644
> > > > --- a/fullscreen-shell/fullscreen-shell.c
> > > > +++ b/fullscreen-shell/fullscreen-shell.c
> > > > @@ -88,8 +88,11 @@ pointer_focus_changed(struct wl_listener *listener, 
> > > > void *data)
> > > >  {
> > > > struct weston_pointer *pointer = data;
> > > >  
> > > > -   if (pointer->focus && pointer->focus->surface->resource)
> > > > -   weston_surface_activate(pointer->focus->surface, 
> > > > pointer->seat);
> > > > +   if (pointer->focus && pointer->focus->surface->resource) {
> > > > +   weston_surface_assign_keyboard(pointer->focus->surface, 
> > > > pointer->seat);
> > > > +   if (pointer->focus->surface != NULL)  
> > > 
> > > pointer->focus->surface cannot be NULL.
> > >   
> > > > +   
> > > > weston_surface_activate(pointer->focus->surface);
> > > > +   }
> > > >  }
> > > >  
> > > >  static void
> > > > @@ -118,7 +121,9 @@ seat_caps_changed(struct wl_listener *l, void *data)
> > > > if (keyboard && keyboard->focus != NULL) {
> > > > wl_list_for_each(fsout, >shell->output_list, 
> > > > link) {
> > > > if (fsout->surface) {
> > > > -   weston_surface_activate(fsout->surface, 
> > > > seat);
> > > > +   
> > > > weston_surface_assign_keyboard(fsout->surface, seat);
> > > > +   if (fsout->surface != NULL)  
> > 

Re: [PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface

[Bryce Harrington]

On Thu, May 26, 2016 at 06:01:27PM +0300, Pekka Paalanen wrote:
> On Thu,  7 Apr 2016 16:44:17 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> Hi,
> 
> the commit message could use an explanation of what the 'bool active'
> will actually be useful for. What do we need the "is in active use" for?
> The comment in the code does not really tell me either. This ties in
> with my comments on patch 3.
> 
> The patch subject is misleading. There is no inhibition state tracking
> at all in this patch. Instead, this patch is about tracking surface
> activeness as defined by the shells.
> 
> > ---
> > v3: Rename inhibit_screensaving to inhibit_idling
> > 
> >  desktop-shell/shell.c   |  4 +++-
> >  fullscreen-shell/fullscreen-shell.c | 25 +++---
> >  ivi-shell/ivi-shell.c   |  4 +++-
> >  src/compositor.c| 42 
> > +
> >  src/compositor.h| 27 +---
> >  src/input.c | 15 -
> >  tests/weston-test.c |  8 +--
> >  7 files changed, 96 insertions(+), 29 deletions(-)
> > 
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index 780902d..6e49076 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -5055,7 +5055,9 @@ activate(struct desktop_shell *shell, struct 
> > weston_surface *es,
> >  * Leave fullscreen surfaces on unrelated outputs alone. */
> > lower_fullscreen_layer(shell, shsurf->output);
> >  
> > -   weston_surface_activate(es, seat);
> > +   weston_surface_assign_keyboard(es, seat);
> > +   if (es != NULL)
> 
> 'es' cannot be NULL.
> 
> > +   weston_surface_activate(es);
> >  
> > state = ensure_focus_state(shell, seat);
> > if (state == NULL)
> > diff --git a/fullscreen-shell/fullscreen-shell.c 
> > b/fullscreen-shell/fullscreen-shell.c
> > index abc4e84..e1f8a63 100644
> > --- a/fullscreen-shell/fullscreen-shell.c
> > +++ b/fullscreen-shell/fullscreen-shell.c
> > @@ -88,8 +88,11 @@ pointer_focus_changed(struct wl_listener *listener, void 
> > *data)
> >  {
> > struct weston_pointer *pointer = data;
> >  
> > -   if (pointer->focus && pointer->focus->surface->resource)
> > -   weston_surface_activate(pointer->focus->surface, pointer->seat);
> > +   if (pointer->focus && pointer->focus->surface->resource) {
> > +   weston_surface_assign_keyboard(pointer->focus->surface, 
> > pointer->seat);
> > +   if (pointer->focus->surface != NULL)
> 
> pointer->focus->surface cannot be NULL.
> 
> > +   weston_surface_activate(pointer->focus->surface);
> > +   }
> >  }
> >  
> >  static void
> > @@ -118,7 +121,9 @@ seat_caps_changed(struct wl_listener *l, void *data)
> > if (keyboard && keyboard->focus != NULL) {
> > wl_list_for_each(fsout, >shell->output_list, link) {
> > if (fsout->surface) {
> > -   weston_surface_activate(fsout->surface, seat);
> > +   weston_surface_assign_keyboard(fsout->surface, 
> > seat);
> > +   if (fsout->surface != NULL)
> 
> Cannot be NULL.
> 
> > +   weston_surface_activate(fsout->surface);
> > return;
> > }
> > }
> > @@ -703,8 +708,11 @@ fullscreen_shell_present_surface(struct wl_client 
> > *client,
> > struct weston_keyboard *keyboard =
> > weston_seat_get_keyboard(seat);
> >  
> > -   if (keyboard && !keyboard->focus)
> > -   weston_surface_activate(surface, seat);
> > +   if (keyboard && !keyboard->focus) {
> > +   weston_surface_assign_keyboard(surface, seat);
> > +   if (surface != NULL)
> 
> Cannot be NULL.
> 
> > +   weston_surface_activate(surface);
> > +   }
> > }
> > }
> >  }
> > @@ -754,8 +762,11 @@ fullscreen_shell_present_surface_for_mode(struct 
> > wl_client *client,
> > struct w

Re: [PATCH weston v2 0/2] Separate Weston from libweston

[Bryce Harrington]

On Wed, Jun 22, 2016 at 02:44:23PM +0300, Pekka Paalanen wrote:
> On Thu,  9 Jun 2016 15:20:35 +0300
> Pekka Paalanen  wrote:
> 
> > From: Pekka Paalanen 
> > 
> > After these two patches, we have weston-the-compositor sources in 
> > compositor/,
> > and libweston sources in libweston/.
> > 
> > Since these patches have been generated with git format-patch -M and so
> > probably cannot be applied from email, I made the branch available at:
> > https://git.collabora.com/cgit/user/pq/weston.git/log/?h=migrate2
> > 
> > v2: move screen-share.c and note weston-launch.
> > 
> 
> Hi,
> 
> so far only Yong has given a Reviewed-by. Bryce gave a sort-of ok but
> not an official tag, can I take that as an Acked-by?

I would like to see the Weston patches that have been waiting for review
since before the release dealt with first, but given your plan to look
at and update them yourself you can count that as a provisional Acked-by.

The ones left in the queue are ones I felt like you'd want to have a say
on, so even apart from the directory split they should be on your
priority list regardless.  Else I'd have landed them myself.
 
> Reviews, Acks, anyone, please?
> Or do we not want this?
> 
> Or should I just go and land this with Yong's R-b and Bryce's A-b, does
> anyone care?
> 
> 
> Thanks,
> pq
> 
> 
> > Pekka Paalanen (2):
> >   Move weston source to compositor/
> >   Rename src/ to libweston/
> > 
> >  Makefile.am | 208 
> > ++--
> >  README  |   7 +-
> >  clients/cliptest.c  |   2 +-
> >  {src => compositor}/cms-colord.c|   0
> >  {src => compositor}/cms-helper.c|   0
> >  {src => compositor}/cms-helper.h|   0
> >  {src => compositor}/cms-static.c|   0
> >  {src => compositor}/main.c  |   0
> >  {src => compositor}/screen-share.c  |   0
> >  {src => compositor}/systemd-notify.c|   0
> >  {src => compositor}/text-backend.c  |   0
> >  {src => compositor}/weston-screenshooter.c  |   0
> >  {src => compositor}/weston.desktop  |   0
> >  {src => compositor}/weston.h|   0
> >  {src => compositor}/weston.pc.in|   0
> >  configure.ac|   4 +-
> >  desktop-shell/shell.c   |   2 +-
> >  ivi-shell/hmi-controller.c  |   2 +-
> >  ivi-shell/ivi-layout.c  |   2 +-
> >  ivi-shell/ivi-shell.c   |   2 +-
> >  {src => libweston}/animation.c  |   0
> >  {src => libweston}/bindings.c   |   0
> >  {src => libweston}/clipboard.c  |   0
> >  {src => libweston}/compositor-drm.c |   0
> >  {src => libweston}/compositor-drm.h |   0
> >  {src => libweston}/compositor-fbdev.c   |   0
> >  {src => libweston}/compositor-fbdev.h   |   0
> >  {src => libweston}/compositor-headless.c|   0
> >  {src => libweston}/compositor-headless.h|   0
> >  {src => libweston}/compositor-rdp.c |   0
> >  {src => libweston}/compositor-rdp.h |   0
> >  {src => libweston}/compositor-wayland.c |   0
> >  {src => libweston}/compositor-wayland.h |   0
> >  {src => libweston}/compositor-x11.c |   0
> >  {src => libweston}/compositor-x11.h |   0
> >  {src => libweston}/compositor.c |   0
> >  {src => libweston}/compositor.h |   0
> >  {src => libweston}/data-device.c|   0
> >  {src => libweston}/dbus.c   |   0
> >  {src => libweston}/dbus.h   |   0
> >  {src => libweston}/gl-renderer.c|   0
> >  {src => libweston}/gl-renderer.h|   0
> >  {src => libweston}/input.c  |   0
> >  {src => libweston}/launcher-direct.c|   0
> >  {src => libweston}/launcher-impl.h  |   0
> >  {src => libweston}/launcher-logind.c|   0
> >  {src => libweston}/launcher-util.c  |   0
> >  {src => libweston}/launcher-util.h  |   0
> >  {src => libweston}/launcher-weston-launch.c |   0
> >  {src => libweston}/libbacklight.c   |   0
> >  {src => libweston}/libbacklight.h   |   0
> >  {src => libweston}/libinput-device.c|   0
> >  {src => libweston}/libinput-device.h|   0
> >  {src => libweston}/libinput-seat.c  |   0
> >  {src => libweston}/libinput-seat.h  |   0
> >  {src => libweston}/libweston.pc.in  |   0
> >  {src => libweston}/linux-dmabuf.c   |   0
> >  {src => libweston}/linux-dmabuf.h   |   0
> >  {src => libweston}/log.c|   0
> >  {src => libweston}/noop-renderer.c  |   0
> >  {src => libweston}/pixman-renderer.c|   0
> >  {src => libweston}/pixman-renderer.h|   0
> >  {src => libweston}/screenshooter.c  |   0
> >  

Re: [PATCH wayland-web] FAQ: Modernise FAQ and inject some optimism

[Bryce Harrington]

On Tue, Jun 21, 2016 at 09:26:31PM +1000, Daniel Stone wrote:
> The FAQ was clearly written a very long time ago, when user sessions
> were a pipe dream, session compositors were the important thing, and we
> had a lot more questions than answers.
> 
> Now things have solidified a bit, and I'm writing this from a native
> Wayland client inside a native Wayland-on-KMS desktop, rewrite good
> chunks of the FAQ to reflect today's reality a little better.
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>

Just gave it a very quick once over.  +1 on anything that helps get the
website better up to date.  Details can always be fixed up more later.

Acked-by: Bryce Harrington <br...@osg.samsung.com>

> ---
>  faq.html | 115 
> ---
>  1 file changed, 43 insertions(+), 72 deletions(-)
> 
> diff --git a/faq.html b/faq.html
> index 58b6fc9..edcd6df 100644
> --- a/faq.html
> +++ b/faq.html
> @@ -52,45 +52,30 @@
>behalf of the clients, it expects the clients to use whatever means
>they prefer to render into a shareable buffer.  When the client is
>done, it informs the Wayland server of the new contents.  The
> -  current test clients use either cairo software rendering, cairo on
> -  OpenGL or hardware accelerated OpenGL directly.  As long as you have
> -  a userspace driver library that will let you render into a sharable
> -  buffer, you're good to go.
> +  current test clients use either Cairo software rendering, Cairo on
> +  OpenGL or hardware-accelerated OpenGL directly.  Additionally, media
> +  frameworks can share their buffers directly with the server.  As
> +  long as you have a userspace driver library that will let you render
> +  into a shareable buffer, you're good to go.
>  
>  
>  Is wayland replacing the X server?
>  
>  
> -  It could replace X as the native Linux graphics server, but I'm sure
> -  X will always be there on the side.  I imagine that Wayland and X
> -  will coexist in two ways on a Linux desktop: Wayland is a graphics
> -  multiplexer for a number of X servers.  Linux today typically only
> -  uses one X server for GDM and the user session, but we'll probably
> -  see that move to a dedicated GDM X server, an X server for user
> -  sessions (spawning more on the fly as more users log in) and maybe a
> -  dedicated screensaver/unlock X server.  Right now we rely on VT
> -  switching to move between X servers, and it's horrible.  We have no
> -  control over what the transitions look like and the VT ioctls are
> -  pretty bad.  Wayland provides a solution here, in that it can host
> -  several X servers as they push their root window to Wayland as
> -  surfaces.  The compositor in this case will be a dedicated session
> -  switcher that will cross-fade between X servers or spin them on a
> -  cube.
> -
> -
> -
> -  Further down the road we run a user session natively under
> -  Wayland with clients written for Wayland.  There will still (always)
> -  be X applications to run, but we now run these under a root-less X
> -  server that is itself a client of the Wayland server.  This will
> -  inject the X windows into the Wayland session as native looking
> -  clients.  The session Wayland server can run as a nested Wayland
> -  server under the system Wayland server described above, maybe even
> -  side by side with X sessions.  There's a number of intermediate
> -  steps, such as running the GNOME screen saver as a native wayland
> -  client, for example, or running a composited X desktop, where the
> -  compositor is a Wayland client, pushing the composited desktop to
> -  Wayland.
> +  Mostly, yes. User sessions are able to run under Wayland today, via
> +  a number of compositors: Weston itself as well as Enlightenment,
> +  GNOME Shell, KDE, and a number of others under development.  With
> +  most toolkits having Wayland ports, as well as frameworks such as
> +  GStreamer and SDL, it's perfectly possible to run a purely native
> +  Wayland session as your desktop.
> +
> +
> +
> +  That being said, there are some clients which rely on X11, and
> +  always will be.  To that end, XWayland provides a plugin for Wayland
> +  compositors, running a real X server.  This gives legacy clients a
> +  real and compliant X11 platform to run on, displayed side by side
> +  with native Wayland clients in your Wayland session.
>  
>  
>  Why not extend the X server?
> @@ -101,15 +86,11 @@
>exchange and update models that Wayland is built on into X.
>However, we have an option here of pushing X out of the hotpath
>between clients and the hardware and making it a compatibility
> -  option.  I'm not deluding myself that an

Re: [PATCH wayland-web] Build instructions for Ubuntu 16.04

[Bryce Harrington]

ibudev) 
> were not met:
> +sudo apt install libudev-dev
> +
> +# configure: error: Package requirements (libevdev 
> = 0.4) were not met:
> +sudo apt install libevdev-dev
> +
> +# configure: error: Package requirements (libwacom 
> = 0.12) were not met:
> +sudo apt install libwacom-dev
> +
> +git clone git://anongit.freedesktop.org/wayland/libinput
> +cd libinput
> +./autogen.sh --prefix=$WLD
> +make  make install
> +cd ..
> +
> +
> +# weston:
> +
> +# configure: error: Package requirements (egl glesv2) 
> were not met:
> +sudo apt install libgles2-mesa-dev 
> +
> +# configure: error: Package requirements (xcb 
> xcb-xfixes xcb-composite xcursor cairo-xcb) were not met:
> +sudo apt install libxcb-composite0-dev libxcursor-dev libcairo2-dev
> +
> +# configure: error: Package requirements (libudev 
> = 136 libdrm = 2.4.30 gbm mtdev = 1.1.0) were not met:
> +sudo apt install libgbm-dev
> +
> +# configure: error: weston-launch requires pam
> +sudo apt install libpam0g-dev
> +
> +git clone git://anongit.freedesktop.org/wayland/weston
> +cd weston
> +./autogen.sh --prefix=$WLD --disable-setuid-install 
> --with-xserver-path=$WLD/bin/Xwayland
> +make  make install
> +cd ..
> +
> +
> +# X Server:
> +
> +sudo apt install xutils-dev # xserver: 
> configure.ac:38: error: must install xorg-macros 1.14 or later before running 
> autoconf/autogen
> +sudo apt install libgl1-mesa-dev # xserver: configure: 
> error: Package requirements (glproto = 1.4.17 gl = 9.2.0) were not 
> met:
> +
> +# checking for SHA1 implementation... configure: 
> error: No suitable SHA1 implementation found
> +# checking for SHA1Init in -lmd... no
> +sudo apt install libmd-dev # no .pc file?
> +
> +# configure: error: Package requirements (fixesproto 
> = 5.0 damageproto = 1.1 xcmiscproto = 1.2.0 xtrans = 1.3.5 
> bigreqsproto = 1.1.0 xproto = 7.0.28 randrproto = 1.5.0 
> renderproto = 0.11 xextproto = 7.2.99.901 inputproto = 2.3 
> kbproto = 1.0.3 fontsproto = 2.1.3 pixman-1 = 0.27.2 videoproto 
> compositeproto = 0.4 recordproto = 1.13.99.1 scrnsaverproto = 1.1 
> resourceproto = 1.2.0 xf86driproto = 2.1.0 glproto = 1.4.17 dri 
> = 7.8.0 presentproto = 1.0 xineramaproto xkbfile  pixman-1 = 
> 0.27.2 xfont = 1.4.2 xau xshmfence = 1.1 xdmcp) were not met:
> +sudo apt install x11proto-xcmisc-dev x11proto-bigreqs-dev x11proto-randr-dev 
> x11proto-fonts-dev x11proto-video-dev x11proto-composite-dev 
> x11proto-record-dev x11proto-scrnsaver-dev x11proto-resource-dev 
> x11proto-xf86dri-dev x11proto-present-dev x11proto-xinerama-dev 
> libxkbfile-dev libxfont-dev
> +
> +# configure: error: Xwayland build explicitly 
> requested, but required modules not found.
> +# checking for XWAYLANDMODULES... no
> +# XWAYLANDMODULES="wayland-client = 1.3.0 libdrm 
> epoxy"
> +sudo apt install libepoxy-dev # this error message 
> sucks
> +
> +git clone git://anongit.freedesktop.org/xorg/xserver
> +cd xserver
> +./autogen.sh --prefix=$WLD --disable-docs --disable-devel-docs \
> +  --enable-xwayland --disable-xorg --disable-xvfb --disable-xnest \
> +  --disable-xquartz --disable-xwin
> +make  make install
> +cd ..
> +
> +# Links needed so XWayland works:
> +mkdir -p $WLD/share/X11/xkb/rules
> +ln -s /usr/share/X11/xkb/rules/evdev $WLD/share/X11/xkb/rules/
> +ln -s /usr/bin/xkbcomp $WLD/bin/
> +
> +# Weston configuration:
> +mkdir -p ~/.config
> +cp weston/weston.ini ~/.config
> +nano ~/.config/weston.ini # edit to set background and 
> turn on xwayland.so module
> +
> +# Run it in an X11 window:
> +weston
> +
> +
> +
> +
> +
> -- 
> 2.7.4

Acked-by: Bryce Harrington <br...@osg.samsung.com>

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] weston-simple-im: Make capitalization consistent in error messages

[Bryce Harrington]

On Thu, Jun 16, 2016 at 09:24:56PM -0600, Yong Bakos wrote:
> 
> > On Jun 16, 2016, at 12:11 PM, Bryce Harrington <br...@osg.samsung.com> 
> > wrote:
> > 
> > On Thu, Jun 16, 2016 at 11:14:01AM -0500, Derek Foreman wrote:
> >> On 15/06/16 07:59 PM, Bryce Harrington wrote:
> >>> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> >> 
> >> Super important work, proudly
> >> Reviewed-by: Derek Foreman <der...@osg.samsung.com>
> >> 
> >> Though maybe we should put . on the end if they're capitalized, make
> >> them complete sentences?
> 
> You will think I'm crazy, but I recently sent a patch to the git crew
> about adding a period to some specific git output and, long story short,
> one of their conventions is to not use them at the end of error messages.
> 
> Makes me wonder, as mundane as it may sound, if this is a greater (posix/
> unix/linux) convention.

I've seen polices both ways in other projects.  I personally don't have
an opinion one way or the other if we're consistent.

One thing to consider is the policy of libraries we use, that might be
issuing warnings and error messages.  If we adopt a differing policy
then those will show up as inconsistent.

> Anyway, just wanted to share that silly story, not debate the point, er,
> period.
> 
> yong
> 
> 
> > Maybe, yeah, but I'll leave that to someone else.  Looks like there's a
> > lot more formatting/wording inconsistencies in the client error messages.
> > 
> > Thanks for the reviews, I've pushed the three weston ones.
> > 
> >   b01c31b..f013ce9  master -> master
> > 
> > Bryce
> > 
> >>> ---
> >>> clients/weston-simple-im.c | 6 +++---
> >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/clients/weston-simple-im.c b/clients/weston-simple-im.c
> >>> index 4c1d7cf..0ee505a 100644
> >>> --- a/clients/weston-simple-im.c
> >>> +++ b/clients/weston-simple-im.c
> >>> @@ -197,13 +197,13 @@ input_method_keyboard_keymap(void *data,
> >>>   close(fd);
> >>> 
> >>>   if (!keyboard->keymap) {
> >>> - fprintf(stderr, "failed to compile keymap\n");
> >>> + fprintf(stderr, "Failed to compile keymap\n");
> >>>   return;
> >>>   }
> >>> 
> >>>   keyboard->state = xkb_state_new(keyboard->keymap);
> >>>   if (!keyboard->state) {
> >>> - fprintf(stderr, "failed to create XKB state\n");
> >>> + fprintf(stderr, "Failed to create XKB state\n");
> >>>   xkb_keymap_unref(keyboard->keymap);
> >>>   return;
> >>>   }
> >>> @@ -489,7 +489,7 @@ main(int argc, char *argv[])
> >>> 
> >>>   simple_im.display = wl_display_connect(NULL);
> >>>   if (simple_im.display == NULL) {
> >>> - fprintf(stderr, "failed to connect to server: %m\n");
> >>> + fprintf(stderr, "Failed to connect to server: %m\n");
> >>>   return -1;
> >>>   }
> >>> 
> >>> 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] terminal: Document console codes less cryptically

[Bryce Harrington]

C.f. http://man7.org/linux/man-pages/man4/console_codes.4.html

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 clients/terminal.c | 72 +++---
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/clients/terminal.c b/clients/terminal.c
index a70fef3..01dc2d2 100644
--- a/clients/terminal.c
+++ b/clients/terminal.c
@@ -1333,12 +1333,12 @@ handle_escape(struct terminal *terminal)
}
 
switch (*p) {
-   case '@':/* ICH */
+   case '@':/* ICH - Insert  blank characters */
count = set[0] ? args[0] : 1;
if (count == 0) count = 1;
terminal_shift_line(terminal, count);
break;
-   case 'A':/* CUU */
+   case 'A':/* CUU - Move cursor up  rows */
count = set[0] ? args[0] : 1;
if (count == 0) count = 1;
if (terminal->row - count >= terminal->margin_top)
@@ -1346,7 +1346,7 @@ handle_escape(struct terminal *terminal)
else
terminal->row = terminal->margin_top;
break;
-   case 'B':/* CUD */
+   case 'B':/* CUD - Move cursor down  rows */
count = set[0] ? args[0] : 1;
if (count == 0) count = 1;
if (terminal->row + count <= terminal->margin_bottom)
@@ -1354,7 +1354,7 @@ handle_escape(struct terminal *terminal)
else
terminal->row = terminal->margin_bottom;
break;
-   case 'C':/* CUF */
+   case 'C':/* CUF - Move cursor right by  columns */
count = set[0] ? args[0] : 1;
if (count == 0) count = 1;
if ((terminal->column + count) < terminal->width)
@@ -1362,7 +1362,7 @@ handle_escape(struct terminal *terminal)
else
terminal->column = terminal->width - 1;
break;
-   case 'D':/* CUB */
+   case 'D':/* CUB - Move cursor left  columns */
count = set[0] ? args[0] : 1;
if (count == 0) count = 1;
if ((terminal->column - count) >= 0)
@@ -1370,7 +1370,7 @@ handle_escape(struct terminal *terminal)
else
terminal->column = 0;
break;
-   case 'E':/* CNL */
+   case 'E':/* CNL - Move cursor down  rows, to column 1 */
count = set[0] ? args[0] : 1;
if (terminal->row + count <= terminal->margin_bottom)
terminal->row += count;
@@ -1378,7 +1378,7 @@ handle_escape(struct terminal *terminal)
terminal->row = terminal->margin_bottom;
terminal->column = 0;
break;
-   case 'F':/* CPL */
+   case 'F':/* CPL - Move cursour up  rows, to column 1 */
count = set[0] ? args[0] : 1;
if (terminal->row - count >= terminal->margin_top)
terminal->row -= count;
@@ -1386,14 +1386,14 @@ handle_escape(struct terminal *terminal)
terminal->row = terminal->margin_top;
terminal->column = 0;
break;
-   case 'G':/* CHA */
+   case 'G':/* CHA - Move cursor to column  in current row */
y = set[0] ? args[0] : 1;
y = y <= 0 ? 1 : y > terminal->width ? terminal->width : y;
 
terminal->column = y - 1;
break;
-   case 'f':/* HVP */
-   case 'H':/* CUP */
+   case 'f':/* HVP - Move cursor to <x, y> */
+   case 'H':/* CUP - Move cursor to <x, y> (origin at 1,1) */
x = (set[1] ? args[1] : 1) - 1;
x = x < 0 ? 0 :
(x >= terminal->width ? terminal->width - 1 : x);
@@ -1420,7 +1420,7 @@ handle_escape(struct terminal *terminal)
}
terminal->column--;
break;
-   case 'J':/* ED */
+   case 'J':/* ED - Erase display */
row = terminal_get_row(terminal, terminal->row);
attr_row = terminal_get_attr_row(terminal, terminal->row);
if (!set[0] || args[0] == 0 || args[0] > 2) {
@@ -1449,7 +1449,7 @@ handle_escape(struct terminal *terminal)
   terminal->end - terminal->start);
}
break;
-   case 'K':/* EL */
+   case 'K':/* EL - Erase line */
row = terminal_get_row(terminal, terminal->row);
attr_row = terminal_get_attr_row(terminal, terminal->row);
if (!set[0] || args[0] == 0 || args[0] > 2) {
@@ -1465,7 +1465,7 @@ handle_escape(struct ter

[PATCH weston] Make config.h inclusion consistent

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 clients/multi-resource.c  | 2 +-
 clients/presentation-shm.c| 2 +-
 clients/simple-damage.c   | 2 +-
 clients/simple-dmabuf-intel.c | 2 +-
 clients/simple-dmabuf-v4l.c   | 2 +-
 clients/simple-shm.c  | 2 +-
 clients/simple-touch.c| 2 +-
 clients/terminal.c| 2 +-
 clients/weston-simple-im.c| 2 +-
 src/cms-colord.c  | 4 +---
 xwayland/hash.c   | 2 +-
 11 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/clients/multi-resource.c b/clients/multi-resource.c
index b061e68..c283022 100644
--- a/clients/multi-resource.c
+++ b/clients/multi-resource.c
@@ -22,7 +22,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/presentation-shm.c b/clients/presentation-shm.c
index 2d9b885..998ce8b 100644
--- a/clients/presentation-shm.c
+++ b/clients/presentation-shm.c
@@ -23,7 +23,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/simple-damage.c b/clients/simple-damage.c
index 82091d5..bb81902 100644
--- a/clients/simple-damage.c
+++ b/clients/simple-damage.c
@@ -23,7 +23,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/simple-dmabuf-intel.c b/clients/simple-dmabuf-intel.c
index 2ac2200..fa2bfc4 100644
--- a/clients/simple-dmabuf-intel.c
+++ b/clients/simple-dmabuf-intel.c
@@ -22,7 +22,7 @@
  * OF THIS SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/simple-dmabuf-v4l.c b/clients/simple-dmabuf-v4l.c
index a73556c..7ee5cce 100644
--- a/clients/simple-dmabuf-v4l.c
+++ b/clients/simple-dmabuf-v4l.c
@@ -20,7 +20,7 @@
  * OF THIS SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/simple-shm.c b/clients/simple-shm.c
index 1aa26ce..c442d88 100644
--- a/clients/simple-shm.c
+++ b/clients/simple-shm.c
@@ -22,7 +22,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/simple-touch.c b/clients/simple-touch.c
index 446f2ca..8fd0b0a 100644
--- a/clients/simple-touch.c
+++ b/clients/simple-touch.c
@@ -22,7 +22,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/terminal.c b/clients/terminal.c
index a70fef3..66bdeec 100644
--- a/clients/terminal.c
+++ b/clients/terminal.c
@@ -21,7 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/clients/weston-simple-im.c b/clients/weston-simple-im.c
index 4c1d7cf..426fec7 100644
--- a/clients/weston-simple-im.c
+++ b/clients/weston-simple-im.c
@@ -21,7 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
diff --git a/src/cms-colord.c b/src/cms-colord.c
index c88eb11..b9bc9e3 100644
--- a/src/cms-colord.c
+++ b/src/cms-colord.c
@@ -23,9 +23,7 @@
  * SOFTWARE.
  */
 
-#ifdef HAVE_CONFIG_H
-#include 
-#endif
+#include "config.h"
 
 #include 
 #include 
diff --git a/xwayland/hash.c b/xwayland/hash.c
index 54f3de9..820e51f 100644
--- a/xwayland/hash.c
+++ b/xwayland/hash.c
@@ -32,7 +32,7 @@
  *Keith Packard <kei...@keithp.com>
  */
 
-#include 
+#include "config.h"
 
 #include 
 #include 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] weston-simple-im: Make capitalization consistent in error messages

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 clients/weston-simple-im.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/clients/weston-simple-im.c b/clients/weston-simple-im.c
index 4c1d7cf..0ee505a 100644
--- a/clients/weston-simple-im.c
+++ b/clients/weston-simple-im.c
@@ -197,13 +197,13 @@ input_method_keyboard_keymap(void *data,
close(fd);
 
if (!keyboard->keymap) {
-   fprintf(stderr, "failed to compile keymap\n");
+   fprintf(stderr, "Failed to compile keymap\n");
return;
}
 
keyboard->state = xkb_state_new(keyboard->keymap);
if (!keyboard->state) {
-   fprintf(stderr, "failed to create XKB state\n");
+   fprintf(stderr, "Failed to create XKB state\n");
xkb_keymap_unref(keyboard->keymap);
return;
}
@@ -489,7 +489,7 @@ main(int argc, char *argv[])
 
simple_im.display = wl_display_connect(NULL);
if (simple_im.display == NULL) {
-   fprintf(stderr, "failed to connect to server: %m\n");
+   fprintf(stderr, "Failed to connect to server: %m\n");
return -1;
}
 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libxkbcommon 3/3] doc: Also mention the wayland test client in the quick guide

[Bryce Harrington]

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 doc/quick-guide.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/quick-guide.md b/doc/quick-guide.md
index 6e04ba5..9d66475 100644
--- a/doc/quick-guide.md
+++ b/doc/quick-guide.md
@@ -20,6 +20,8 @@ can find complete and more complex examples in the source 
directory:
 
 2. test/interactive-x11.c contains an interactive X11 client.
 
+3. test/interactive-wayland.c contains an interactive Wayland client.
+
 Also, the library contains many more functions for examining and using
 the library context, the keymap and the keyboard state. See the
 hyper-linked reference documentation or go through the header files in
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libxkbcommon 2/3] doc: Declare keymap for wayland example

[Bryce Harrington]

keymap was defined in the X11 example, but also define it in the wayland
example just to make it a bit more standalone

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 doc/quick-guide.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/quick-guide.md b/doc/quick-guide.md
index a576070..6e04ba5 100644
--- a/doc/quick-guide.md
+++ b/doc/quick-guide.md
@@ -74,6 +74,7 @@ with a keymap. In this case, we can create the keymap object 
like this:
 ~~~{.c}
 /* From the wl_keyboard::keymap event. */
 const char *keymap_string = <...>;
+struct xkb_keymap *keymap;
 
 keymap = xkb_keymap_new_from_string(ctx, keymap_string,
 XKB_KEYMAP_FORMAT_TEXT_V1,
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libxkbcommon 1/3] doc: Fix ctx type in example

[Bryce Harrington]

xkb_context_new() returns a xkb_context pointer, so change the variable
definition to be consistent.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 doc/quick-guide.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/quick-guide.md b/doc/quick-guide.md
index 972d09c..a576070 100644
--- a/doc/quick-guide.md
+++ b/doc/quick-guide.md
@@ -32,7 +32,7 @@ Before we can do anything interesting, we need a library 
context:
 ~~~{.c}
 #include 
 
-struct xkb_context ctx;
+struct xkb_context *ctx;
 
 ctx = xkb_context_new(XKB_CONTEXT_NO_FLAGS);
 if (!ctx) 
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2 0/2] Separate Weston from libweston

[Bryce Harrington]

On Thu, Jun 09, 2016 at 03:20:35PM +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> After these two patches, we have weston-the-compositor sources in compositor/,
> and libweston sources in libweston/.
> 
> Since these patches have been generated with git format-patch -M and so
> probably cannot be applied from email, I made the branch available at:
> https://git.collabora.com/cgit/user/pq/weston.git/log/?h=migrate2
> 
> v2: move screen-share.c and note weston-launch.

This seems fine, although there's a number of pending patches for weston
that this will obviously break.  Since the rename can be done
mechanically for the most part, what would you think if we make an
effort to get all the weston patches prior to, say, June 1st, reviewed
and either landed or sent back with requests to update them to the new
layout?  I seem to recall a number of the pending patches we felt might
be landable post-release, so might be nice to clear the slate of those.

Bryce
 
> Thanks,
> pq
> 
> Pekka Paalanen (2):
>   Move weston source to compositor/
>   Rename src/ to libweston/
> 
>  Makefile.am | 208 
> ++--
>  README  |   7 +-
>  clients/cliptest.c  |   2 +-
>  {src => compositor}/cms-colord.c|   0
>  {src => compositor}/cms-helper.c|   0
>  {src => compositor}/cms-helper.h|   0
>  {src => compositor}/cms-static.c|   0
>  {src => compositor}/main.c  |   0
>  {src => compositor}/screen-share.c  |   0
>  {src => compositor}/systemd-notify.c|   0
>  {src => compositor}/text-backend.c  |   0
>  {src => compositor}/weston-screenshooter.c  |   0
>  {src => compositor}/weston.desktop  |   0
>  {src => compositor}/weston.h|   0
>  {src => compositor}/weston.pc.in|   0
>  configure.ac|   4 +-
>  desktop-shell/shell.c   |   2 +-
>  ivi-shell/hmi-controller.c  |   2 +-
>  ivi-shell/ivi-layout.c  |   2 +-
>  ivi-shell/ivi-shell.c   |   2 +-
>  {src => libweston}/animation.c  |   0
>  {src => libweston}/bindings.c   |   0
>  {src => libweston}/clipboard.c  |   0
>  {src => libweston}/compositor-drm.c |   0
>  {src => libweston}/compositor-drm.h |   0
>  {src => libweston}/compositor-fbdev.c   |   0
>  {src => libweston}/compositor-fbdev.h   |   0
>  {src => libweston}/compositor-headless.c|   0
>  {src => libweston}/compositor-headless.h|   0
>  {src => libweston}/compositor-rdp.c |   0
>  {src => libweston}/compositor-rdp.h |   0
>  {src => libweston}/compositor-wayland.c |   0
>  {src => libweston}/compositor-wayland.h |   0
>  {src => libweston}/compositor-x11.c |   0
>  {src => libweston}/compositor-x11.h |   0
>  {src => libweston}/compositor.c |   0
>  {src => libweston}/compositor.h |   0
>  {src => libweston}/data-device.c|   0
>  {src => libweston}/dbus.c   |   0
>  {src => libweston}/dbus.h   |   0
>  {src => libweston}/gl-renderer.c|   0
>  {src => libweston}/gl-renderer.h|   0
>  {src => libweston}/input.c  |   0
>  {src => libweston}/launcher-direct.c|   0
>  {src => libweston}/launcher-impl.h  |   0
>  {src => libweston}/launcher-logind.c|   0
>  {src => libweston}/launcher-util.c  |   0
>  {src => libweston}/launcher-util.h  |   0
>  {src => libweston}/launcher-weston-launch.c |   0
>  {src => libweston}/libbacklight.c   |   0
>  {src => libweston}/libbacklight.h   |   0
>  {src => libweston}/libinput-device.c|   0
>  {src => libweston}/libinput-device.h|   0
>  {src => libweston}/libinput-seat.c  |   0
>  {src => libweston}/libinput-seat.h  |   0
>  {src => libweston}/libweston.pc.in  |   0
>  {src => libweston}/linux-dmabuf.c   |   0
>  {src => libweston}/linux-dmabuf.h   |   0
>  {src => libweston}/log.c|   0
>  {src => libweston}/noop-renderer.c  |   0
>  {src => libweston}/pixman-renderer.c|   0
>  {src => libweston}/pixman-renderer.h|   0
>  {src => libweston}/screenshooter.c  |   0
>  {src => libweston}/spring-tool.c|   0
>  {src => libweston}/timeline-object.h|   0
>  {src => libweston}/timeline.c   |   0
>  {src => libweston}/timeline.h   |   0
>  {src => libweston}/vaapi-recorder.c |   0
>  {src => libweston}/vaapi-recorder.h |   0
>  {src => libweston}/version.h.in |   0
>  {src => libweston}/vertex-clipping.c|   0
>  {src => libweston}/vertex-clipping.h|   0
>  

Re: [PATCH weston v3 7/8] Define the screensaver inhibitor client interface

[Bryce Harrington]

On Thu, May 26, 2016 at 06:02:09PM +0300, Pekka Paalanen wrote:
> On Thu,  7 Apr 2016 16:44:22 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > Hook up the API defined in wayland-protocols to allow client screensaver
> > inhibition requests.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> > v2: Update for protocol rename
> > 
> >  Makefile.am  |  9 +++--
> >  clients/simple-shm.c | 28 
> >  2 files changed, 31 insertions(+), 6 deletions(-)
> 
> Hi,
> 
> I would really wish we didn't add any more stuff into simple-shm, or
> any of the simple-* clients, unless it is essential to them.
> 
> Something like the subsurfaces demo would be more suitable, that would
> allow you to test the inhibi... hmm, wait, no, all subsurfaces there
> are still inside the parent. I think you need a new test client that
> creates a subsurface outside of the main surface, so you can test the
> inhibitor inheritance.

Given the subsurfaces stuff seems to still be a source of disagreement,
and evidently is something I totally am not grokking, I don't want to
get into implementing subsurface support.  The typical client won't be
using subsurfaces, so a simpler demo would be more relevant for them.
For clients that *do* use subsurfaces, well it sounds like they could
have quite a diverse range of behavioral needs regarding inhibition, so
making a good subsurface demo sounds like it could get rather involved.

I do understand your point about wanting to avoid shoehorning more
features into the simple-* clients; they're already kind of a hodge
podge.  So I'll look at creating a new client like simple-shm that is
boiled down to just showing off the inhibit feature with a single top
level surface.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Wed, Jun 08, 2016 at 12:02:14PM +0300, Pekka Paalanen wrote:
> On Wed, 8 Jun 2016 10:51:30 +0800
> Jonas Ådahl <jad...@gmail.com> wrote:
> 
> > On Tue, Jun 07, 2016 at 07:10:25PM -0700, Bryce Harrington wrote:
> > > On Fri, Jun 03, 2016 at 09:26:24AM +0800, Jonas Ådahl wrote:  
> > > > On Thu, Jun 02, 2016 at 02:24:20PM -0700, Bryce Harrington wrote:  
> > > > > +  Note that in the case of a compound window (a surface with
> > > > > +  sub-surfaces), the inhibitor effects should be inherited from 
> > > > > the top
> > > > > +  level surface.  
> > > > 
> > > > Does this mean that if we have a subsurface with an inhibitor object,
> > > > and for example an xdg_surface without one, the subsurface would still
> > > > not inhibit the idle behavior, as it'd inherit the behavior?
> > > > 
> > > > If it should work like this, maybe we should make it an error to create
> > > > an idle inhibitor on a subsurface. If it should inhibit, I think it
> > > > needs to be clarified.  
> > > 
> > > Well, I suppose we should consider likely use cases here.
> > > 
> > > Is video-in-browser considered one of the big uses for subsurfaces?
> > > Here's a couple use cases off the top of my head:
> > > 
> > > a) I want to watch a video embedded in a web page.  I typically maximize
> > >the video so I can see it well.  I would expect the screensaver to
> > >not kick on for the duration of this video.  Once I'm done with it
> > >and go back to regular web browser usage, I want my screensaver to
> > >work normally.
> > > 
> > > b) I'm surfing the web reading articles on some annoying site that
> > >auto-plays videos for me.  Maybe it's an advertisement, or maybe just
> > >a live video news report version of the news article I'm reading.  I
> > >just ignore it because I don't care.  When the video is done, it
> > >automatically plays the next advertisement or video.  These videos
> > >are annoying enough themselves, I definitely do not want these videos
> > >causing my screensaver to not kick in.
> > > 
> > > So based on this I'm thinking subsurfaces probably should not be allowed
> > > to automatically override the parent.  But if the user takes some action
> > > that can be interpreted as "I am focusing on this subsurface" such as
> > > maximizing it or making it larger or something, then in that case I
> > > could see permitting it to inhibit.  
> > 
> > I don't think we should start to adding things about focusing
> > subsurfaces. I also don't think we should really try to solve good vs
> > bad videos in a browser in the protocol.

Neither of which was I doing.  Perhaps you're misunderstanding my use of
'focusing' to mean something technical like mouse focus, when all I'm
saying is "the user is actually looking at it."  And I don't see how you
are possibly interpreting my speculative example into some sort of
absurd proposal to fix bad internet videos.  You're just building a
strawman argument here, rather than helping me get to a tangible use
case where inhibition in relation to subsurfaces are actually of some
utility.

> > > Is a maximized subsurface still a subsurface or does it become a parent
> > > level surface?  If it does, then I think we can just let maximization be
> > > the indication to allow inhibition, and otherwise just not let
> > > subsurfaces interfere with their parent settings.  But if it doesn't,
> > > then perhaps we need a more sophisticated mechanism here.  
> > 
> > There is no such thing as a maximized subsurface. We maximize or
> > fullscreen a shell surface, and the client updates its subsurface
> > accordingly. I don't think we should mix in shell surface behavior in
> > this protocol, we should just trust the web browser to protect us from
> > nasty web page behaviors, and let it inhibit things the way it sees fit.
> 
> Also agreed.
> 
> Furthermore, one cannot use any shell concepts in defining the
> idle-inhibit extension, because idle-inhibit is a sibling, not a child
> extension to shells. If you need to use those concepts, then you need
> define them or make this a shell-extension-specific extension.

Jonas' question was for how this would work in a larger context, for
instance with xdg_surface.  This is not a proposal, and I'm quite aware
of the need to keep this agnostic of shell behavior, so no need for
pedagogy on that point.

> > The decision we need to make is: should

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Fri, Jun 03, 2016 at 09:26:24AM +0800, Jonas Ådahl wrote:
> On Thu, Jun 02, 2016 at 02:24:20PM -0700, Bryce Harrington wrote:
> > This interface allows disabling of screensaver/screenblanking on a
> > per-surface basis.  As long as the surface remains visible and
> > non-occluded it blocks the screensaver, etc. from activating on the
> > output(s) that the surface is visible on.
> > 
> > To uninhibit, simply destroy the inhibitor object.
> > 
> > Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
> > ---
> > 
> > v2:
> >  + Rename protocol to idle-inhibit
> > v3:
> >  + Added a destructor for the idle manager
> >  + Changed sub-surface behavior to inherit idle from parent surface
> >  + Various wording changes suggested by pq
> > 
> 
> Hi,
> 
> I think this looks sane, I only have a couple of questions that might
> need clarification.
> 
> > 
> >  Makefile.am|  1 +
> >  unstable/idle-inhibit/README   |  4 ++
> >  unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 84 
> > ++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644 unstable/idle-inhibit/README
> >  create mode 100644 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 71d2632..de691db 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -8,6 +8,7 @@ unstable_protocols =
> > \
> > unstable/relative-pointer/relative-pointer-unstable-v1.xml  
> > \
> > unstable/pointer-constraints/pointer-constraints-unstable-v1.xml
> > \
> > unstable/tablet/tablet-unstable-v1.xml  
> > \
> > +   unstable/idle-inhibit/idle-inhibit-unstable-v1.xml  
> > \
> > $(NULL)
> >  
> >  stable_protocols =     
> > \
> > diff --git a/unstable/idle-inhibit/README b/unstable/idle-inhibit/README
> > new file mode 100644
> > index 000..396e871
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/README
> > @@ -0,0 +1,4 @@
> > +Screensaver inhibition protocol
> > +
> > +Maintainers:
> > +Bryce Harrington <br...@osg.samsung.com>
> > diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml 
> > b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > new file mode 100644
> > index 000..af3a911
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > @@ -0,0 +1,84 @@
> > +
> > +
> > +
> > +  
> > +Copyright © 2015 Samsung Electronics Co., Ltd
> > +
> > +Permission is hereby granted, free of charge, to any person obtaining a
> > +copy of this software and associated documentation files (the 
> > "Software"),
> > +to deal in the Software without restriction, including without 
> > limitation
> > +the rights to use, copy, modify, merge, publish, distribute, 
> > sublicense,
> > +and/or sell copies of the Software, and to permit persons to whom the
> > +Software is furnished to do so, subject to the following conditions:
> > +
> > +The above copyright notice and this permission notice (including the 
> > next
> > +paragraph) shall be included in all copies or substantial portions of 
> > the
> > +Software.
> > +
> > +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> > EXPRESS OR
> > +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> > MERCHANTABILITY,
> > +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> > SHALL
> > +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > +DEALINGS IN THE SOFTWARE.
> > +  
> > +
> > +  
> > +
> > +  This interface permits inhibiting the idle behavior such as screen
> > +  blanking, locking, and screensaving.  The client binds the idle 
> > manager
> > +  globally, then creates idle-inhibitor objects for each surface.
> 
> + for each surface that should inhibit the idle behavior. ?

How about just a simple:

  for each surface as appropriate.

> > +  Warning! The p

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Fri, Jun 03, 2016 at 11:41:12AM +0300, Pekka Paalanen wrote:
> On Thu, 2 Jun 2016 14:33:42 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > On Wed, May 18, 2016 at 04:11:39PM +0300, Pekka Paalanen wrote:
> > > On Thu, 24 Mar 2016 11:14:33 -0700
> > > Bryce Harrington <br...@osg.samsung.com> wrote:
> > >   
> > > > This interface allows disabling of screensaver/screenblanking on a
> > > > per-surface basis.  As long as the surface remains visible and
> > > > non-occluded it blocks the screensaver, etc. from activating on the
> > > > output(s) that the surface is visible on.
> > > > 
> > > > To uninhibit, simply destroy the inhibitor object.  
> > > 
> > > Hi,
> > > 
> > > ok, that sounds good.
> > >   
> > > > Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
> > > > ---
> > > > v2: Rename protocol to idle-inhibit
> > > >  + Use "idle" rather than "screensaver".  People likely to be interested
> > > >in this protocol would know the difference between "blanking", 
> > > > "dpms",
> > > >and literal "screensavers", so don't lead them to think that this 
> > > > only
> > > >deals with the latter.  The protocol addresses all manner of idle
> > > >behaviors, so is a more accurate name.
> > > >  + Standardize nomenclature to "idle-manager" and "inhibitor" for 
> > > > clarity.
> > > >"inhibition_inhibit" was just too cumbersome.  
> > > 
> > > Alright!
> > >   
> > > > 
> > > >  Makefile.am|  1 +
> > > >  unstable/idle-inhibit/README   |  4 ++
> > > >  unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 78 
> > > > ++
> > > >  3 files changed, 83 insertions(+)
> > > >  create mode 100644 unstable/idle-inhibit/README
> > > >  create mode 100644 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > > > 
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 5926a41..ac497d8 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -5,6 +5,7 @@ unstable_protocols =
> > > > \
> > > > unstable/text-input/text-input-unstable-v1.xml  
> > > > \
> > > > unstable/input-method/input-method-unstable-v1.xml      
> > > > \
> > > > unstable/xdg-shell/xdg-shell-unstable-v5.xml
> > > > \
> > > > +   unstable/idle-inhibit/idle-inhibit-unstable-v1.xml  \
> > > > $(NULL)
> > > >  
> > > >  nobase_dist_pkgdata_DATA = 
> > > > \
> > > > diff --git a/unstable/idle-inhibit/README b/unstable/idle-inhibit/README
> > > > new file mode 100644
> > > > index 000..396e871
> > > > --- /dev/null
> > > > +++ b/unstable/idle-inhibit/README
> > > > @@ -0,0 +1,4 @@
> > > > +Screensaver inhibition protocol
> > > > +
> > > > +Maintainers:
> > > > +Bryce Harrington <br...@osg.samsung.com>
> > > > diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml 
> > > > b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > > > new file mode 100644
> > > > index 000..a8c8a85
> > > > --- /dev/null
> > > > +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > > > @@ -0,1 +1,78 @@
> > > > +
> > > > +
> > > > +
> > > > +  
> > > > +Copyright © 2015-2016 Samsung Electronics Co., Ltd
> > > > +
> > > > +Permission is hereby granted, free of charge, to any person 
> > > > obtaining a
> > > > +copy of this software and associated documentation files (the 
> > > > "Software"),
> > > > +to deal in the Software without restriction, including without 
> > > > limitation
> > > > +the rights to use, copy, modify, merge, publish, distribute, 
> > > > sublicense,
> > > > +and/or sell copies of the Software, and to permit persons t

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Mon, Jun 06, 2016 at 10:08:00AM -0500, Yong Bakos wrote:
> On Jun 6, 2016, at 7:18 AM, Pekka Paalanen  wrote:
> >> I don't believe this is a good choice. Imagine the case of a surface-less
> >> 'inhibitor daemon.'
> > 
> > An ordinary client must not be able to do that.
> > 
> >> There may be no visible surface (is my thinking out
> >> of scope here?). Imagine another case, that of a "caffeine" widget. This
> >> widget's surface would be hidden when another app is fullscreen.
> > 
> > If you cannot see the widget anyway, it must not be able to affect
> > screen saving. Therefore by definition, surfaceless clients must not be
> > able to inhibit.
> > 
> >> Furthermore, I don't believe that inhibition should be coupled to outputs.
> >> See below.
> > 
> > It should work per-output. If there is important info on just one
> > output, why should the other outputs also stay on for no good reason?
> 
> Just because I'm looking at info on one monitor doesn't mean I want my other
> monitors to go to sleep.

Not quite, there's actually three different situations to consider

First is if you're watching one monitor and doing something else on the
other.  Like watching a movie while browsing emails.  In this case, your
other usage is going to be generating input activity, and the system
isn't going to be idling to sleep in the first place, so inhibition
simply isn't relevant.

Next consider if you're passively watching two things at once, one
client on one screen and another client on the other.  Maybe you're
monitoring a security camera on one screen and watching live stock
tickers on the other or something.  Well, this is completely in scope of
the idle inhibit design - each client issues an idle inhibit request and
you're good to go.

Finally consider you're watching something on one and *not* doing
anything on the other.  In this case, the second head *should* power
save when appropriate.

State of the art for most screensaver inhibitor tech addresses the
second situation, but leaves the third unsolved.  We should be able to
handle the set.

> I can imagine this being specified via a shell's
> screensaver/power savings config panel, with a checkbox enabling "idle per 
> screen" or "idle
> all screens in unison." So I think this is a moot point.

Exactly - in practice there are generally out-of-band ways to control
screensavers and DPMS and so forth.  I'm trying to avoid making any
assumptions that they exist or are usable, but at least for cases in
where a given client hasn't implemented idle inhibition, or is broken
for some other reason, the user is going to have options.  The protocol
doesn't need to design for non-supporting clients.

> However, I don't believe that just because a surface is occluded, that
> it's ability to inhibit idling should be suppressed.

It's entirely possible; I don't claim to be prescient about all the
various use cases that may exist.  But that would be a pretty
fundamental change, so lets only consider it if there are likely use
cases that just can't be addressed with what we've got so far.

Like I mentioned earlier, we've actually taken a few things *out* of the
protocol because we just lacked rational use cases for them, and because
its easy to add more functionality later but much much harder to remove
functionality that proves unnecessary later.

> >> I don't believe that inhibition should /only/ be coupled to outputs.
> >> In the case of a "caffeine" widget or daemon, the use case is to
> >> prevent /all/ outputs from idling, regardless of what output the widget's
> >> surface resides upon.
> > 
> > That is explicitly not what we want to enable.
> > 
> > What is this "caffeine" you talk about? I've never heard of it. Maybe I
> > never had to fight against screen saving.
> 
> Here's a screenshot of the Gnome-flavored implementation of the widget:
> http://tinyurl.com/caffeine-widget-2
> 
> OSX and others have something similar. This program visually resides in the
> menubar/taskbar of the shell. It has two states, on and off. On (caffeinated)
> overrides the configured screensaver/sleep idle timeout, preventing /all/
> monitors from idling. In the off (un-caffeinated) state, the system behaves
> according to configuration.
> 
> Use case: I've configured my system to screensave at 5 minutes and put the
> monitors to sleep at 10 minutes. But sometimes, I want to override this
> without going in and changing my config, only to have to change it back
> later. I run the caffeine widget, turn it on, and all my monitors remain on
> despite me not interacting with my machine. This behavior remains valid
> even if I am running a program fullscreen, which occludes the visibility
> of the widget.
> 
> It sounds like, from what Bryce had wrote in a separate reply, such a
> widget would use a different API and not this idle inhibitor protocol. I
> might be misunderstanding his point.

Right, idle-inhibit is a functionality that is automatic and
per-client.  What 

[PATCH v2] scanner: Fix reported executable name to 'wayland-scanner'

[Bryce Harrington]

'wayland-scanner -v' (correctly) reports the program as named
"wayland-scanner", but 'wayland-scanner -h' was inconsistent, referring
to it as './scanner'.

Also refactor this and other references to the program name to use a
common #define, PROGRAM_NAME.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
Reviewed-by: Jonas Ådahl <jad...@gmail.com>
---
 src/scanner.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/scanner.c b/src/scanner.c
index 5f06e8e..d5442c1 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -42,6 +42,8 @@
 #if HAVE_LIBXML
 #include 
 
+#define PROGRAM_NAME "wayland-scanner"
+
 /* Embedded wayland.dtd file, see dtddata.S */
 extern char DTD_DATA_begin;
 extern int DTD_DATA_len;
@@ -57,8 +59,8 @@ enum side {
 static int
 usage(int ret)
 {
-   fprintf(stderr, "usage: ./scanner [OPTION] 
[client-header|server-header|code]"
-   " [input_file output_file]\n");
+   fprintf(stderr, "usage: %s [OPTION] [client-header|server-header|code]"
+   " [input_file output_file]\n", PROGRAM_NAME);
fprintf(stderr, "\n");
fprintf(stderr, "Converts XML protocol descriptions supplied on "
"stdin or input file to client\n"
@@ -76,7 +78,7 @@ usage(int ret)
 static int
 scanner_version(int ret)
 {
-   fprintf(stderr, "wayland-scanner %s\n", WAYLAND_VERSION);
+   fprintf(stderr, "%s %s\n", PROGRAM_NAME, WAYLAND_VERSION);
exit(ret);
 }
 
@@ -236,7 +238,7 @@ static void *
 fail_on_null(void *p)
 {
if (p == NULL) {
-   fprintf(stderr, "wayland-scanner: out of memory\n");
+   fprintf(stderr, "%s: out of memory\n", PROGRAM_NAME);
exit(EXIT_FAILURE);
}
 
@@ -1467,7 +1469,7 @@ emit_header(struct protocol *protocol, enum side side)
const char *s = (side == SERVER) ? "SERVER" : "CLIENT";
char **p, *prev;
 
-   printf("/* Generated by wayland-scanner %s */\n\n", WAYLAND_VERSION);
+   printf("/* Generated by %s %s */\n\n", PROGRAM_NAME, WAYLAND_VERSION);
 
printf("#ifndef %s_%s_PROTOCOL_H\n"
   "#define %s_%s_PROTOCOL_H\n"
@@ -1670,7 +1672,7 @@ emit_code(struct protocol *protocol)
struct wl_array types;
char **p, *prev;
 
-   printf("/* Generated by wayland-scanner %s */\n\n", WAYLAND_VERSION);
+   printf("/* Generated by %s %s */\n\n", PROGRAM_NAME, WAYLAND_VERSION);
 
if (protocol->copyright)
format_text_to_comment(protocol->copyright, true);
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] scanner: Fix reported executable name to 'wayland-scanner'

[Bryce Harrington]

On Fri, Jun 03, 2016 at 07:33:09PM +1000, Peter Hutterer wrote:
> On Fri, Jun 03, 2016 at 09:16:43AM +0800, Jonas Ådahl wrote:
> > On Thu, Jun 02, 2016 at 02:42:44PM -0700, Bryce Harrington wrote:
> > > 'wayland-scanner -v' (correctly) reports the program as named
> > > "wayland-scanner", but 'wayland-scanner -h' was inconsistent, referring
> > > to it as './scanner'.
> > > 
> > > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > 
> > I guess we could also pass argv and use argv[0], but this works as well.
> 
> can't we use program_invocation_short_name here? or is that to GNU-ish?

We appear to be using that in weston, but I'm not seeing prior usage of
it in wayland.  I'm guessing platform compatibility considerations are
stronger for wayland than weston, particularly for the scanner since it
needs to work everywhere we expect the protocols to be used; whereas in
weston if the demo clients don't work on certain platforms it isn't the
end of the world.

Bryce


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v3] compositor-x11: fix title overflow in x11_backend_create_output

[Bryce Harrington]

On Sun, Jun 05, 2016 at 07:01:11PM +0200, Benoit Gschwind wrote:
> sprintf can overflow the fixed length title which is char[32]. This
> patch change title to dynamically allocated char array using asprintf or
> strdup. If one of them fail we leave returning NULL to indicate the
> failure.
> 
> Signed-of-by: Benoit Gschwind<gschw...@gnu-log.net>

Yeah I had noticed this myself earlier and wondered if it should be
dynamically allocated.  This looks like it would be a possible candidate
for the stable branch if we do a .1.

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

> ---
> v3:
>  - fix xcb_destroy_window arguments
> 
> v2:
>  - fix spacing
>  - properly cleanup everything on failure.
> 
>  src/compositor-x11.c | 28 +++-
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 629b5f3..8727934 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -782,7 +782,7 @@ x11_backend_create_output(struct x11_backend *b, int x, 
> int y,
>  {
>   static const char name[] = "Weston Compositor";
>   static const char class[] = "weston-1\0Weston Compositor";
> - char title[32];
> + char *title = NULL;
>   struct x11_output *output;
>   xcb_screen_t *screen;
>   struct wm_normal_hints normal_hints;
> @@ -800,11 +800,6 @@ x11_backend_create_output(struct x11_backend *b, int x, 
> int y,
>   output_width = width * scale;
>   output_height = height * scale;
>  
> - if (configured_name)
> - sprintf(title, "%s - %s", name, configured_name);
> - else
> - strcpy(title, name);
> -
>   if (!no_input)
>   values[0] |=
>   XCB_EVENT_MASK_KEY_PRESS |
> @@ -871,9 +866,24 @@ x11_backend_create_output(struct x11_backend *b, int x, 
> int y,
>   }
>  
>   /* Set window name.  Don't bother with non-EWMH WMs. */
> - xcb_change_property(b->conn, XCB_PROP_MODE_REPLACE, output->window,
> - b->atom.net_wm_name, b->atom.utf8_string, 8,
> - strlen(title), title);
> + if (configured_name) {
> + if (asprintf(, "%s - %s", name, configured_name) < 0)
> + title = NULL;
> + } else {
> + title = strdup(name);
> + }
> +
> + if (title) {
> + xcb_change_property(b->conn, XCB_PROP_MODE_REPLACE, 
> output->window,
> + b->atom.net_wm_name, b->atom.utf8_string, 8,
> + strlen(title), title);
> + free(title);
> + } else {
> + xcb_destroy_window(b->conn, output->window);
> + free(output);
> + return NULL;
> + }
> +
>   xcb_change_property(b->conn, XCB_PROP_MODE_REPLACE, output->window,
>   b->atom.wm_class, b->atom.string, 8,
>   sizeof class, class);
> -- 
> 2.7.3
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] compositor-wayland: Correct output base make name

[Bryce Harrington]

On Sun, Jun 05, 2016 at 11:01:17AM -0500, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> Change the output make value from "waywayland" to "wayland".
> 
> References: 90bc88c710b34f46ef89e1c5765e5f63f8e02847
> 
> Signed-off-by: Yong Bakos <yba...@humanoriented.com>

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

> ---
>  src/compositor-wayland.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index fe8b082..2e0ea68 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -1005,7 +1005,7 @@ wayland_output_create(struct wayland_backend *b, int x, 
> int y,
>   return NULL;
>  
>   output->name = name ? strdup(name) : NULL;
> - output->base.make = "waywayland";
> + output->base.make = "wayland";
>   output->base.model = "none";
>  
>   output_width = width * scale;
> -- 
> 2.7.2
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Fri, Jun 03, 2016 at 10:39:24AM -0500, Yong Bakos wrote:
> >>> + This destroys the inhibit manager.
> > 
> > Good addition. Should probably say how destroying the manager affects
> > the inhibitors created from it (no effect at all)?
> 
> Agreed w/ pq. But, is the inhibit manager a /singleton/ global?

On the client side yes.

> >>> +  
> >>> +
> >>> +
> >>> +
> >>> +  
> >>> + Create a new inhibitor object associated with the given surface.
> >>> +  
> >>> +  
> >>> +   >>> +summary="the surface that inhibits the idle behavior"/>
> >>> +
> 
> Does it make sense to associate inhibitors to surfaces, rather than clients?
> See below.
> 
> 
> >>> +
> >>> +  
> >>> +
> >>> +  
> >>> +
> >>> +  An idle inhibitor prevents the output that the associated surface 
> >>> is
> >>> +  visible on from being blanked, dimmed, locked, set to power save, 
> >>> or
> >>> +  otherwise obscured due to lack of user interaction.  Any active
> >>> +  screensaver processes are also temporarily blocked from displaying.
> >>> +
> >>> +  If the surface is destroyed, unmapped, becomes occluded or 
> >>> otherwise
> >>> +  loses visibility, the screen behavior is restored to normal; if the
> >>> +  surface subsequently regains visibility the inhibitor takes effect 
> >>> once
> >>> +  again.
> 
> I don't believe this is a good choice. Imagine the case of a surface-less
> 'inhibitor daemon.' There may be no visible surface (is my thinking out
> of scope here?). Imagine another case, that of a "caffeine" widget. This
> widget's surface would be hidden when another app is fullscreen.

This use case did get discussed a bit in previous rounds.  But such
tools were felt to essentially be just crude workarounds for lack of
proper API level control over things.  Sort of in the same class as why
we don't provide an xrandr-equivalent tool for configuring the monitors
- don't provide tools to work around bad designs, just fix the design so
such tools are superfluous.  :-)

> Furthermore, I don't believe that inhibition should be coupled to outputs.
> See below.
> 
> 
> >>> +
> >>> +  Note that in the case of a compound window (a surface with
> >>> +  sub-surfaces), the inhibitor effects should be inherited from the 
> >>> top
> >>> +  level surface.  
> >> 
> >> Does this mean that if we have a subsurface with an inhibitor object,
> >> and for example an xdg_surface without one, the subsurface would still
> >> not inhibit the idle behavior, as it'd inherit the behavior?
> >> 
> >> If it should work like this, maybe we should make it an error to create
> >> an idle inhibitor on a subsurface. If it should inhibit, I think it
> >> needs to be clarified.
> > 
> > Right, I was thinking something like: Sub-surfaces that do not have an
> > inhibitor associated inherit the inhibition behaviour from their
> > parent. This applies recursively.
> > 
> > To clarify what I mean, lets consider an artificial case like this:
> > 
> > wl_surface A is a top-level, only on output 1.
> > wl_surface B is a sub-surface to A, only on output 2.
> > wl_surface C is a sub-surface to B, only on output 3.
> > 
> > wl_surface B has an inhibitor associated. All surfaces are completely
> > visible etc. on their own outputs.
> > 
> > Output 1 will go to powersave: no surface is inhibiting it.
> > 
> > Output 2 does not go to powersave: B inhibitor is in effect.
> > 
> > Output 3 does not go to powersave: C inherits B's inhibitor behaviour.
> 
> I don't believe that inhibition should /only/ be coupled to outputs.
> In the case of a "caffeine" widget or daemon, the use case is to
> prevent /all/ outputs from idling, regardless of what output the widget's
> surface resides upon.

You're right in gathering that you missed some earlier discussion on
this.  :-)  In the original RFC this protocol did provide an
"inhibit idle on all-outputs" type function but in earlier reviews there
just wasn't a solid use case for that so I took it out.

I believe the reason software like you mention operate on all monitors
is just that there isn't a convenient API for doing the per-output level
control.  Back in the day I suppose it was unusual to have more than one
output, so having finer grained control over screensavers and power
saving modes probably didn't make much sense.

Bryce

> > This raises a corner-case: If surface B becomes completely occluded so
> > that the compositor considers it is no longer inhibiting, what happens
> > with output 3? Should it powersave or not?
> > 
> > That is a strange use case but still possible. I'm not sure what should
> > happen. Should it behave like C had its own inhibitor, or should the
> > inheritance take also the effectiveness of the inhibitor on B?
> > 
> > I suppose you could pick either way.
> > 
> >>> +
> >>> +
> >>> +
> >>> +  
> >>> + This removes the inhibitor effect from the associated wl_surface.
> >>> +  
> >>> +
> >>> +
> >>> +  
> >>> 

Re: [PATCH] scanner: Fix reported executable name to 'wayland-scanner'

[Bryce Harrington]

On Fri, Jun 03, 2016 at 06:34:38PM -0500, Yong Bakos wrote:
> Hi,
> 
> On Jun 2, 2016, at 8:16 PM, Jonas Ådahl <jad...@gmail.com> wrote:
> > 
> > On Thu, Jun 02, 2016 at 02:42:44PM -0700, Bryce Harrington wrote:
> >> 'wayland-scanner -v' (correctly) reports the program as named
> >> "wayland-scanner", but 'wayland-scanner -h' was inconsistent, referring
> >> to it as './scanner'.
> >> 
> >> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> 
> Bryce, good catch! But...
> 
> 
> > I guess we could also pass argv and use argv[0], but this works as well.
> > 
> > Reviewed-by: Jonas Ådahl <jad...@gmail.com>
> 
> 
> I don't follow this at all, because...
> 
> 
> >> ---
> >> src/scanner.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/scanner.c b/src/scanner.c
> >> index 5f06e8e..ae2326f 100644
> >> --- a/src/scanner.c
> >> +++ b/src/scanner.c
> >> @@ -57,8 +57,8 @@ enum side {
> >> static int
> >> usage(int ret)
> >> {
> >> -  fprintf(stderr, "usage: ./scanner [OPTION] 
> >> [client-header|server-header|code]"
> >> -  " [input_file output_file]\n");
> >> +  fprintf(stderr, "usage: wayland-scanner [OPTION] 
> >> [client-header|server-header|code]"
> >> +  " [input_file output_file]\n", argv[0]);
> 
> argv is out of scope here in `usage`. This won't compile without changing
> the parameter list, and the passed args at all call sites.

Sorry, I'd been experimenting with using argv initially, but realized
it'd have to be plumbed in as an arg to usage() (and to
scanner_version()), and opted to keep it simple and inline the program
name like scanner_version() does.

> Also, shouldn't the diff be more like (notice the %s format specifier):
> 
> + fprintf(stderr, "usage: %s [OPTION] [client-header|server-header|code]"
> + " [input_file output_file]\n", argv[0]);

> >>fprintf(stderr, "\n");
> >>fprintf(stderr, "Converts XML protocol descriptions supplied on "
> >>"stdin or input file to client\n"
> >> -- 
> >> 1.9.1
> 
> Regards,
> yong
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3 6/8] ivi-shell: Support tracking active surfaces

[Bryce Harrington]

On Thu, May 26, 2016 at 06:02:00PM +0300, Pekka Paalanen wrote:
> On Thu,  7 Apr 2016 16:44:21 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > Activity for ivi-shell follows either click or touch.  Only a single
> > surface can be active in the shell at a time.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> >  Makefile.am   |  4 ++-
> >  configure.ac  |  2 ++
> >  ivi-shell/ivi-shell.c | 10 ++
> >  ivi-shell/ivi-shell.h |  2 ++
> >  src/compositor.c  | 91 
> > ++-
> >  5 files changed, 107 insertions(+), 2 deletions(-)
> 
> Hi,
> 
> this looks like commit squashing accident, I would not expect to find
> the generic server implementation to be found in this patch. Please
> split.
> 
> > diff --git a/Makefile.am b/Makefile.am
> > index cbb3b57..aa3aa1b 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -126,7 +126,9 @@ nodist_weston_SOURCES = 
> > \
> > protocol/scaler-protocol.c  \
> > protocol/scaler-server-protocol.h   \
> > protocol/linux-dmabuf-unstable-v1-protocol.c\
> > -   protocol/linux-dmabuf-unstable-v1-server-protocol.h
> > +   protocol/linux-dmabuf-unstable-v1-server-protocol.h \
> > +   protocol/idle-inhibit-unstable-v1-protocol.c\
> > +   protocol/idle-inhibit-unstable-v1-server-protocol.h
> >  
> >  BUILT_SOURCES += $(nodist_weston_SOURCES)
> >  
> > diff --git a/configure.ac b/configure.ac
> > index bba8050..d4817a9 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -4,6 +4,8 @@ m4_define([weston_micro_version], [90])
> >  m4_define([weston_version],
> >[weston_major_version.weston_minor_version.weston_micro_version])
> >  
> > +# Note: Inhibition patchset requires inhibition protocol in 
> > wayland-protocol
> > +
> >  AC_PREREQ([2.64])
> >  AC_INIT([weston],
> >  [weston_version],
> > diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
> > index 59f5656..9739014 100644
> > --- a/ivi-shell/ivi-shell.c
> > +++ b/ivi-shell/ivi-shell.c
> > @@ -434,11 +434,16 @@ static void
> >  click_to_activate_binding(struct weston_pointer *pointer, uint32_t time,
> >   uint32_t button, void *data)
> >  {
> > +   struct ivi_shell *shell = data;
> > +
> > if (pointer->grab != >default_grab)
> > return;
> > if (pointer->focus == NULL)
> > return;
> >  
> > +   if (shell->active_surface != NULL)
> > +   weston_surface_deactivate(shell->active_surface);
> > +
> > activate_binding(pointer->seat, pointer->focus);
> >  }
> >  
> > @@ -446,11 +451,16 @@ static void
> >  touch_to_activate_binding(struct weston_touch *touch, uint32_t time,
> >   void *data)
> >  {
> > +   struct ivi_shell *shell = data;
> > +
> > if (touch->grab != >default_grab)
> > return;
> > if (touch->focus == NULL)
> > return;
> >  
> > +   if (shell->active_surface != NULL)
> > +   weston_surface_deactivate(shell->active_surface);
> > +
> > activate_binding(touch->seat, touch->focus);
> >  }
> >  
> > diff --git a/ivi-shell/ivi-shell.h b/ivi-shell/ivi-shell.h
> > index 9a05eb2..87bce3a 100644
> > --- a/ivi-shell/ivi-shell.h
> > +++ b/ivi-shell/ivi-shell.h
> > @@ -55,6 +55,8 @@ struct ivi_shell
> > struct wl_resource *binding;
> > struct wl_list surfaces;
> > } input_panel;
> > +
> > +   struct weston_surface *active_surface;
> 
> I suspect this probably should be per-seat... it was the same as
> keyboard focus.
> 
> >  };
> >  
> >  struct weston_view *
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 8e01d38..c89e443 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -51,6 +51,8 @@
> >  #include 
> >  #include 
> >  
> > +#include 
> 
> Use "" as this header is local, not from an installed location.
> 
> > +
> >  #include "timeline.h"
> >  
> >  #include "compositor.h"
> > @@ -2412,7 +2414,7 @@ weston_output_inhibited_outputs(struct 
> > weston_compositor *compositor)
> > continue;
> >  
&

Re: [PATCH weston v3 5/8] fullscreen-shell: Support tracking active surfaces

[Bryce Harrington]

On Thu, May 26, 2016 at 06:01:52PM +0300, Pekka Paalanen wrote:
> On Thu,  7 Apr 2016 16:44:20 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > Surface activity is determined by what surface is being displayed
> > fullscreen.  Only a single surface can be active in the shell.
> 
> Hi,
> 
> only a single surface can be active? But there is a surface for every
> output.
> 
> Shouldn't every visible surface be active in this case, as the shell
> protocol enforces a single visible surface per output policy?

Then in that case can all this just be shortcircuited and make all
surfaces be active?  If I understand what you're saying then by
definition with fullscreen-shell there would be no such thing as
non-active surfaces.

Bryce
 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > ---
> >  fullscreen-shell/fullscreen-shell.c | 27 ++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fullscreen-shell/fullscreen-shell.c 
> > b/fullscreen-shell/fullscreen-shell.c
> > index e1f8a63..8b1930f 100644
> > --- a/fullscreen-shell/fullscreen-shell.c
> > +++ b/fullscreen-shell/fullscreen-shell.c
> > @@ -46,6 +46,7 @@ struct fullscreen_shell {
> > struct wl_listener output_created_listener;
> >  
> > struct wl_listener seat_created_listener;
> > +   struct weston_surface *active_surface;
> >  };
> >  
> >  struct fs_output {
> > @@ -84,14 +85,23 @@ struct pointer_focus_listener {
> >  };
> >  
> >  static void
> > -pointer_focus_changed(struct wl_listener *listener, void *data)
> > +pointer_focus_changed(struct wl_listener *l, void *data)
> >  {
> > struct weston_pointer *pointer = data;
> > +   struct weston_surface *old_surface;
> > +   struct pointer_focus_listener *listener;
> > +
> > +   listener = container_of(l, struct pointer_focus_listener,
> > +   seat_destroyed);
> >  
> > if (pointer->focus && pointer->focus->surface->resource) {
> > +   old_surface = listener->shell->active_surface;
> > +   if (old_surface != NULL)
> > +   weston_surface_deactivate(old_surface);
> > weston_surface_assign_keyboard(pointer->focus->surface, 
> > pointer->seat);
> > if (pointer->focus->surface != NULL)
> > weston_surface_activate(pointer->focus->surface);
> > +   listener->shell->active_surface = pointer->focus->surface;
> 
> So the active surface is the one with the pointer focus?
> What happens when you have multiple weston_seats each with a pointer?
> 
> > }
> >  }
> >  
> > @@ -101,6 +111,7 @@ seat_caps_changed(struct wl_listener *l, void *data)
> > struct weston_seat *seat = data;
> > struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> > struct weston_pointer *pointer = weston_seat_get_pointer(seat);
> > +   struct weston_surface *old_surface;
> > struct pointer_focus_listener *listener;
> > struct fs_output *fsout;
> >  
> > @@ -119,11 +130,15 @@ seat_caps_changed(struct wl_listener *l, void *data)
> > }
> >  
> > if (keyboard && keyboard->focus != NULL) {
> > +   old_surface = listener->shell->active_surface;
> > wl_list_for_each(fsout, >shell->output_list, link) {
> > if (fsout->surface) {
> > +   if (old_surface != NULL)
> > +   weston_surface_deactivate(old_surface);
> > weston_surface_assign_keyboard(fsout->surface, 
> > seat);
> > if (fsout->surface != NULL)
> > weston_surface_activate(fsout->surface);
> > +   listener->shell->active_surface = 
> > fsout->surface;
> > return;
> 
> Seat cap change taken into account, I wouldn't probably have thought of
> that. Very good.
> 
> > }
> > }
> > @@ -676,6 +691,7 @@ fullscreen_shell_present_surface(struct wl_client 
> > *client,
> > wl_resource_get_user_data(resource);
> > struct weston_output *output;
> > struct weston_surface *surface;
> > +   struct weston_surface *old_surface;
> > struct weston_seat *seat;
> > struct fs_output *fsout;
> >  
> > @@ -709,9 +725,13 @@ fulls

Re: [PATCH v2c] weston: catch missing commandline values

[Bryce Harrington]

On Thu, May 12, 2016 at 08:12:09PM -0700, Florian Hänel wrote:
> Sorry for the spam, last version for today.
> Does not make sense to fprintf a value that causes ENOMEM.
> Fixed some more whitespace.
> 
> Florian

> >From 4c390be81da27e7b2331c3e71b848e03173b4632 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Florian=20H=C3=A4nel?= 
> Date: Thu, 12 May 2016 14:40:44 -0700
> Subject: [PATCH] Catch missing commandline values
> 
> Exit if commandline options are missing values.
> 
> Tested with:
> 
> missing = and value
> $ weston --tty
> missing =
> $ weston --tty 2
> missing value
> $ weston --tty=
> no digits found:
> $ weston --tty=ADASDASDAS
> overflow:
> $ weston --tty=99
> hex prefix is allowed:
> $ weston --tty=0x2
> ---
>  shared/option-parser.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/shared/option-parser.c b/shared/option-parser.c
> index 7061268..fb893ee 100644
> --- a/shared/option-parser.c
> +++ b/shared/option-parser.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "config-parser.h"
>  
> @@ -34,20 +35,34 @@ static int
>  handle_option(const struct weston_option *option, char *value)
>  {
>   char* p;
> + if (strlen(value) == 0) {
> + fprintf(stderr, "\nERROR: expected value after %s=\n", 
> option->name);
> + exit(EXIT_FAILURE);
> + }
>  
>   switch (option->type) {
>   case WESTON_OPTION_INTEGER:
>   * (int32_t *) option->data = strtol(value, , 0);
> + if (errno == EINVAL || errno == ERANGE || p == value) goto 
> err_parse;
>   return *value && !*p;
>   case WESTON_OPTION_UNSIGNED_INTEGER:
>   * (uint32_t *) option->data = strtoul(value, , 0);
> + if (errno == EINVAL || errno == ERANGE || p == value) goto 
> err_parse;
>   return *value && !*p;
>   case WESTON_OPTION_STRING:
>   * (char **) option->data = strdup(value);
> + if (errno == ENOMEM) {
> + fprintf(stderr, "ERROR: out of memory parsing 
> commandline\n");
> + exit(EXIT_FAILURE);

The first two cases use a goto for error handling but in this third case
the error handling is inline, which is inconsistent.  Given that there's
multiple error exits already, I'd suggest eliminating the gotos and just
inlining the fprintf and exit.

> + };
>   return 1;
>   default:
> - assert(0);
> + fprintf(stderr, "\nERROR: internal commandline parsing error 
> after option %s.\n",option->name);

Needs a space after the second comma

> + exit(EXIT_FAILURE);

Is there a particular reason for dropping the assert here?  
The only way this branch could be hit is if there is a programming error
prior to the handle_option call.  An assert would stop the debugger at
this point so the issue could be investigated, so I'm not certain it
would be better to exit with an error message.

>   }
> +err_parse:
> + fprintf(stderr, "\nERROR: invalid value (%s) for option %s.\n", 
> value, option->name);
> + exit(EXIT_FAILURE);
>  }
>  
>  static int
> @@ -71,6 +86,9 @@ long_option(const struct weston_option *options, int count, 
> char *arg)
>   }
>   } else if (arg[len+2] == '=') {
>   return handle_option(options + k, arg + len + 3);
> + } else {
> + fprintf(stderr, "\nERROR: expected syntax: 
> --%s=\n", options[k].name);

It looks a bit awkward to have two colons in an error message, maybe
rephrase the error message?

> + exit(EXIT_FAILURE);
>   }
>   }

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] scanner: Fix reported executable name to 'wayland-scanner'

[Bryce Harrington]

'wayland-scanner -v' (correctly) reports the program as named
"wayland-scanner", but 'wayland-scanner -h' was inconsistent, referring
to it as './scanner'.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 src/scanner.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/scanner.c b/src/scanner.c
index 5f06e8e..ae2326f 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -57,8 +57,8 @@ enum side {
 static int
 usage(int ret)
 {
-   fprintf(stderr, "usage: ./scanner [OPTION] 
[client-header|server-header|code]"
-   " [input_file output_file]\n");
+   fprintf(stderr, "usage: wayland-scanner [OPTION] 
[client-header|server-header|code]"
+   " [input_file output_file]\n", argv[0]);
fprintf(stderr, "\n");
fprintf(stderr, "Converts XML protocol descriptions supplied on "
"stdin or input file to client\n"
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Mon, Mar 28, 2016 at 11:10:22AM +0300, Giulio Camuffo wrote:
> 2016-03-24 20:14 GMT+02:00 Bryce Harrington <br...@osg.samsung.com>:
> > This interface allows disabling of screensaver/screenblanking on a
> > per-surface basis.  As long as the surface remains visible and
> > non-occluded it blocks the screensaver, etc. from activating on the
> > output(s) that the surface is visible on.
> >
> > To uninhibit, simply destroy the inhibitor object.
> >
> > Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
> > ---
> > v2: Rename protocol to idle-inhibit
> >  + Use "idle" rather than "screensaver".  People likely to be interested
> >in this protocol would know the difference between "blanking", "dpms",
> >and literal "screensavers", so don't lead them to think that this only
> >deals with the latter.  The protocol addresses all manner of idle
> >behaviors, so is a more accurate name.
> >  + Standardize nomenclature to "idle-manager" and "inhibitor" for clarity.
> >"inhibition_inhibit" was just too cumbersome.
> >
> >  Makefile.am|  1 +
> >  unstable/idle-inhibit/README   |  4 ++
> >  unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 78 
> > ++
> >  3 files changed, 83 insertions(+)
> >  create mode 100644 unstable/idle-inhibit/README
> >  create mode 100644 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 5926a41..ac497d8 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -5,6 +5,7 @@ unstable_protocols =
> > \
> > unstable/text-input/text-input-unstable-v1.xml  
> > \
> > unstable/input-method/input-method-unstable-v1.xml  
> > \
> > unstable/xdg-shell/xdg-shell-unstable-v5.xml
> > \
> > +   unstable/idle-inhibit/idle-inhibit-unstable-v1.xml  \
> > $(NULL)
> >
> >  nobase_dist_pkgdata_DATA = 
> > \
> > diff --git a/unstable/idle-inhibit/README b/unstable/idle-inhibit/README
> > new file mode 100644
> > index 000..396e871
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/README
> > @@ -0,0 +1,4 @@
> > +Screensaver inhibition protocol
> > +
> > +Maintainers:
> > +Bryce Harrington <br...@osg.samsung.com>
> > diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml 
> > b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > new file mode 100644
> > index 000..a8c8a85
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > @@ -0,1 +1,78 @@
> > +
> > +
> > +
> > +  
> > +Copyright © 2015-2016 Samsung Electronics Co., Ltd
> > +
> > +Permission is hereby granted, free of charge, to any person obtaining a
> > +copy of this software and associated documentation files (the 
> > "Software"),
> > +to deal in the Software without restriction, including without 
> > limitation
> > +the rights to use, copy, modify, merge, publish, distribute, 
> > sublicense,
> > +and/or sell copies of the Software, and to permit persons to whom the
> > +Software is furnished to do so, subject to the following conditions:
> > +
> > +The above copyright notice and this permission notice (including the 
> > next
> > +paragraph) shall be included in all copies or substantial portions of 
> > the
> > +Software.
> > +
> > +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> > EXPRESS OR
> > +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> > MERCHANTABILITY,
> > +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> > SHALL
> > +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > +DEALINGS IN THE SOFTWARE.
> > +  
> > +
> > +  
> > +
> > +  This interface permits inhibiting the idle behavior such as screen
> > +  blanking, locking, and screensaving.  The client binds the idle 
> > manager
> >

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Wed, May 18, 2016 at 04:11:39PM +0300, Pekka Paalanen wrote:
> On Thu, 24 Mar 2016 11:14:33 -0700
> Bryce Harrington <br...@osg.samsung.com> wrote:
> 
> > This interface allows disabling of screensaver/screenblanking on a
> > per-surface basis.  As long as the surface remains visible and
> > non-occluded it blocks the screensaver, etc. from activating on the
> > output(s) that the surface is visible on.
> > 
> > To uninhibit, simply destroy the inhibitor object.
> 
> Hi,
> 
> ok, that sounds good.
> 
> > Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
> > ---
> > v2: Rename protocol to idle-inhibit
> >  + Use "idle" rather than "screensaver".  People likely to be interested
> >in this protocol would know the difference between "blanking", "dpms",
> >and literal "screensavers", so don't lead them to think that this only
> >deals with the latter.  The protocol addresses all manner of idle
> >behaviors, so is a more accurate name.
> >  + Standardize nomenclature to "idle-manager" and "inhibitor" for clarity.
> >"inhibition_inhibit" was just too cumbersome.
> 
> Alright!
> 
> > 
> >  Makefile.am|  1 +
> >  unstable/idle-inhibit/README   |  4 ++
> >  unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 78 
> > ++
> >  3 files changed, 83 insertions(+)
> >  create mode 100644 unstable/idle-inhibit/README
> >  create mode 100644 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 5926a41..ac497d8 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -5,6 +5,7 @@ unstable_protocols =
> > \
> > unstable/text-input/text-input-unstable-v1.xml  
> > \
> > unstable/input-method/input-method-unstable-v1.xml  
> > \
> > unstable/xdg-shell/xdg-shell-unstable-v5.xml
> > \
> > +   unstable/idle-inhibit/idle-inhibit-unstable-v1.xml  \
> > $(NULL)
> >  
> >  nobase_dist_pkgdata_DATA = 
> > \
> > diff --git a/unstable/idle-inhibit/README b/unstable/idle-inhibit/README
> > new file mode 100644
> > index 000..396e871
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/README
> > @@ -0,0 +1,4 @@
> > +Screensaver inhibition protocol
> > +
> > +Maintainers:
> > +Bryce Harrington <br...@osg.samsung.com>
> > diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml 
> > b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > new file mode 100644
> > index 000..a8c8a85
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > @@ -0,1 +1,78 @@
> > +
> > +
> > +
> > +  
> > +Copyright © 2015-2016 Samsung Electronics Co., Ltd
> > +
> > +Permission is hereby granted, free of charge, to any person obtaining a
> > +copy of this software and associated documentation files (the 
> > "Software"),
> > +to deal in the Software without restriction, including without 
> > limitation
> > +the rights to use, copy, modify, merge, publish, distribute, 
> > sublicense,
> > +and/or sell copies of the Software, and to permit persons to whom the
> > +Software is furnished to do so, subject to the following conditions:
> > +
> > +The above copyright notice and this permission notice (including the 
> > next
> > +paragraph) shall be included in all copies or substantial portions of 
> > the
> > +Software.
> > +
> > +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> > EXPRESS OR
> > +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> > MERCHANTABILITY,
> > +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> > SHALL
> > +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > +DEALINGS IN THE SOFTWARE.
> > +  
> > +
> > +  
> > +
> > +  This interface permits inhibiting the idle behavior such as screen
> > +  blanking, lockin

[PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

This interface allows disabling of screensaver/screenblanking on a
per-surface basis.  As long as the surface remains visible and
non-occluded it blocks the screensaver, etc. from activating on the
output(s) that the surface is visible on.

To uninhibit, simply destroy the inhibitor object.

Signed-off-by: Bryce Harrington <br...@bryceharrington.org>
---

v2:
 + Rename protocol to idle-inhibit
v3:
 + Added a destructor for the idle manager
 + Changed sub-surface behavior to inherit idle from parent surface
 + Various wording changes suggested by pq


 Makefile.am|  1 +
 unstable/idle-inhibit/README   |  4 ++
 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 84 ++
 3 files changed, 89 insertions(+)
 create mode 100644 unstable/idle-inhibit/README
 create mode 100644 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml

diff --git a/Makefile.am b/Makefile.am
index 71d2632..de691db 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -8,6 +8,7 @@ unstable_protocols =
\
unstable/relative-pointer/relative-pointer-unstable-v1.xml  
\
unstable/pointer-constraints/pointer-constraints-unstable-v1.xml
\
unstable/tablet/tablet-unstable-v1.xml  
\
+   unstable/idle-inhibit/idle-inhibit-unstable-v1.xml  
\
$(NULL)
 
 stable_protocols = 
\
diff --git a/unstable/idle-inhibit/README b/unstable/idle-inhibit/README
new file mode 100644
index 000..396e871
--- /dev/null
+++ b/unstable/idle-inhibit/README
@@ -0,0 +1,4 @@
+Screensaver inhibition protocol
+
+Maintainers:
+Bryce Harrington <br...@osg.samsung.com>
diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml 
b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
new file mode 100644
index 000..af3a911
--- /dev/null
+++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
@@ -0,0 +1,84 @@
+
+
+
+  
+Copyright © 2015 Samsung Electronics Co., Ltd
+
+Permission is hereby granted, free of charge, to any person obtaining a
+copy of this software and associated documentation files (the "Software"),
+to deal in the Software without restriction, including without limitation
+the rights to use, copy, modify, merge, publish, distribute, sublicense,
+and/or sell copies of the Software, and to permit persons to whom the
+Software is furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice (including the next
+paragraph) shall be included in all copies or substantial portions of the
+Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+DEALINGS IN THE SOFTWARE.
+  
+
+  
+
+  This interface permits inhibiting the idle behavior such as screen
+  blanking, locking, and screensaving.  The client binds the idle manager
+  globally, then creates idle-inhibitor objects for each surface.
+
+  Warning! The protocol described in this file is experimental and
+  backward incompatible changes may be made. Backward compatible changes
+  may be added together with the corresponding interface version bump.
+  Backward incompatible changes are done by bumping the version number in
+  the protocol and interface names and resetting the interface version.
+  Once the protocol is to be declared stable, the 'z' prefix and the
+  version number in the protocol and interface names are removed and the
+  interface version number is reset.
+
+
+
+  
+   This destroys the inhibit manager.
+  
+
+
+
+  
+   Create a new inhibitor object associated with the given surface.
+  
+  
+  
+
+
+  
+
+  
+
+  An idle inhibitor prevents the output that the associated surface is
+  visible on from being blanked, dimmed, locked, set to power save, or
+  otherwise obscured due to lack of user interaction.  Any active
+  screensaver processes are also temporarily blocked from displaying.
+
+  If the surface is destroyed, unmapped, becomes occluded or otherwise
+  loses visibility, the screen behavior is restored to normal; if the
+  surface subsequently regains visibility the inhibitor takes effect once
+  again.
+
+  Note that in the case of a compound window (a surface with
+  sub-surfaces), the inhibitor effects 

Re: [PATCH wayland] wayland-server: Clarify included header dependencies

[Bryce Harrington]

On Tue, May 17, 2016 at 09:02:01PM -0600, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> wayland-server.c directly depends on wayland-util.h, and will include
> wayland-server-protocol.h via wayland-server.h.
> 
> Explicitly include wayland-util.h, making this dependency clear.
> Remove the redundant inclusion of wayland-server-protocol.h.
> 
> Signed-off-by: Yong Bakos <yba...@humanoriented.com>
Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

Pushed:
To ssh://git.freedesktop.org/git/wayland/wayland
   972f1a2..d588efc  master -> master


> ---
>  src/wayland-server.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index f745e62..0fe532c 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -43,9 +43,9 @@
>  #include 
>  #include 
>  
> +#include "wayland-util.h"
>  #include "wayland-private.h"
>  #include "wayland-server.h"
> -#include "wayland-server-protocol.h"
>  #include "wayland-os.h"
>  
>  /* This is the size of the char array in struct sock_addr_un.
> -- 
> 2.7.2
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland] event-loop: Make transitive include explicit

[Bryce Harrington]

On Tue, May 17, 2016 at 02:32:02PM +0800, Jonas Ådahl wrote:
> On Mon, May 16, 2016 at 12:05:39PM -0600, Yong Bakos wrote:
> > From: Yong Bakos <yba...@humanoriented.com>
> > 
> > The explicit inclusion of wayland-server.h hides the real dependency, which
> > is wayland-server-core.h.
> > 
> > Signed-off-by: Yong Bakos <yba...@humanoriented.com>
> 
> Reviewed-by: Jonas Ådahl <jad...@gmail.com>

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

Pushed:

To ssh://git.freedesktop.org/git/wayland/wayland
   a1bce0e..972f1a2  master -> master


> >  src/event-loop.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/event-loop.c b/src/event-loop.c
> > index ea27b69..11a9bf2 100644
> > --- a/src/event-loop.c
> > +++ b/src/event-loop.c
> > @@ -37,7 +37,7 @@
> >  #include 
> >  #include 
> >  #include "wayland-private.h"
> > -#include "wayland-server.h"
> > +#include "wayland-server-core.h"
> >  #include "wayland-os.h"
> >  
> >  struct wl_event_loop {
> > -- 
> > 2.7.2
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] weston-launch: Handle invalid command line options

[Bryce Harrington]

On Mon, May 16, 2016 at 12:43:37PM -0600, Yong Bakos wrote:
> On May 7, 2016, at 5:57 AM, Otavio Salvador <ota...@ossystems.com.br> wrote:
> > 
> > From: Tom Hochstein <tom.hochst...@nxp.com>
> > 
> > Exit the program if an unrecognized command line option is found.
> > 
> > Signed-off-by; Tom Hochstein <tom.hochst...@nxp.com>
> > Signed-off-by: Otavio Salvador <ota...@ossystems.com.br>
> 
> Simple enough of a review, and I did test this. But the question is
> whether we want weston-launch to ignore invalid options or to quit in the
> event of their presence. I'm not experienced enough to judge, so others
> will have to chime in. So fwiw,
> 
> Reviewed-by: Yong Bakos <yba...@humanoriented.com>
> Tested-by: Yong Bakos <yba...@humanoriented.com>

Thanks, pushed:

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

To ssh://git.freedesktop.org/git/wayland/weston
   6657fda..e57056f  master -> master


> 
> > ---
> > 
> > src/weston-launch.c | 2 ++
> > 1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/weston-launch.c b/src/weston-launch.c
> > index b8dfb17..9987d8e 100644
> > --- a/src/weston-launch.c
> > +++ b/src/weston-launch.c
> > @@ -703,6 +703,8 @@ main(int argc, char *argv[])
> > case 'h':
> > help("weston-launch");
> > exit(EXIT_FAILURE);
> > +   default:
> > +   exit(EXIT_FAILURE);
> > }
> > }
> > 
> > -- 
> > 2.8.2
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 0/3] doc: Unpublish private objects and functions

[Bryce Harrington]

On Thu, May 12, 2016 at 03:52:36PM -0500, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> The Wayland docbook and the doxygen html docs had been presenting a number
> of private/inaccessible things as part of the public API, which is misleading.
> 
> For 1/3, a \private command was introduced since the function is a private
> wl_display member function. For the xsl rule, I did test both != 'private'
> and = 'public' (thinking of future flexibility) but the doc diff is the same
> and = 'public' is more direct. Note that I placed the \private marker between
> the \sa and \memberof to match the convention that \sa follows a long
> description and \memberof precedes the close of a doc comment.
> 
> In 2-3/3, using the \cond command was sufficient and preferred.
> 
> Yong Bakos (3):
>   doc: Unpublish wl_display_get_additional_shm_formats
>   doc: Unpublish wl_log* and wl_abort
>   doc: Unpublish global_zombie_object and wl_interface_equal
> 
>  doc/publican/doxygen-to-publican.xsl |  2 +-
>  src/wayland-server.c |  2 ++
>  src/wayland-util.c   | 34 +-
>  3 files changed, 20 insertions(+), 18 deletions(-)

In checking the doxygen documentation, these changes all look good, and
near as I can tell the routines in question are in fact private, so for
all three patches:

Reviewed-by: Bryce Harrington <br...@osg.samsung.com>

And as we're now post-release I've gone ahead and pushed them to trunk.

To ssh://git.freedesktop.org/git/wayland/wayland
   98d94b5..a1bce0e  master -> master

Thanks,
Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] Remove Raspberry Pi backend and renderer

[Bryce Harrington]

Given there's an all-FOSS alternative coming down the pike, I see little
reason to hold on to the proprietary-driver dependent backend.

Acked-by: Bryce Harrington <br...@osg.samsung.com>

On Wed, Jun 01, 2016 at 01:11:15PM +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> 
> This patch completely removes the Raspberry Pi backend and the renderer.
> 
> The backend and the renderer were written to use the proprietary
> DispmanX API available only on the Raspberry Pi, to demonstrate what the
> tiny computer is capable of graphics wise. They were also used to
> demonstrate how Wayland and Weston in particular could leverage hardware
> compositing capabilities that are not OpenGL. The backend was first
> added in e8de35c922871bc5b15fbf0436efa233a6db8e41, in 2012.
> 
> Since then, the major point has been proven. Over time, support for the
> rpi-backend diminished, it started to deteriorate and hinder Weston
> development. On May 11, I tried to ask if anyone actually cared about
> the rpi-backend, but did not get any votes for keeping it:
> https://lists.freedesktop.org/archives/wayland-devel/2016-May/028764.html
> 
> The rpi-backend is a good example of how using an API that is only
> available for specific hardware, even more so as it is only available
> with a proprietary driver stack, is not maintainable in the long run.
> Most developers working on Weston either just cannot, or cannot bother
> to test things also on the RPi. Breakage creeps in without anyone
> noticing. If someone actually notices it, fixing it will require a very
> specific environment to be able to test. Also the quality of the
> proprietary implementation fluctuated. There are reports that RPi
> firmware updates randomly broke Weston, and that nowadays it is very
> hard to find a RPi firmware version that you could expect to work with
> Weston if Weston itself was not broken. We are not even sure what is
> broken nowadays.
> 
> This removal does not leave Raspberry Pi users cold (for long), though.
> There is serious work going on in implementing a FOSS driver stack for
> Raspberry Pi, including modern kernel DRM drivers and Mesa drivers. It
> might not be fully there yet, but the plan is to be able to use the
> standard DRM-backend of Weston on the RPis. See:
> http://dri.freedesktop.org/wiki/VC4/
> 
> The rpi-backend had its moments. Now, it needs to go. Good riddance!
> 
> Signed-off-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> ---
>  Makefile.am  |   34 -
>  README   |2 -
>  configure.ac |   18 -
>  man/weston.ini.man   |1 -
>  src/compositor-rpi.c |  575 
>  src/main.c   |   19 -
>  src/rpi-bcm-stubs.h  |  327 -
>  src/rpi-renderer.c   | 1830 
> --
>  src/rpi-renderer.h   |   52 --
>  9 files changed, 2858 deletions(-)
>  delete mode 100644 src/compositor-rpi.c
>  delete mode 100644 src/rpi-bcm-stubs.h
>  delete mode 100644 src/rpi-renderer.c
>  delete mode 100644 src/rpi-renderer.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 00b74e5..8ee9c8d 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -328,40 +328,6 @@ nodist_wayland_backend_la_SOURCES =  
> \
>   protocol/fullscreen-shell-unstable-v1-client-protocol.h
>  endif
>  
> -if ENABLE_RPI_COMPOSITOR
> -if INSTALL_RPI_COMPOSITOR
> -module_LTLIBRARIES += rpi-backend.la
> -else
> -noinst_LTLIBRARIES += rpi-backend.la
> -endif
> -
> -rpi_backend_la_LDFLAGS = -module -avoid-version
> -rpi_backend_la_LIBADD = $(COMPOSITOR_LIBS)   \
> - $(RPI_COMPOSITOR_LIBS)  \
> - $(RPI_BCM_HOST_LIBS)\
> - $(INPUT_BACKEND_LIBS)   \
> - libsession-helper.la\
> - libshared.la
> -rpi_backend_la_CFLAGS =  \
> - $(AM_CFLAGS)\
> - $(COMPOSITOR_CFLAGS)\
> - $(RPI_COMPOSITOR_CFLAGS)\
> - $(RPI_BCM_HOST_CFLAGS)
> -rpi_backend_la_SOURCES = \
> - src/compositor-rpi.c\
> - src/rpi-renderer.c  \
> - src/rpi-renderer.h  \
> - src/rpi-bcm-stubs.h \
> - shared/helpers.h\
> - $(INPUT_BACKEND_SOURCES)
> -
> -if ENABLE_EGL
> -rpi_backend_la_LIBADD += $(EGL_LIBS)
> -rpi_backend_la_CFLAGS += $(EGL_CFLAGS)
> -endif
> -
> -endif
> -
>  if ENABLE_HEADLESS_COMPOSITOR
>  module_LTLIBRARIES += headless-backend.la
>  headless_backend_la_LDFLAGS = -module -avoid-version
>

[ANNOUNCE] wayland 1.11.0

[Bryce Harrington]

This is the official release of Wayland 1.11.  Here's a brief synopsis
of some of the key enhancements provided by this update.

Proxy wrappers were introduced, which help avoid race conditions in
multi-threaded clients.  A new "proxy wrapper" API is added for the
client to use when sending requests, that doesn't proxy events.  This
helps avoid one thread causing events to be dropped before they can be
handled by other threads.  Tests have been added to verify
functionality, and the new functionality is now used in fixing a race
condition present with wl_display_roundtrip_queue().

Wayland's shared memory (shm) received several improvements.  When
undergoing a resize operation while external users are holding
references to it, the resize is deferred to prevent leading to a crash;
Wayland now counts external and internal users differently to permit
tracking this.  Other invalid situations during resizes now emit better
warnings to the log and properly clean up their memory allocations.

As part of Wayland's ongoing work in improving enum for language
bindings, support for cross-interface enum attributes is added.  This
allows the documentation to reference enums defined in other interfaces
using a dot notation.

Documentation now includes HTML generation of doxygen comments in the
source code.  This provides client-side and server-side API
documentation, allowing developers easier reference to Wayland's
functionality from the web.  Each protocol gets a separate doxygen
@page, and each interface its own @subpage; in the case of Wayland there
is just the one core protocol, but for the wayland-protocols package
this will better organize the various content since each will have a
fixed HTML file name so that it is directly linkable/bookmark-able.
A few configuration settings for doxygen were tweaked to enable better
scanning C code.

Other changes this release include:

* Add --version arg for wayland-scanner.

* Add summaries to protocol event parameters.

* Make scanner's stack non-executable, for security purposes.

* Fix configuration with --disable-dtd-validation due to incorrect
  autoconf variable name.
  (Fixes https://bugs.gentoo.org/show_bug.cgi?id=575212)

* Fix a crash when errors are sent involving objects that have already
  been destroyed.  Log messages are changed to report [unknown
  interface] and [unknown id] in this case.  A test case is added to
  cover this as well.

* Add an --enable-fatal-warnings config option for making build warnings
  terminate compilation.  This is not set for build distcheck because
  some distros are overly-pedantic and would terminate building
  unnecessarily.

* Various grammar, spelling, and punctuation cleanup throughout protocol
  documentation.

* Various fixes and enhancements to test cases.

* Various code cleanups related to header includes


Our next major release will be version 1.12, which will be scheduled
tentatively as follows:

  √ Development opens immediately

  - 1.12-alpha in mid-August

  - 1.12-beta in early September

  - 1.12-rc1 in mid-September, with rc2 only if necessary

  - 1.12.0 by the end of September


Changes since RC:
-

Bryce Harrington (1):
  configure.ac: bump to version 1.11.0 for the official release


Full list of changes since 1.10.0:
--

Armin Krezović (1):
  scanner: Add version argument to wayland-scanner

Auke Booij (1):
  protocol: add support for cross-interface enum attributes

Bill Spitzak (1):
  doc: Use enum argument type to make links in protocol documentation

Bryce Harrington (6):
  configure.ac: bump to version 1.10.90 for open development
  doc: Note strong recommendation to use S-o-b in contributions
  configure.ac: bump to version 1.10.91 for the alpha release
  configure.ac: bump to version 1.10.92 for the beta release
  configure.ac: bump to version 1.10.93 for the RC1 release
  configure.ac: bump to version 1.11.0 for the official release

Derek Foreman (9):
  resource-test: Use wl_seat instead of wl_display for testing
  server: validate resource versions at creation time
  build: Add an --enable-fatal-warnings configure option
  build: build distcheck with --enable-fatal-warnings
  Revert "build: build distcheck with --enable-fatal-warnings"
  Revert "server: validate resource versions at creation time"
  shm: Split pool reference counting into external and internal references
  shm: Defer wl_shm_pool_resize if a pool has external references
  shm: Log a warning if a shm buffer address is requested when it may be 
invalid

Emil Velikov (3):
  scanner: move include directives before extern "C" wrapper
  server: move include directives before extern "C" wrapper
  utils: move include directives before extern "C" wrapper

Eric Engestrom (7):
  protocol: fix spelling mis

Re: [PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

[Bryce Harrington]

On Wed, May 18, 2016 at 04:11:39PM +0300, Pekka Paalanen wrote:
> On Thu, 24 Mar 2016 11:14:33 -0700
> 
> Should I go through the v3 Weston patches too, or do you want to revise
> them according to protocol changes first?

Yes, the weston patches are the main thing that haven't gotten reviewed
so far.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[ANNOUNCE] weston 1.10.93

[Bryce Harrington]

 changes.
Various warnings text improvements, whitespace cleanup, spelling fixes,
and grammar corrections.  Various error handling improvements, NULL
Pointer checks, and leak fixes.  Various improvements to usage of
x*alloc and related functions.  Various codebase refactoring, removal of
unneeded functions, casting cleanups, etc.

One crash bug relating to xwayland drag-and-drop is being tracked for
possible inclusion in the release, but no other major changes are
anticipated between now and the release.

See the alpha and beta release announcements for previous changes.

Changes since beta:
---

Bryce Harrington (4):
  ivi: Fix spellings in comments
  build: Define wayland prereq version
  releasing: Update release docs in regards to the wayland versioned 
dependency
  configure.ac: bump to version 1.10.93 for the RC1 release

Emmanuel Gil Peyrot (2):
  zunitc: use platform-independent macros for integer formatting
  desktop-shell: Don’t reconfigure an already fullscreen surface

FORT David (3):
  rdp: allow to compile against FreeRDP 2.0
  rdp: Fix the ContextNew callback with recent FreeRDP versions
  compositor: use generated constant instead of hardcoded value

git tag: 1.10.93
http://wayland.freedesktop.org/releases/weston-1.10.93.tar.xz
MD5:  b981951c8b5c9116aec89581f595971e  weston-1.10.93.tar.xz
SHA1: 6247c9afa046788132708e85e8a6d37f02593159  weston-1.10.93.tar.xz
SHA256: bc671b18cf1872f50c9dc39cd89b3b2dc9f785760d465a51f38b28f925503ee4  
weston-1.10.93.tar.xz
PGP:  http://wayland.freedesktop.org/releases/weston-1.10.93.tar.xz.sig


signature.asc
Description: Digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[ANNOUNCE] wayland 1.10.93

[Bryce Harrington]

This is the first release candidate for wayland 1.11.  There have been
only a few minor changes since the beta; see the end of this note.

To summarize the full changes included in this release over 1.10.0:

Proxy wrappers were introduced, which help avoid race conditions in
multi-threaded clients.  A new "proxy wrapper" API is added for the
client to use when sending requests, that doesn't proxy events.  This
helps avoid one thread causing events to be dropped before they can be
handled by other threads.  Tests have been added to verify
functionality, and the new functionality is now used in fixing a race
condition present with wl_display_roundtrip_queue().

Wayland's shared memory (shm) received several improvements.  When
undergoing a resize operation while external users are holding
references to it, the resize is deferred to prevent leading to a crash;
Wayland now counts external and internal users differently to permit
tracking this.  Other invalid situations during resizes now emit better
warnings to the log and properly clean up their memory allocations.

As part of Wayland's ongoing work in improving enum for language
bindings, support for cross-interface enum attributes is added.  This
allows the documentation to reference enums defined in other interfaces
using a dot notation.

Documentation now includes HTML generation of doxygen comments in the
source code.  This provides client-side and server-side API
documentation, allowing developers easier reference to Wayland's
functionality from the web.  Each protocol gets a separate doxygen
@page, and each interface its own @subpage; in the case of Wayland there
is just the one core protocol, but for the wayland-protocols package
this will better organize the various content since each will have a
fixed HTML file name so that it is directly linkable/bookmark-able.
A few configuration settings for doxygen were tweaked to enable better
scanning C code.

Other changes this release include:

* Add --version arg for wayland-scanner.

* Add summaries to protocol event parameters.

* Make scanner's stack non-executable, for security purposes.

* Fix configuration with --disable-dtd-validation due to incorrect
  autoconf variable name.
  (Fixes https://bugs.gentoo.org/show_bug.cgi?id=575212)

* Fix a crash when errors are sent involving objects that have already
  been destroyed.  Log messages are changed to report [unknown
  interface] and [unknown id] in this case.  A test case is added to
  cover this as well.

* Add an --enable-fatal-warnings config option for making build warnings
  terminate compilation.  This is not set for build distcheck because
  some distros are overly-pedantic and would terminate building
  unnecessarily.

* Various grammar, spelling, and punctuation cleanup throughout protocol
  documentation.

* Various fixes and enhancements to test cases.

* Various code cleanups related to header includes


We're still on target for a release at the end of the month.  There are
no release blocking bugs for Wayland, so I do not anticipate any
deviation from the schedule.

See the alpha and beta release announcements for previous changes.

Changes since previous Beta:


Bryce Harrington (1):
  configure.ac: bump to version 1.10.93 for the RC1 release

Marek Chalupa (1):
  display-test: move a misplaced comment

Yong Bakos (2):
  tests: Check for client/server-core.h inclusion
  scanner: Remove unused forward decs from client protocol

git tag: 1.10.93
http://wayland.freedesktop.org/releases/wayland-1.10.93.tar.xz
MD5:  35c611575015abde5e4d9459f9b36b5a  wayland-1.10.93.tar.xz
SHA1: da0fe199b0211203986d5f29024b40970e01ff7a  wayland-1.10.93.tar.xz
SHA256: a0fbe4e27f6a09ee251e1326f0a17ac8159cdbd894c1e6190ee64c3d37086836  
wayland-1.10.93.tar.xz
PGP:  http://wayland.freedesktop.org/releases/wayland-1.10.93.tar.xz.sig


signature.asc
Description: Digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [ANNOUNCE] wayland 1.10.91

[Bryce Harrington]

On Tue, May 24, 2016 at 09:49:20AM +0800, Jonas Ådahl wrote:
> On Mon, May 23, 2016 at 02:22:45PM -0700, Bryce Harrington wrote:
> > On Fri, May 20, 2016 at 03:35:15PM -0700, Bryce Harrington wrote:
> > > On Wed, May 11, 2016 at 12:11:50PM -0700, Bryce Harrington wrote:
> > > > On Fri, May 06, 2016 at 02:17:23PM +0300, Pekka Paalanen wrote:
> > > > > On Wed, 4 May 2016 11:46:23 -0700
> > > > > Bryce Harrington <br...@osg.samsung.com> wrote:
> > > > > 
> > > > > > On Wed, May 04, 2016 at 12:55:16AM -0700, Bryce Harrington wrote:
> > > > > > > Here's the alpha for the upcoming 1.11 release.  I'll summarize 
> > > > > > > the
> > > > > > > major features for this release in the beta announcement, but see 
> > > > > > > below
> > > > > > > for the detailed listing.
> > > > > > > 
> > > > > > > The schedule going forward is:
> > > > > > > 
> > > > > > >   √ 1.11-alpha on May 3rd.  Major features done by this point.
> > > > 
> > > > It looks like someone (pq?) has marked some patches we decided to leave
> > > > for post-1.11 as deferred in patchwork.  I've followed suit and moved
> > > > other patchsets there which are new APIs or that sound like feature
> > > > work.
> > > 
> > > The patch queue has been fully processed now, except for a few items.
> > > 
> > >   https://patchwork.freedesktop.org/project/wayland/patches/
> > > 
> > > What wasn't landable as-is has been marked as deferred for now, and I
> > > will move them back to the main queue once the release is out.
> > > 
> > > > 3. xwayland drag-and-drop window creation [Carlos]
> > > >- Looks ok to be but xwayland isn't my area
> > > >- Been in the queue a long time, but no reviews to date
> > > >- From the commit description sounds WIP-ish and that there was
> > > >  planned some followup work, but not sure if that happened?
> > > 
> > > https://patchwork.freedesktop.org/patch/74663/
> > > 
> > > I'm still unsure what to do with this drag-and-drop patch.  I'm
> > > defaulting to defer it at this point, but it seems to fix a legitimate
> > > crash bug so I'm reticent to kick it down the road without at least a
> > > next action identified.  It's a short patch, can someone take a quick
> > > look and give an opinion on it?
> > 
> > Due to lack of response I've deferred this one.
> 
> As stated in the other mail thread that brought up this patch, I think
> it seems better to land it now, rather than keep crashing.

I completely agree it would be nice to land this, but someone still
needs to actually respond with a review of the patch.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: writing Was: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Bryce Harrington]

On Wed, May 18, 2016 at 06:33:15PM -0400, Yong Bakos wrote:
> Hi,
> I'm partly to blame for the bikeshedding on writing, yet believe me,
> I'm always asking "is this worth it?", prioritizing corrections/clarity
> over mere style judgements, and am definitely sensitive to
> everyone's cognitive bandwidth. My goal is high quality documentation
> for Wayland, and I am open to suggestions on how to best deliver that.
> 
> (Maybe we can chat about this over IRC rather than further hijack this 
> thread.)
> 
> On May 18, 2016, at 10:31 AM, Daniel Stone  wrote:
> > 
> > Hi,
> > 
> > On 18 May 2016 at 15:25, Derek Foreman  wrote:
> >> On 18/05/16 08:41 AM, Mike Blumenkrantz wrote:
> >>> In fairness, we'd likely be less short on review bandwidth if the
> >>> majority of that bandwidth was not in use to make/revise trivial
> >>> criticisms such as whitespace usage and comment grammar which are
> >>> guaranteed to be cleaned up in future patches; this leads to burnout on
> >>> both the code-writing side as well as the reviewing side since everyone
> >>> has become hyper attuned to the insignificant and non-functional
> >>> minutiae of patches rather than focusing more on expediting the
> >>> technical development of the protocol.
> 
> I agree with the crux of Mike's point. Yet, should trivial correctness
> be revised before merging or should post-merge corrections clutter the
> blame history? (Rhetorical question.)
> 
> 
> >> Fair points, though I'm not certain "will certainly get fixed up later"
> >> is a given.  Certainly indenting and basic style is a mechanical problem
> >> that could be tested pre-commit hooks, and there should be no reason to
> >> bike shed that on the list at all.
> 
> Yes!
> 
> 
> >> Grammar probably needs more serious consideration for protocol doc than
> >> elsewhere due to its potential impact on compositor implementors - but
> >> ever there probably not to the degree we're seeing lately.
> >> 
> >> Follow up commits that do nothing but change style and grammar can make
> >> "git blame" less useful (when trying to figure out who would best review
> >> a piece of code - not just "agh who wrote this stupid bug") and
> >> provide churn for very little benefit, imho.
> > 
> > Yeah, I agree. I get that the bikeshedding can be annoying; I do (for
> > that reason, if no other) like tagging commits as 'RFC' or similar,
> > which is effectively, 'please just check out the technical concept and
> > don't worry about memory leaks or spelling mistakes right now'. But
> > given that it's pretty trivial to fix up, and you're likely to have to
> > rebase _anyway_, I don't see the harm in doing one round of review for
> > clarity.
> > 
> > Generally, there's no need to send out a subsequent revision round
> > just because someone has noted some typos - send it again if you need
> > a resend anyway to get people to pick it up after a rebase, or if
> > there have been notable changes, but you shouldn't be arriving at v17
> > just because you have difficulty spelling.
> 
> Perhaps non-technical suggestions should be sent only directly to the
> author, for consideration in incorporating in the event of subsequent
> revisions, and we not send these to the list? This gives the author
> the benefit of the information, and does not clutter the list with
> non-technical reviews.

No, I don't think we need special rules for such things.  Just send them
to the list like anything else.  If you've done a spelling/grammar check
then posting it publically means the next reviewer knows they don't need
to comment on the same stuff and can focus on something else.

In general, we don't need to bikeshed the bikeshedding.  ;-)

> > Similarly, 'no, I disagree' is a reasonable response to someone
> > bikeshedding your exact choice of variables or naming. Review is meant
> > to be a discussion, not something you just have to unilaterally
> > acqiuesce to.
> > 
> >> While we're drifting just slightly off topic here, I'm also concerned
> >> about the basic usage of some of our tags:
> >> 
> >> Reviewed-by:  indicates rigorous technical review *AND* a firm
> >> conviction that the feature is important and should be included.
> >> 
> >> Acked-by: Indicates a firm conviction that the feature is important and
> >> should be included, but no rigorous review has taken place.
> >> 
> >> Signed-off-by: Indicates an assumption of responsibility for the code.
> 
> The rigor is important. We should add this to:
> https://wayland.freedesktop.org/reviewing.html.

That's all fine but frankly I don't see this as a serious problem in
practice.  The real problem we have is more that patches aren't getting
*enough* R-b's.

When landing patches I do take into account who gave the R-b, and am
aware that each of us has areas we're stronger in than others.  I'm
guessing here that pq takes this into account as well.  I'd rather have
excessive R-b's than none.

A good technique I've seen (and tried to 

Re: [ANNOUNCE] wayland 1.10.91

[Bryce Harrington]

On Fri, May 20, 2016 at 03:35:15PM -0700, Bryce Harrington wrote:
> On Wed, May 11, 2016 at 12:11:50PM -0700, Bryce Harrington wrote:
> > On Fri, May 06, 2016 at 02:17:23PM +0300, Pekka Paalanen wrote:
> > > On Wed, 4 May 2016 11:46:23 -0700
> > > Bryce Harrington <br...@osg.samsung.com> wrote:
> > > 
> > > > On Wed, May 04, 2016 at 12:55:16AM -0700, Bryce Harrington wrote:
> > > > > Here's the alpha for the upcoming 1.11 release.  I'll summarize the
> > > > > major features for this release in the beta announcement, but see 
> > > > > below
> > > > > for the detailed listing.
> > > > > 
> > > > > The schedule going forward is:
> > > > > 
> > > > >   √ 1.11-alpha on May 3rd.  Major features done by this point.
> > 
> > It looks like someone (pq?) has marked some patches we decided to leave
> > for post-1.11 as deferred in patchwork.  I've followed suit and moved
> > other patchsets there which are new APIs or that sound like feature
> > work.
> 
> The patch queue has been fully processed now, except for a few items.
> 
>   https://patchwork.freedesktop.org/project/wayland/patches/
> 
> What wasn't landable as-is has been marked as deferred for now, and I
> will move them back to the main queue once the release is out.
> 
> > 3. xwayland drag-and-drop window creation [Carlos]
> >- Looks ok to be but xwayland isn't my area
> >- Been in the queue a long time, but no reviews to date
> >- From the commit description sounds WIP-ish and that there was
> >  planned some followup work, but not sure if that happened?
> 
> https://patchwork.freedesktop.org/patch/74663/
> 
> I'm still unsure what to do with this drag-and-drop patch.  I'm
> defaulting to defer it at this point, but it seems to fix a legitimate
> crash bug so I'm reticent to kick it down the road without at least a
> next action identified.  It's a short patch, can someone take a quick
> look and give an opinion on it?

Due to lack of response I've deferred this one.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3] configure: unify wayland-client and wayland-server requirements

[Bryce Harrington]

On Mon, May 23, 2016 at 11:43:29AM +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> 
> This introduces a variable common for both wayland-client and
> wayland-server required version number. As most developers usually build
> everything, testing server and client version requirements separately is
> more poorly tested than version requirements in general. The server
> requirement being greater than the client requirement will mask any
> issues from forgetting to bump the client requirement appropriately.
> Therefore the requirements are unified.
> 
> This bumps wayland-client requirement to 1.10.0, on par with the
> existing wayland-server requirement.
> 
> Only the checks which explicitly defined a version are updated,
> unversioned checks are left as is.
> 
> The variable style follows that of Mesa.
> 
> This patch is based on the similar patch by Bryce Harrington that used
> m4_define instead of a variable.

Since that's such a minor change, I'll just squash this to mine.

Bryce
 
> Cc: Bryce Harrington <br...@osg.samsung.com>
> Cc: Quentin Glidic <sardemff7+wayl...@sardemff7.net>
> Signed-off-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> ---
>  configure.ac | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index e1300b4..e4516c1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -11,6 +11,9 @@ AC_INIT([weston],
>  [weston],
>  [http://wayland.freedesktop.org])
>  
> +# External dependency versions
> +WAYLAND_REQUIRED="1.10.0"
> +
>  AC_SUBST([WESTON_VERSION_MAJOR], [weston_major_version])
>  AC_SUBST([WESTON_VERSION_MINOR], [weston_minor_version])
>  AC_SUBST([WESTON_VERSION_MICRO], [weston_micro_version])
> @@ -60,7 +63,7 @@ AC_CHECK_HEADERS([execinfo.h])
>  
>  AC_CHECK_FUNCS([mkostemp strchrnul initgroups posix_fallocate])
>  
> -COMPOSITOR_MODULES="wayland-server >= 1.10.0 pixman-1 >= 0.25.2"
> +COMPOSITOR_MODULES="wayland-server >= $WAYLAND_REQUIRED pixman-1 >= 0.25.2"
>  
>  AC_CONFIG_FILES([doc/doxygen/tools.doxygen doc/doxygen/tooldev.doxygen])
>  
> @@ -193,7 +196,7 @@ AM_CONDITIONAL(ENABLE_WAYLAND_COMPOSITOR,
>  if test x$enable_wayland_compositor = xyes -a x$enable_egl = xyes; then
>AC_DEFINE([BUILD_WAYLAND_COMPOSITOR], [1],
>   [Build the Wayland (nested) compositor])
> -  PKG_CHECK_MODULES(WAYLAND_COMPOSITOR, [wayland-client >= 1.5.91 
> wayland-egl wayland-cursor])
> +  PKG_CHECK_MODULES(WAYLAND_COMPOSITOR, [wayland-client >= $WAYLAND_REQUIRED 
> wayland-egl wayland-cursor])
>  fi
>  
>  
> @@ -335,7 +338,7 @@ AM_CONDITIONAL(ENABLE_VAAPI_RECORDER, test "x$have_libva" 
> = xyes)
>  
>  PKG_CHECK_MODULES(CAIRO, [cairo])
>  
> -PKG_CHECK_MODULES(TEST_CLIENT, [wayland-client >= 1.10.0])
> +PKG_CHECK_MODULES(TEST_CLIENT, [wayland-client >= $WAYLAND_REQUIRED])
>  
>  AC_ARG_ENABLE(simple-clients,
>AS_HELP_STRING([--disable-simple-clients],
> @@ -389,9 +392,9 @@ AM_CONDITIONAL(BUILD_CLIENTS, test x$enable_clients = 
> xyes)
>  if test x$enable_clients = xyes; then
>AC_DEFINE([BUILD_CLIENTS], [1], [Build the Wayland clients])
>  
> -  PKG_CHECK_MODULES(CLIENT, [wayland-client >= 1.5.91 cairo >= 1.10.0 
> xkbcommon wayland-cursor])
> +  PKG_CHECK_MODULES(CLIENT, [wayland-client >= $WAYLAND_REQUIRED cairo >= 
> 1.10.0 xkbcommon wayland-cursor])
>PKG_CHECK_MODULES(SERVER, [wayland-server])
> -  PKG_CHECK_MODULES(WESTON_INFO, [wayland-client >= 1.5.91])
> +  PKG_CHECK_MODULES(WESTON_INFO, [wayland-client >= $WAYLAND_REQUIRED])
>  
># Only check for cairo-egl if a GL or GLES renderer requested
>AS_IF([test "x$cairo_modules" = "xcairo-gl" -o "x$cairo_modules" = 
> "xcairo-glesv2"], [
> -- 
> 2.7.3
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v3] build: Define wayland prereq version

[Bryce Harrington]

Establishes a single variable for defining the libwayland version
requirements.  Enforces the same version dependency between
libwayland-client and libwayland-server, as per the 1.11 release
discussions.  This also sets wayland-client's required version to 1.10,
same as for the server.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 configure.ac | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index e1300b4..d034907 100644
--- a/configure.ac
+++ b/configure.ac
@@ -11,6 +11,8 @@ AC_INIT([weston],
 [weston],
 [http://wayland.freedesktop.org])
 
+WAYLAND_PREREQ_VERSION="1.10.0"
+
 AC_SUBST([WESTON_VERSION_MAJOR], [weston_major_version])
 AC_SUBST([WESTON_VERSION_MINOR], [weston_minor_version])
 AC_SUBST([WESTON_VERSION_MICRO], [weston_micro_version])
@@ -60,7 +62,7 @@ AC_CHECK_HEADERS([execinfo.h])
 
 AC_CHECK_FUNCS([mkostemp strchrnul initgroups posix_fallocate])
 
-COMPOSITOR_MODULES="wayland-server >= 1.10.0 pixman-1 >= 0.25.2"
+COMPOSITOR_MODULES="wayland-server >= $WAYLAND_PREREQ_VERSION pixman-1 >= 
0.25.2"
 
 AC_CONFIG_FILES([doc/doxygen/tools.doxygen doc/doxygen/tooldev.doxygen])
 
@@ -193,7 +195,7 @@ AM_CONDITIONAL(ENABLE_WAYLAND_COMPOSITOR,
 if test x$enable_wayland_compositor = xyes -a x$enable_egl = xyes; then
   AC_DEFINE([BUILD_WAYLAND_COMPOSITOR], [1],
[Build the Wayland (nested) compositor])
-  PKG_CHECK_MODULES(WAYLAND_COMPOSITOR, [wayland-client >= 1.5.91 wayland-egl 
wayland-cursor])
+  PKG_CHECK_MODULES(WAYLAND_COMPOSITOR, [wayland-client >= 
$WAYLAND_PREREQ_VERSION wayland-egl wayland-cursor])
 fi
 
 
@@ -335,7 +337,7 @@ AM_CONDITIONAL(ENABLE_VAAPI_RECORDER, test "x$have_libva" = 
xyes)
 
 PKG_CHECK_MODULES(CAIRO, [cairo])
 
-PKG_CHECK_MODULES(TEST_CLIENT, [wayland-client >= 1.10.0])
+PKG_CHECK_MODULES(TEST_CLIENT, [wayland-client >= $WAYLAND_PREREQ_VERSION])
 
 AC_ARG_ENABLE(simple-clients,
   AS_HELP_STRING([--disable-simple-clients],
@@ -389,9 +391,9 @@ AM_CONDITIONAL(BUILD_CLIENTS, test x$enable_clients = xyes)
 if test x$enable_clients = xyes; then
   AC_DEFINE([BUILD_CLIENTS], [1], [Build the Wayland clients])
 
-  PKG_CHECK_MODULES(CLIENT, [wayland-client >= 1.5.91 cairo >= 1.10.0 
xkbcommon wayland-cursor])
+  PKG_CHECK_MODULES(CLIENT, [wayland-client >= $WAYLAND_PREREQ_VERSION cairo 
>= 1.10.0 xkbcommon wayland-cursor])
   PKG_CHECK_MODULES(SERVER, [wayland-server])
-  PKG_CHECK_MODULES(WESTON_INFO, [wayland-client >= 1.5.91])
+  PKG_CHECK_MODULES(WESTON_INFO, [wayland-client >= $WAYLAND_PREREQ_VERSION])
 
   # Only check for cairo-egl if a GL or GLES renderer requested
   AS_IF([test "x$cairo_modules" = "xcairo-gl" -o "x$cairo_modules" = 
"xcairo-glesv2"], [
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [ANNOUNCE] wayland 1.10.91

[Bryce Harrington]

On Wed, May 11, 2016 at 12:11:50PM -0700, Bryce Harrington wrote:
> On Fri, May 06, 2016 at 02:17:23PM +0300, Pekka Paalanen wrote:
> > On Wed, 4 May 2016 11:46:23 -0700
> > Bryce Harrington <br...@osg.samsung.com> wrote:
> > 
> > > On Wed, May 04, 2016 at 12:55:16AM -0700, Bryce Harrington wrote:
> > > > Here's the alpha for the upcoming 1.11 release.  I'll summarize the
> > > > major features for this release in the beta announcement, but see below
> > > > for the detailed listing.
> > > > 
> > > > The schedule going forward is:
> > > > 
> > > >   √ 1.11-alpha on May 3rd.  Major features done by this point.
> 
> It looks like someone (pq?) has marked some patches we decided to leave
> for post-1.11 as deferred in patchwork.  I've followed suit and moved
> other patchsets there which are new APIs or that sound like feature
> work.

The patch queue has been fully processed now, except for a few items.

  https://patchwork.freedesktop.org/project/wayland/patches/

What wasn't landable as-is has been marked as deferred for now, and I
will move them back to the main queue once the release is out.

> 3. xwayland drag-and-drop window creation [Carlos]
>- Looks ok to be but xwayland isn't my area
>- Been in the queue a long time, but no reviews to date
>- From the commit description sounds WIP-ish and that there was
>  planned some followup work, but not sure if that happened?

https://patchwork.freedesktop.org/patch/74663/

I'm still unsure what to do with this drag-and-drop patch.  I'm
defaulting to defer it at this point, but it seems to fix a legitimate
crash bug so I'm reticent to kick it down the road without at least a
next action identified.  It's a short patch, can someone take a quick
look and give an opinion on it?



https://patchwork.freedesktop.org/patch/88324/

build: Define wayland prereq version

I posted a new version of this with quoting fixed.  This one is a
priority for the release, but given the lateness in the cycle I'd like
to have a R-b or two before I land it.


Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2] build: Define wayland prereq version

[Bryce Harrington]

Establishes a single variable for defining the libwayland version
requirements.  Enforces the same version dependency between
libwayland-client and libwayland-server, as recommended by pq in the
1.11 release discussions.

Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
---
 configure.ac | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index e1300b4..0fd6076 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4,6 +4,8 @@ m4_define([weston_micro_version], [92])
 m4_define([weston_version],
   [weston_major_version.weston_minor_version.weston_micro_version])
 
+m4_define([WAYLAND_PREREQ_VERSION], [1.10.0])
+
 AC_PREREQ([2.64])
 AC_INIT([weston],
 [weston_version],
@@ -60,7 +62,7 @@ AC_CHECK_HEADERS([execinfo.h])
 
 AC_CHECK_FUNCS([mkostemp strchrnul initgroups posix_fallocate])
 
-COMPOSITOR_MODULES="wayland-server >= 1.10.0 pixman-1 >= 0.25.2"
+COMPOSITOR_MODULES="wayland-server >= WAYLAND_PREREQ_VERSION pixman-1 >= 
0.25.2"
 
 AC_CONFIG_FILES([doc/doxygen/tools.doxygen doc/doxygen/tooldev.doxygen])
 
@@ -193,7 +195,7 @@ AM_CONDITIONAL(ENABLE_WAYLAND_COMPOSITOR,
 if test x$enable_wayland_compositor = xyes -a x$enable_egl = xyes; then
   AC_DEFINE([BUILD_WAYLAND_COMPOSITOR], [1],
[Build the Wayland (nested) compositor])
-  PKG_CHECK_MODULES(WAYLAND_COMPOSITOR, [wayland-client >= 1.5.91 wayland-egl 
wayland-cursor])
+  PKG_CHECK_MODULES(WAYLAND_COMPOSITOR, [wayland-client >= 
WAYLAND_PREREQ_VERSION wayland-egl wayland-cursor])
 fi
 
 
@@ -335,7 +337,7 @@ AM_CONDITIONAL(ENABLE_VAAPI_RECORDER, test "x$have_libva" = 
xyes)
 
 PKG_CHECK_MODULES(CAIRO, [cairo])
 
-PKG_CHECK_MODULES(TEST_CLIENT, [wayland-client >= 1.10.0])
+PKG_CHECK_MODULES(TEST_CLIENT, [wayland-client >= WAYLAND_PREREQ_VERSION])
 
 AC_ARG_ENABLE(simple-clients,
   AS_HELP_STRING([--disable-simple-clients],
@@ -389,9 +391,9 @@ AM_CONDITIONAL(BUILD_CLIENTS, test x$enable_clients = xyes)
 if test x$enable_clients = xyes; then
   AC_DEFINE([BUILD_CLIENTS], [1], [Build the Wayland clients])
 
-  PKG_CHECK_MODULES(CLIENT, [wayland-client >= 1.5.91 cairo >= 1.10.0 
xkbcommon wayland-cursor])
+  PKG_CHECK_MODULES(CLIENT, [wayland-client >= WAYLAND_PREREQ_VERSION cairo >= 
1.10.0 xkbcommon wayland-cursor])
   PKG_CHECK_MODULES(SERVER, [wayland-server])
-  PKG_CHECK_MODULES(WESTON_INFO, [wayland-client >= 1.5.91])
+  PKG_CHECK_MODULES(WESTON_INFO, [wayland-client >= WAYLAND_PREREQ_VERSION])
 
   # Only check for cairo-egl if a GL or GLES renderer requested
   AS_IF([test "x$cairo_modules" = "xcairo-gl" -o "x$cairo_modules" = 
"xcairo-glesv2"], [
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] build: Define wayland prereq version

[Bryce Harrington]

On Thu, May 12, 2016 at 09:48:17AM +0100, Emil Velikov wrote:
> On 12 May 2016 at 09:13, Pekka Paalanen <ppaala...@gmail.com> wrote:
> > On Thu, 12 May 2016 11:12:28 +1000
> > Peter Hutterer <peter.hutte...@who-t.net> wrote:
> >
> >> On Wed, May 11, 2016 at 01:18:59PM -0700, Bryce Harrington wrote:
> >> > Establishes a single variable for defining the libwayland version
> >> > requirements.  Enforces the same version dependency between
> >> > libwayland-client and libwayland-server, as recommended by pq in the
> >> > 1.11 release discussions.
> >> >
> >> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> >> > ---
> >> >  configure.ac | 12 +++-
> >> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/configure.ac b/configure.ac
> >> > index 2ca1f4e..0b23fc4 100644
> >> > --- a/configure.ac
> >> > +++ b/configure.ac
> >> > @@ -4,6 +4,8 @@ m4_define([weston_micro_version], [91])
> >> >  m4_define([weston_version],
> >> >
> >> > [weston_major_version.weston_minor_version.weston_micro_version])
> >> >
> >> > +m4_define([WAYLAND_PREREQ_VERSION], "1.10.0")
> >>
> >> how comes the line above uses [] and here you use ""? is that intentional?
> >> (I keep forgetting whether there's a difference between the two in m4)
> >
> > Yeah, I'm not that sure about using a m4 define. It is one way to do
> > it, but the quotes do look suspicious.
> >
> > FWIW, Mesa uses a big list of common dependency variables too, maybe
> > copy that approach?
> >
> > CC'ing Quentin and Emil, they probably know what's good.
> >
> In all honesty I don't know which one is better, so any
> info/references will be appreciated. For the time being (personally)
> I'd stick with the following as it reads a bit easier ;-)
> 
> WAYLAND_PREREQ_VERSION="1.10.0"

Alright, so I've tested several different variations.  I've tested both
using version 1.10.0 (which must pass), and 1.99.0 (which must fail)

   N=10  N=99
m4_define([WAYLAND_PREREQ_VERSION], "1.N.0")   PASS  FAIL  --> Okay
m4_define([WAYLAND_PREREQ_VERSION], [1.N.0])   PASS  FAIL  --> Okay
m4_define([WAYLAND_PREREQ_VERSION], 1.N.0) PASS  FAIL  --> Okay

WAYLAND_PREREQ_VERSION="1.N.0" PASS  PASS  --> Incorrect
WAYLAND_PREREQ_VERSION=1.N.0   PASS  PASS  --> Incorrect
WAYLAND_PREREQ_VERSION=[1.N.0] PASS  PASS  --> Incorrect

In all cases, I've referenced the variable as just
WAYLAND_PREREQ_VERSION in the code.  If I reference it as
$WAYLAND_PREREQ_VERSION then autogen.sh errors indicating a blank string
was substituted.  E.g.:

  configure: error: Package requirements (wayland-server >=  pixman-1 >= 0.25.2 
xkbcommon >= 0.3.0) were not met:
  No package '>=' found
  No package '0.25.2' found

I'd tested a number of other variations prior to settling on the
m4_define() syntax, which is why I'm leaning that direction - I just
couldn't get anything else to work.  So if anyone feels m4_define() to
be the wrong way to do it, I'm happy to try another way but will need
more specific direction.

Regarding the quoting, it doesn't appear to matter what form to use.
I'll go ahead and resubmit the patch with the bracketed form since that
looks like it would be more consistent with the rest of the code, and
sounds like it would be more standard.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 3/5] server: don't proceed in posting no-memory error on client destruction

[Bryce Harrington]

On Mon, May 16, 2016 at 12:22:02PM +0300, Pekka Paalanen wrote:
> On Fri, 13 May 2016 15:01:20 +0200
> Marek Chalupa  wrote:

Hi Marek,

Thanks for providing the test cases to go along with this, much
appreciated.

The first patch in this set looked fine to me, and with pq's r-b I've
landed it for the release.  The remaining patches sound like they need a
bit more work so am deferring them for post-1.11.

Bryce
 
> > When a client is being destroyed, the display_resource is set to NULL.
> > If then some destroy handler calls wl_client_post_no_memory() or
> > wl_resource_post_no_memory() we crash.
> > 
> > Signed-off-by: Marek Chalupa 
> > ---
> >  src/wayland-server.c | 10 ++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index c93a426..b26a48d 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-server.c
> > @@ -546,6 +546,11 @@ wl_client_get_object(struct wl_client *client, 
> > uint32_t id)
> >  WL_EXPORT void
> >  wl_client_post_no_memory(struct wl_client *client)
> >  {
> > +   /* don't send any other errors
> > +* if we are destroying the client */
> > +   if (!client->display_resource)
> > +   return;
> > +
> > wl_resource_post_error(client->display_resource,
> >WL_DISPLAY_ERROR_NO_MEMORY, "no memory");
> >  }
> > @@ -553,6 +558,11 @@ wl_client_post_no_memory(struct wl_client *client)
> >  WL_EXPORT void
> >  wl_resource_post_no_memory(struct wl_resource *resource)
> >  {
> > +   /* don't send any other errors
> > +* if we are destroying the client */
> > +   if (!resource->client->display_resource)
> > +   return;
> > +
> > wl_resource_post_error(resource->client->display_resource,
> >WL_DISPLAY_ERROR_NO_MEMORY, "no memory");
> >  }
> 
> Hi Marek,
> 
> wl_resource_post_error() already checks and escapes if display_resource
> is NULL, but first it uses the passed in resource to get 'client',
> which is where it would crash if there wasn't a display_resource.
> 
> Ok, so the way to trigger this is to send a no-memory from an object
> destroy handler. A little strange, but should work still indeed.
> 
> The alternative to this patch would be to make
> wl_resource_post_error(NULL, ...) not crash, but ABI-wise that is a
> change rather than just a fix.
> 
> Therefore:
> Reviewed-by: Pekka Paalanen 
> 
> 
> Thanks,
> pq

> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2
> 
> iQIVAwUBVzmROiNf5bQRqqqnAQgK/A//Y0XBxWLVdLfxr0mC2koE41ggKbjIfU4A
> l1Hcy5Z0wZFzOQWJr+Wzv7CPrMYFldJRAFYeABl1nSZAuP94ns8ji1Y4gWLQ5goD
> iSo43V6ZN6kKpt07c7lgJwbLJSZ6vDisKAmRsS/6Z4AH+fYy4GGmrsk/aoX1Vh18
> 5SFbDv1RZIoRP3tWhP89Uwa4EzRD+8xgs8YTO9un/78rhE7lknviOYX9R/WsJfFb
> DfzO8hB3sUa912eyrLqHpIPG6PngcUO2UQTiVFaLe76e6DTEM97Yt9IVOMOzkS6Y
> 5+YXyClrjEdc36lTMtlYCxHzjcL+aGhAfUttdixF9j8srpjB/bfON8ufUJStlRwQ
> NMBiPVJYwXM4+XotfxvwalbPG6Et+ZILtbGJKuXtdgigxRtm8zdx5+WhnSnATtT4
> Gf3bxzG+rRvnqi23MQHZkLrsobacY/tH59o0WrrxT4LKLKonD3aIx1Ajo7ohbYWO
> JVSROwMV3JFKCiBigFDaNUSXmCbYejFldOjcPbCJzdRwYuvWnr590NLr7zCSPqS8
> Hm2zfQA+lJ5PRElY1G8TbG5Aqw0rTjM/Zwf/rU1S+yBueryyyxhn/kfedfKR8edu
> 2voLaaXchvtk0BpzmrrWE77ShU1pCVYfX3AwGG51hwKlVHRQ154O4KL8CodOQzYQ
> eA+NsN8HH/k=
> =EW5S
> -END PGP SIGNATURE-


> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel