[PATCH weston 2/3] xwayland: fix spelling mistake

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 xwayland/selection.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xwayland/selection.c b/xwayland/selection.c
index 6f5328d..bd5e28a 100644
--- a/xwayland/selection.c
+++ b/xwayland/selection.c
@@ -117,7 +117,7 @@ weston_wm_get_incr_chunk(struct weston_wm *wm)
dump_property(wm, wm->atom.wl_selection, reply);
 
if (xcb_get_property_value_length(reply) > 0) {
-   /* reply's ownership is transfered to wm, which is responsible
+   /* reply's ownership is transferred to wm, which is responsible
 * for freeing it */
weston_wm_write_property(wm, reply);
} else {
@@ -251,7 +251,7 @@ weston_wm_get_selection_data(struct weston_wm *wm)
free(reply);
} else {
wm->incr = 0;
-   /* reply's ownership is transfered to wm, which is responsible
+   /* reply's ownership is transferred to wm, which is responsible
 * for freeing it */
weston_wm_write_property(wm, reply);
}
-- 
2.8.0

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

[PATCH weston 1/3] zunitc: fix spelling mistake

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 tools/zunitc/inc/zunitc/zunitc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/zunitc/inc/zunitc/zunitc.h b/tools/zunitc/inc/zunitc/zunitc.h
index 0fa78e3..6ac6f39 100644
--- a/tools/zunitc/inc/zunitc/zunitc.h
+++ b/tools/zunitc/inc/zunitc/zunitc.h
@@ -189,7 +189,7 @@ zuc_list_tests(void);
  * character will match any sequence and the '?' character will match any
  * single character.
  * The '-' character at the start of a pattern marks the end of the
- * patterns required to match and the begining of patterns that names
+ * patterns required to match and the beginning of patterns that names
  * must not match.
  * Defaults to use all tests.
  *
-- 
2.8.0

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

[PATCH weston 3/3] client: fix spelling mistake

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 clients/flower.c | 2 +-
 clients/scaler.c | 2 +-
 clients/smoke.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/clients/flower.c b/clients/flower.c
index aafc63c..34287fd 100644
--- a/clients/flower.c
+++ b/clients/flower.c
@@ -110,7 +110,7 @@ resize_handler(struct widget *widget,
 {
struct flower *flower = data;
 
-   /* Dont resize me */
+   /* Don't resize me */
widget_set_size(flower->widget, flower->width, flower->height);
 }
 
diff --git a/clients/scaler.c b/clients/scaler.c
index 8e9a7fa..1fcf2c0 100644
--- a/clients/scaler.c
+++ b/clients/scaler.c
@@ -123,7 +123,7 @@ resize_handler(struct widget *widget,
 {
struct box *box = data;
 
-   /* Dont resize me */
+   /* Don't resize me */
widget_set_size(box->widget, box->width, box->height);
 }
 
diff --git a/clients/smoke.c b/clients/smoke.c
index 77120b4..cedd17f 100644
--- a/clients/smoke.c
+++ b/clients/smoke.c
@@ -260,7 +260,7 @@ resize_handler(struct widget *widget,
 {
struct smoke *smoke = data;
 
-   /* Dont resize me */
+   /* Don't resize me */
widget_set_size(smoke->widget, smoke->width, smoke->height);
 }
 
-- 
2.8.0

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

[PATCH libinput] fix spelling mistakes

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 doc/t440-support.dox | 2 +-
 src/evdev-tablet.c   | 2 +-
 src/evdev.c  | 2 +-
 src/filter.c | 2 +-
 src/libinput.h   | 8 
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/doc/t440-support.dox b/doc/t440-support.dox
index 652a6d0..f4ce3ed 100644
--- a/doc/t440-support.dox
+++ b/doc/t440-support.dox
@@ -70,7 +70,7 @@ digraph top_button_routing
touchpad -> libinput_tp [color="red4"]
 
events_tp [label="other touchpad events"];
-   events_topbutton [label="top sofware button events"];
+   events_topbutton [label="top software button events"];
 
libinput_tp -> events_tp [arrowhead="none"]
libinput_ts -> events_topbutton [color="red4"]
diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c
index 7359881..9a1ac52 100644
--- a/src/evdev-tablet.c
+++ b/src/evdev-tablet.c
@@ -516,7 +516,7 @@ tablet_check_notify_axes(struct tablet_dispatch *tablet,
axes.tilt.x = 0;
axes.tilt.y = 0;
 
-   /* tilt is already coverted to left-handed, so mouse
+   /* tilt is already converted to left-handed, so mouse
 * rotation is converted to left-handed automatically */
} else {
axes.rotation = tablet_handle_artpen_rotation(tablet, device);
diff --git a/src/evdev.c b/src/evdev.c
index a5c965d..6bb8986 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1306,7 +1306,7 @@ fallback_dispatch_create(struct libinput_device *device)
evdev_init_sendevents(evdev_device, dispatch);
 
/* BTN_MIDDLE is set on mice even when it's not present. So
-* we can only use the absense of BTN_MIDDLE to mean something, i.e.
+* we can only use the absence of BTN_MIDDLE to mean something, i.e.
 * we enable it by default on anything that only has L
 * If we have L and no middle, we don't expose it as config
 * option */
diff --git a/src/filter.c b/src/filter.c
index 4c39b0e..cf8996d 100644
--- a/src/filter.c
+++ b/src/filter.c
@@ -690,7 +690,7 @@ touchpad_lenovo_x230_accel_profile(struct motion_filter 
*filter,
 * trial-and-error. No other meaning should be interpreted.
 * The calculation is a compressed form of
 * pointer_accel_profile_linear(), look at the git history of that
-* function for an explaination of what the min/max/etc. does.
+* function for an explanation of what the min/max/etc. does.
 */
speed_in *= X230_MAGIC_SLOWDOWN / X230_TP_MAGIC_LOW_RES_FACTOR;
 
diff --git a/src/libinput.h b/src/libinput.h
index 819efa8..3449611 100644
--- a/src/libinput.h
+++ b/src/libinput.h
@@ -1340,7 +1340,7 @@ libinput_event_gesture_get_scale(struct 
libinput_event_gesture *event);
  *
  * The angle delta is defined as the change in angle of the line formed by
  * the 2 fingers of a pinch gesture. Clockwise rotation is represented
- * by a postive delta, counter-clockwise by a negative delta. If e.g. the
+ * by a positive delta, counter-clockwise by a negative delta. If e.g. the
  * fingers are on the 12 and 6 location of a clock face plate and they move
  * to the 1 resp. 7 location in a single event then the angle delta is
  * 30 degrees.
@@ -2308,7 +2308,7 @@ libinput_get_event(struct libinput *libinput);
  *
  * @param libinput A previously initialized libinput context
  * @return The event type of the next available event or @ref
- * LIBINPUT_EVENT_NONE if no event is availble.
+ * LIBINPUT_EVENT_NONE if no event is available.
  */
 enum libinput_event_type
 libinput_next_event_type(struct libinput *libinput);
@@ -3590,7 +3590,7 @@ enum libinput_config_accel_profile {
  *
  * @param device The device to configure
  *
- * @return A bitmask of all configurable modes availble on this device.
+ * @return A bitmask of all configurable modes available on this device.
  */
 uint32_t
 libinput_device_config_accel_get_profiles(struct libinput_device *device);
@@ -4234,7 +4234,7 @@ enum libinput_config_dwt_state {
  * @ingroup config
  *
  * Check if this device supports configurable disable-while-typing feature.
- * This feature is usally available on built-in touchpads and disables the
+ * This feature is usually available on built-in touchpads and disables the
  * touchpad while typing. See @ref disable-while-typing for details.
  *
  * @param device The device to configure
-- 
2.8.0

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

[PATCH wayland 2/2] wayland-client: fix spelling mistake

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 src/wayland-client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 297c7a5..33033e7 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -1587,7 +1587,7 @@ wl_display_poll(struct wl_display *display, short int 
events)
  * the appropriate event queues. Finally, events on given event queue are
  * dispatched. On failure -1 is returned and errno set appropriately.
  *
- * In a multi threaded enviroment, do not manually wait using poll() (or
+ * In a multi threaded environment, do not manually wait using poll() (or
  * equivalent) before calling this function, as doing so might cause a dead
  * lock. If external reliance on poll() (or equivalent) is required, see
  * wl_display_prepare_read_queue() of how to do so.
@@ -1692,7 +1692,7 @@ wl_display_dispatch_queue_pending(struct wl_display 
*display,
  * the appropriate event queues. Finally, events on the default event queue
  * are dispatched. On failure -1 is returned and errno set appropriately.
  *
- * In a multi threaded enviroment, do not manually wait using poll() (or
+ * In a multi threaded environment, do not manually wait using poll() (or
  * equivalent) before calling this function, as doing so might cause a dead
  * lock. If external reliance on poll() (or equivalent) is required, see
  * wl_display_prepare_read_queue() of how to do so.
-- 
2.8.0

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

[PATCH weston] zunitc: remove `break` after `return`

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 tools/zunitc/src/zuc_base_logger.c |  6 --
 tools/zunitc/src/zunitc_impl.c | 11 ---
 2 files changed, 17 deletions(-)

diff --git a/tools/zunitc/src/zuc_base_logger.c 
b/tools/zunitc/src/zuc_base_logger.c
index 1beb60d..cdbd9ea 100644
--- a/tools/zunitc/src/zuc_base_logger.c
+++ b/tools/zunitc/src/zuc_base_logger.c
@@ -310,22 +310,16 @@ zuc_get_opstr(enum zuc_check_op op)
switch (op) {
case ZUC_OP_EQ:
return "=";
-   break;
case ZUC_OP_NE:
return "!=";
-   break;
case ZUC_OP_GE:
return ">=";
-   break;
case ZUC_OP_GT:
return ">";
-   break;
case ZUC_OP_LE:
return "<=";
-   break;
case ZUC_OP_LT:
return "<";
-   break;
default:
return "???";
}
diff --git a/tools/zunitc/src/zunitc_impl.c b/tools/zunitc/src/zunitc_impl.c
index 6f591f0..58a5b17 100644
--- a/tools/zunitc/src/zunitc_impl.c
+++ b/tools/zunitc/src/zunitc_impl.c
@@ -1066,7 +1066,6 @@ spawn_test(struct zuc_test *test, void *test_data,
 
zuc_cleanup();
exit(rc);
-   break;
}
default: { /* parent */
ssize_t rc = 0;
@@ -1519,40 +1518,30 @@ get_pred2(enum zuc_check_op op, enum zuc_check_valtype 
valtype)
switch (op) {
case ZUC_OP_TRUE:
return pred2_true;
-   break;
case ZUC_OP_FALSE:
return pred2_false;
-   break;
case ZUC_OP_NULL:
return pred2_false;
-   break;
case ZUC_OP_NOT_NULL:
return pred2_true;
-   break;
case ZUC_OP_EQ:
if (valtype == ZUC_VAL_CSTR)
return pred2_streq;
else
return pred2_eq;
-   break;
case ZUC_OP_NE:
if (valtype == ZUC_VAL_CSTR)
return pred2_strne;
else
return pred2_ne;
-   break;
case ZUC_OP_GE:
return pred2_ge;
-   break;
case ZUC_OP_GT:
return pred2_gt;
-   break;
case ZUC_OP_LE:
return pred2_le;
-   break;
case ZUC_OP_LT:
return pred2_lt;
-   break;
default:
return pred2_unknown;
}
-- 
2.8.0

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

Re: [PATCH wayland] connection: remove redundant assignment

[Eric Engestrom]

On Fri, Apr 22, 2016 at 05:46:00PM +0200, Marek Chalupa wrote:
> the code is something like:
> 

Maybe add:
struct wl_object *object;
to your abridged version to clearly show that it's a local object?

>   if (object == NULL && ...) {
>   object = NULL;
>   return;
>   }
> 
> first, the object is already NULL, second, the assignment has no effect
> since we return from the function right away
> 
> Signed-off-by: Marek Chalupa <mchqwe...@gmail.com>

Either way, it's
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

[PATCH wayland 1/5] client: fix typo

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 src/wayland-client.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 7af806c..03c087a 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -581,7 +581,7 @@ create_outgoing_proxy(struct wl_proxy *proxy, const struct 
wl_message *message,
  *
  * For new-id arguments, this function will allocate a new wl_proxy
  * and send the ID to the server.  The new wl_proxy will be returned
- * on success or NULL on errror with errno set accordingly.  The newly
+ * on success or NULL on error with errno set accordingly.  The newly
  * created proxy will inherit their version from their parent.
  *
  * \note This is intended to be used by language bindings and not in
@@ -616,7 +616,7 @@ wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
  *
  * For new-id arguments, this function will allocate a new wl_proxy
  * and send the ID to the server.  The new wl_proxy will be returned
- * on success or NULL on errror with errno set accordingly.  The newly
+ * on success or NULL on error with errno set accordingly.  The newly
  * created proxy will have the version specified.
  *
  * \note This is intended to be used by language bindings and not in
@@ -711,7 +711,7 @@ wl_proxy_marshal(struct wl_proxy *proxy, uint32_t opcode, 
...)
  *
  * For new-id arguments, this function will allocate a new wl_proxy
  * and send the ID to the server.  The new wl_proxy will be returned
- * on success or NULL on errror with errno set accordingly.  The newly
+ * on success or NULL on error with errno set accordingly.  The newly
  * created proxy will inherit their version from their parent.
  *
  * \note This should not normally be used by non-generated code.
@@ -749,7 +749,7 @@ wl_proxy_marshal_constructor(struct wl_proxy *proxy, 
uint32_t opcode,
  *
  * For new-id arguments, this function will allocate a new wl_proxy
  * and send the ID to the server.  The new wl_proxy will be returned
- * on success or NULL on errror with errno set accordingly.  The newly
+ * on success or NULL on error with errno set accordingly.  The newly
  * created proxy will have the version specified.
  *
  * \note This should not normally be used by non-generated code.
-- 
2.8.2

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

[PATCH wayland 3/5] util: fix typo

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 src/wayland-util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wayland-util.h b/src/wayland-util.h
index cc1999d..8da156c 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -300,7 +300,7 @@ union wl_argument {
  * A dispatcher takes five arguments:  The first is the dispatcher-specific
  * implementation data associated with the target object.  The second is the
  * object on which the callback is being invoked (either wl_proxy or
- * wl_resource).  The third and fourth arguments are the opcode the wl_messsage
+ * wl_resource).  The third and fourth arguments are the opcode the wl_message
  * structure corresponding to the callback being emitted.  The final argument
  * is an array of arguments received from the other process via the wire
  * protocol.
-- 
2.8.2

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

[PATCH wayland 5/5] tests: fix typo

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 tests/test-runner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-runner.c b/tests/test-runner.c
index 742b4f0..4aa6667 100644
--- a/tests/test-runner.c
+++ b/tests/test-runner.c
@@ -205,7 +205,7 @@ run_test(const struct test *t)
 
t->run();
 
-   /* turn off timeout (if any) after test completition */
+   /* turn off timeout (if any) after test completion */
if (timeouts_enabled)
alarm(0);
 
-- 
2.8.2

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

[PATCH wayland 2/5] server: fix typo

[Eric Engestrom]

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 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 ae9365f..f745e62 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1308,7 +1308,7 @@ wl_display_add_socket_fd(struct wl_display *display, int 
sock_fd)
  * fails and returns -1.
  *
  * The length of socket path, i.e., the path set in XDG_RUNTIME_DIR and the
- * socket name, must not exceed the maxium length of a Unix socket path.
+ * socket name, must not exceed the maximum length of a Unix socket path.
  * The function also fails if the user do not have write permission in the
  * XDG_RUNTIME_DIR path or if the socket name is already in use.
  *
-- 
2.8.2

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

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

[Eric Engestrom]

On Wed, Jul 13, 2016 at 07:01:28PM -0700, Bryce Harrington wrote:
> 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/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;
> + }

Looks good to me, except I would put some validation on the expected
input string, eg:

switch (strlen(entry->value))
{
case 10:
if (entry->value[0] != '0' || entry->value[1] != 'x')
goto invalid;
entry->value += 2;

case 8:
size_t i;
for (i = 0; i < 8; i++)
if (!isxdigit(entry->value[i]))
goto invalid;
break;

default:
goto invalid;
}

/* ... */

invalid:
*color = default_color;
errno = EINVAL;
    return -1;


Additionally, I agree with Quentin, since you made a special function
for colors, we might as well have it support more color formats, but IMO
that should be a followup patch.

So, with the added input validation (and 7-char example removed from the
commit message), this patch is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> +
> + return 0;
> +}
> +
> +WL_EXPORT
> +int
>  weston_config_section_get_double(struct weston_config_section *section,
>const char *key,
>double *value, double default_value)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3 4/5] Add safe_strtoint() helper

[Eric Engestrom]

On Mon, Aug 01, 2016 at 12:48:21PM +1000, Peter Hutterer wrote:
> On Fri, Jul 29, 2016 at 09:43:59AM -0700, Bryce Harrington wrote:
> > Adds a safe strtol helper function, modeled loosely after Wayland
> > scanner's strtouint.  This encapsulates the various quirks of strtol
> > behavior, and streamlines the interface to just handling base-10 numbers
> > with a simple true/false error indicator and a uint32_t return by
> > reference.
> > 
> > Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> > Reviewed-by: Thiago Macieira <thiago.macie...@intel.com>
> > ---
> >  shared/string-helpers.h | 78 
> > +
> >  1 file changed, 78 insertions(+)
> >  create mode 100644 shared/string-helpers.h
> > 
> > diff --git a/shared/string-helpers.h b/shared/string-helpers.h
> > new file mode 100644
> > index 000..0617229
> > --- /dev/null
> > +++ b/shared/string-helpers.h
> > @@ -0,0 +1,78 @@
> > +/*
> > + * Copyright © 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.
> > + */
> > +
> > +#ifndef WESTON_STRING_HELPERS_H
> > +#define WESTON_STRING_HELPERS_H
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> 
> do we need this in an internal helper?
> 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* Convert string to integer
> > + *
> > + * Parses a base-10 number from the given string.  Checks that the
> > + * string is not blank, contains only numerical characters, and is
> > + * within the range of -INT_MAX to INT_MAX.  If the validation is

s/-INT_MAX/INT_MIN/, and if I'm being pedantic it should be
"INT32_MIN to INT32_MAX" since you're using int32_t, not int :)

> > + * successful the result is stored in *value; otherwise *value is
> > + * unchanged and errno is set appropriately.
> > + *
> > + * \return true if number parsed successfully, false on error
> 
> "if the number". IMO you should squash the safe_strtoint() tests into this
> patch and have a separate patch for switching the existing calls over. 5/5
> looks like a rebase gone wrong.
> 
> with that, Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>

Agreed, and you can also have my r-b (for the whole series):
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> 
> Cheers,
>Peter
> 
> > + */
> > +static inline bool
> > +safe_strtoint(const char *str, int32_t *value)
> > +{
> > +   long ret;
> > +   char *end;
> > +
> > +   assert(str != NULL);
> > +
> > +   errno = 0;
> > +   ret = strtol(str, , 10);
> > +   if (errno != 0) {
> > +   return false;
> > +   } else if (end == str || *end != '\0') {
> > +   errno = EINVAL;
> > +   return false;
> > +   }
> > +
> > +   *value = (int32_t)ret;
> > +   if ((long)*value != ret) {
> > +   errno = ERANGE;
> > +   return false;
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* WESTON_STRING_HELPERS_H */
> > -- 
> > 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

[Eric Engestrom]

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

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

[Eric Engestrom]

On Fri, Jul 15, 2016 at 10:13:03AM +0100, Eric Engestrom wrote:
> On Thu, Jul 14, 2016 at 11:30:39AM -0700, Bryce Harrington wrote:
> > On Thu, Jul 14, 2016 at 01:35:09PM +0100, Eric Engestrom wrote:
> > > On Wed, Jul 13, 2016 at 07:01:28PM -0700, Bryce Harrington wrote:
> > > > 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/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;
> > > > +   }
> > > 
> > > Looks good to me, except I would put some validation on the expected
> > > input string, eg:
> > > 
> > >   switch (strlen(entry->value))
> > >   {
> > >   case 10:
> > >   if (entry->value[0] != '0' || entry->value[1] != 'x')
> > >   goto invalid;
> > >   entry->value += 2;
> > >   case 8:
> > >   size_t i;
> > >   for (i = 0; i < 8; i++)
> > >   if (!isxdigit(entry->value[i]))
> > >   goto invalid;
> > >   break;
> > > 
> > >   default:
> > > 

Re: [PATCH wayland] (multiple): Include stdint.h

[Eric Engestrom]

On Mon, Jul 18, 2016 at 12:42:25PM -0500, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> Some headers and source files have been using types such as uint32_t
> without explicitly including stdint.h.
> 
> Explicitly include stdint.h where appropriate.
> 
> Signed-off-by: Yong Bakos <yba...@humanoriented.com>

Matches my grep.
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  cursor/cursor-data.h  | 2 ++
>  cursor/wayland-cursor.c   | 1 +
>  src/event-loop.c  | 1 +
>  src/scanner.c | 1 +
>  src/wayland-client-core.h | 1 +
>  src/wayland-private.h | 1 +
>  src/wayland-server.h  | 1 +
>  src/wayland-shm.c | 1 +
>  tests/connection-test.c   | 1 +
>  tests/display-test.c  | 1 +
>  tests/event-loop-test.c   | 1 +
>  tests/map-test.c  | 1 +
>  tests/os-wrappers-test.c  | 1 +
>  tests/queue-test.c| 1 +
>  tests/resources-test.c| 1 +
>  tests/test-compositor.c   | 1 +
>  tests/test-compositor.h   | 1 +
>  17 files changed, 18 insertions(+)
> 
> diff --git a/cursor/cursor-data.h b/cursor/cursor-data.h
> index 0d03cd5..dd7a80a 100644
> --- a/cursor/cursor-data.h
> +++ b/cursor/cursor-data.h
> @@ -25,6 +25,8 @@
>  * Author:  Keith Packard, SuSE, Inc.
>  */
>  
> +#include 
> +
>  static uint32_t cursor_data[] = {
>   0x, 0x, 0x, 0x, 0x, 0x,
>   0x, 0x, 0x, 0x, 0x, 0x,
> diff --git a/cursor/wayland-cursor.c b/cursor/wayland-cursor.c
> index 18abe87..d40c5c8 100644
> --- a/cursor/wayland-cursor.c
> +++ b/cursor/wayland-cursor.c
> @@ -29,6 +29,7 @@
>  #include "wayland-client.h"
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/src/event-loop.c b/src/event-loop.c
> index 32216a7..6130d2a 100644
> --- a/src/event-loop.c
> +++ b/src/event-loop.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/src/scanner.c b/src/scanner.c
> index ebae4cc..d501ba7 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h
> index dfd3e18..03e781b 100644
> --- a/src/wayland-client-core.h
> +++ b/src/wayland-client-core.h
> @@ -26,6 +26,7 @@
>  #ifndef WAYLAND_CLIENT_CORE_H
>  #define WAYLAND_CLIENT_CORE_H
>  
> +#include 
>  #include "wayland-util.h"
>  #include "wayland-version.h"
>  
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 045109b..adfbe01 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -30,6 +30,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #define WL_HIDE_DEPRECATED 1
>  
> diff --git a/src/wayland-server.h b/src/wayland-server.h
> index a6e7951..f11fb4d 100644
> --- a/src/wayland-server.h
> +++ b/src/wayland-server.h
> @@ -36,6 +36,7 @@
>  #ifndef WAYLAND_SERVER_H
>  #define WAYLAND_SERVER_H
>  
> +#include 
>  #include "wayland-server-core.h"
>  
>  #ifdef  __cplusplus
> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
> index 20bb8c8..7fea364 100644
> --- a/src/wayland-shm.c
> +++ b/src/wayland-shm.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 5d97fd9..3e34f77 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/tests/display-test.c b/tests/display-test.c
> index 17956db..f4800ce 100644
> --- a/tests/display-test.c
> +++ b/tests/display-test.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/tests/event-loop-test.c b/tests/event-loop-test.c
> index ae5ff94..33566b4 100644
> --- a/tests/event-loop-test.c
> +++ b/tests/event-loop-test.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/tests/map-test.c b/tests/map-test.c
> index d259048..8ecc1aa 100644
> --- a/tests/map-test.c
> +++ b/tests/map-test.c
> @@ -25,6 +25,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "wayland-priv

Re: [PATCH libinput 5/6] touchpad: change palm detection trigger functions to bools

[Eric Engestrom]

On Tue, Jul 19, 2016 at 10:49:28AM +1000, Peter Hutterer wrote:
> And rename to make it more obvious what the return value means.
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> ---
>  src/evdev-mt-touchpad.c | 28 +++-
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 7ee86a9..190448b 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -573,15 +573,17 @@ tp_palm_tap_is_palm(const struct tp_dispatch *tp, const 
> struct tp_touch *t)
>   return false;
>  }
>  
> -static int
> -tp_palm_detect_dwt(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +static bool
> +tp_palm_detect_dwt_triggered(struct tp_dispatch *tp,
> +  struct tp_touch *t,
> +  uint64_t time)
>  {
>   if (tp->dwt.dwt_enabled &&
>   tp->dwt.keyboard_active &&
>   t->state == TOUCH_BEGIN) {
>   t->palm.state = PALM_TYPING;
>   t->palm.first = t->point;
> - return 1;
> + return true;
>   } else if (!tp->dwt.keyboard_active &&
>  t->state == TOUCH_UPDATE &&
>  t->palm.state == PALM_TYPING) {
> @@ -599,22 +601,22 @@ tp_palm_detect_dwt(struct tp_dispatch *tp, struct 
> tp_touch *t, uint64_t time)
>   }
>   }
>  
> - return 0;
> + return false;
>  }
>  
> -static int
> -tp_palm_detect_trackpoint(struct tp_dispatch *tp,
> -   struct tp_touch *t,
> -   uint64_t time)
> +static bool
> +tp_palm_detect_trackpoint_triggered(struct tp_dispatch *tp,
> + struct tp_touch *t,
> + uint64_t time)
>  {
>   if (!tp->palm.monitor_trackpoint)
> - return 0;
> + return false;
>  
>   if (t->palm.state == PALM_NONE &&
>   t->state == TOUCH_BEGIN &&
>   tp->palm.trackpoint_active) {
>   t->palm.state = PALM_TRACKPOINT;
> - return 1;
> + return true;
>   } else if (t->palm.state == PALM_TRACKPOINT &&
>  t->state == TOUCH_UPDATE &&
>  !tp->palm.trackpoint_active) {
> @@ -627,7 +629,7 @@ tp_palm_detect_trackpoint(struct tp_dispatch *tp,
>   }
>   }
>  
> - return 0;
> + return false;
>  }
>  
>  static inline bool
> @@ -684,10 +686,10 @@ static void
>  tp_palm_detect(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
>  {
>  
> - if (tp_palm_detect_dwt(tp, t, time))
> + if (tp_palm_detect_dwt_triggered(tp, t, time))
>   goto out;
>  
> - if (tp_palm_detect_trackpoint(tp, t, time))
> + if (tp_palm_detect_trackpoint_triggered(tp, t, time))
>   goto out;
>  
>   if (t->palm.state == PALM_EDGE) {
> -- 
> 2.7.4

I feel like the "detect" + "triggered" combo is a bit confusing, and
`tp_palm_dwt_triggered()` & `tp_palm_trackpoint_triggered()` might be
better names.

Either way this whole series is welcome, and is
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

BTW I was reading an article a few days ago about what a mess the
success-like return values are in the C world. Maybe you read it too? ^^

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

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

[Eric Engestrom]

On Thu, Jul 14, 2016 at 11:30:39AM -0700, Bryce Harrington wrote:
> On Thu, Jul 14, 2016 at 01:35:09PM +0100, Eric Engestrom wrote:
> > On Wed, Jul 13, 2016 at 07:01:28PM -0700, Bryce Harrington wrote:
> > > 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/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;
> > > + }
> > 
> > Looks good to me, except I would put some validation on the expected
> > input string, eg:
> > 
> > switch (strlen(entry->value))
> > {
> > case 10:
> > if (entry->value[0] != '0' || entry->value[1] != 'x')
> > goto invalid;
> > entry->value += 2;
> > case 8:
> > size_t i;
> > for (i = 0; i < 8; i++)
> > if (!isxdigit(entry->value[i]))
> > goto invalid;
> > break;
> > 
> > default:
> > goto invalid;
> > }
> 
> I like the extra checking, however I think these might be redundant with
> strtoul's existing checks?  In the case of a ten digit number, that
> should trip the ERANGE error; and in the case of non-valid digits
> present in the 8 character length string, shouldn't that cause strtoul
> to point *end at that character in the string?  Our *end test in the if
> clause should catch that I think.
> 
> But your idea of verifying the string length to be exactly 8 or 10
> characters could be good.

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

[Eric Engestrom]

On Mon, Jul 11, 2016 at 05:02:44PM -0700, Bryce Harrington 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>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.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 weston] shared: include stdint.h for int32_t

[Eric Engestrom]

On Sat, Jul 16, 2016 at 11:32:49PM +0300, Jussi Kukkonen wrote:
> This fixes build on musl.
> 
> Signed-off-by: Jussi Kukkonen <jussi.kukko...@intel.com>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  shared/xalloc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/shared/xalloc.h b/shared/xalloc.h
> index 85fccb4..484de2d 100644
> --- a/shared/xalloc.h
> +++ b/shared/xalloc.h
> @@ -30,6 +30,7 @@
>  extern "C" {
>  #endif
>  
> +#include 
>  #include 
>  #include 
>  
> -- 
> 2.1.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] shared: include stdint.h for int32_t

[Eric Engestrom]

On Mon, Jul 18, 2016 at 09:53:20AM +0100, Eric Engestrom wrote:
> On Sat, Jul 16, 2016 at 11:32:49PM +0300, Jussi Kukkonen wrote:
> > This fixes build on musl.
> > 
> > Signed-off-by: Jussi Kukkonen <jussi.kukko...@intel.com>
> 
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Actually, a quick grep tells me 92 other files could use the same
treatment:

clients/editor.c
clients/eventdemo.c
clients/ivi-shell-user-interface.c
clients/keyboard.c
clients/multi-resource.c
clients/nested-client.c
clients/presentation-shm.c
clients/scaler.c
clients/simple-damage.c
clients/simple-dmabuf-intel.c
clients/simple-dmabuf-v4l.c
clients/simple-egl.c
clients/simple-shm.c
clients/simple-touch.c
clients/stacking.c
clients/weston-info.c
clients/weston-simple-im.c
clients/window.h
compositor/cms-colord.c
compositor/cms-helper.c
compositor/main.c
compositor/screen-share.c
compositor/text-backend.c
compositor/weston-screenshooter.c
desktop-shell/exposay.c
desktop-shell/input-panel.c
desktop-shell/shell.c
desktop-shell/shell.h
fullscreen-shell/fullscreen-shell.c
ivi-shell/hmi-controller.c
ivi-shell/input-panel-ivi.c
ivi-shell/ivi-layout.c
ivi-shell/ivi-layout-export.h
ivi-shell/ivi-layout-private.h
ivi-shell/ivi-layout-transition.c
ivi-shell/ivi-shell.c
ivi-shell/ivi-shell.h
libweston/animation.c
libweston/bindings.c
libweston/clipboard.c
libweston/compositor-drm.c
libweston/compositor-fbdev.c
libweston/compositor-fbdev.h
libweston/compositor.h
libweston/compositor-headless.c
libweston/compositor-headless.h
libweston/compositor-rdp.c
libweston/compositor-wayland.c
libweston/compositor-wayland.h
libweston/compositor-x11.c
libweston/compositor-x11.h
libweston/data-device.c
libweston/dbus.c
libweston/gl-renderer.c
libweston/gl-renderer.h
libweston/launcher-logind.c
libweston/launcher-util.c
libweston/launcher-weston-launch.c
libweston/libbacklight.c
libweston/libinput-device.c
libweston/libinput-seat.c
libweston/linux-dmabuf.c
libweston/noop-renderer.c
libweston/pixman-renderer.c
libweston/screenshooter.c
libweston/spring-tool.c
libweston/timeline.c
libweston/zoom.c
shared/config-parser.c
shared/config-parser.h
shared/frame.c
shared/image-loader.c
shared/xalloc.c
tests/buffer-count-test.c
tests/internal-screenshot-test.c
tests/ivi_layout-internal-test.c
tests/ivi_layout-test.c
tests/ivi_layout-test-plugin.c
tests/keyboard-test.c
tests/presentation-test.c
tests/surface-global-test.c
tests/surface-screenshot.c
tests/text-test.c
tests/weston-test.c
tests/weston-test-client-helper.c
tests/weston-test-client-helper.h
wcap/wcap-decode.h
xwayland/dnd.c
xwayland/hash.h
xwayland/launcher.c
xwayland/selection.c
xwayland/window-manager.c

I expect most of them build fine because somewhere in the include chain,
some other header includes it for them, but IMHO the #include line
should be explicitly there anyway.
Would you like to send a patch for those as well?
___
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

[Eric Engestrom]

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>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] xwayland: Grammar fixes

[Eric Engestrom]

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?

Either way, your changes are good:
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

Re: [PATCH weston] dmabuf: Fix grammar in a comment

[Eric Engestrom]

On Wed, Jul 06, 2016 at 03:30:54PM -0700, Bryce Harrington wrote:
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Eric Engestrom]

On Wed, Jul 06, 2016 at 03:18:46PM -0700, Bryce Harrington wrote:
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.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

Re: [PATCH weston] xwayland: Include missing config.h

[Eric Engestrom]

On Wed, Jul 06, 2016 at 03:14:20PM -0700, Bryce Harrington wrote:
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

Indeed, this is needed for XSERVER_PATH
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.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 libinput] gestures: make the gesture movement threshold depending on finger count

[Eric Engestrom]

On Thu, Jun 30, 2016 at 02:10:24PM +0100, Eric Engestrom wrote:
> The threshold now becomes 0 for 1 finger, which I assume was not the

Sorry for the noise, I just realized this code only applies to >2
fingers, so you can just ignore my previous message :)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] gestures: make the gesture movement threshold depending on finger count

[Eric Engestrom]

On Thu, Jun 30, 2016 at 12:22:10PM +1000, Peter Hutterer wrote:
> Increase the mm move threshold for 3 and 4 finger gestures to 2 and 3 mm,
> respectively. In multi-finger gestures it's common to have minor movement
> while all fingers are being put down or before the conscious movement starts.
> This can trigger invalid gesture detection (e.g. a pinch instead of a swipe).
> Increase the movement threshold to make sure we have sufficient input data.
> 
> No changes to 2-finger movements.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=96687
> 
> Signed-off-by: Peter Hutterer 
> ---
>  src/evdev-mt-touchpad-gestures.c |  9 ++---
>  test/gestures.c  | 10 +-
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/src/evdev-mt-touchpad-gestures.c 
> b/src/evdev-mt-touchpad-gestures.c
> index e4e465a..a910bec 100644
> --- a/src/evdev-mt-touchpad-gestures.c
> +++ b/src/evdev-mt-touchpad-gestures.c
> @@ -182,12 +182,15 @@ tp_gesture_get_active_touches(const struct tp_dispatch 
> *tp,
>  }
>  
>  static int
> -tp_gesture_get_direction(struct tp_dispatch *tp, struct tp_touch *touch)
> +tp_gesture_get_direction(struct tp_dispatch *tp, struct tp_touch *touch,
> +  unsigned int nfingers)
>  {
>   struct normalized_coords normalized;
>   struct device_float_coords delta;
>   double move_threshold = TP_MM_TO_DPI_NORMALIZED(1);
>  
> + move_threshold *= (nfingers - 1);

The threshold now becomes 0 for 1 finger, which I assume was not the
intended behaviour. Should `-1` be removed (and the commit message adapted)?

Alternatively, something like this would give the result from the commit
message:

move_threshold *= min(1, nfingers - 1);

> +
>   delta = device_delta(touch->point, touch->gesture.initial);
>  
>   normalized = tp_normalize_delta(tp, delta);
> @@ -347,8 +350,8 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, 
> uint64_t time)
>   }
>  
>   /* Else wait for both fingers to have moved */
> - dir1 = tp_gesture_get_direction(tp, first);
> - dir2 = tp_gesture_get_direction(tp, second);
> + dir1 = tp_gesture_get_direction(tp, first, tp->gesture.finger_count);
> + dir2 = tp_gesture_get_direction(tp, second, tp->gesture.finger_count);
>   if (dir1 == UNDEFINED_DIRECTION || dir2 == UNDEFINED_DIRECTION)
>   return GESTURE_STATE_UNKNOWN;
>  

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

Re: [PATCH weston] clients/weston-info: print unknown formats better

[Eric Engestrom]

On Monday, 2017-02-20 15:47:57 +0200, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> 
> Don't just dump the raw 32-bit values, try to interpret it as a DRM
> fourcc too.
> 
> This prints properly the formats YUYV, NV12 and YU12 supported by
> Weston.
> 
> Signed-off-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> ---
>  clients/weston-info.c | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/clients/weston-info.c b/clients/weston-info.c
> index 712346a..ab12947 100644
> --- a/clients/weston-info.c
> +++ b/clients/weston-info.c
> @@ -30,6 +30,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #include 
>  
> @@ -240,9 +242,33 @@ print_output_info(void *data)
>   }
>  }
>  
> +static char
> +bits2graph(uint32_t value, unsigned bitoffset)
> +{
> + int c = (value >> bitoffset) & 0xff;
> +
> + if (isgraph(c) || isspace(c))
> + return c;
> +
> + return '?';
> +}
> +
> +static void
> +fmt2str(uint32_t format, char *str, int len)

`fourcc2str()` ?
This function is specific to 4 char codes, I think its name should
reflect this.

> +{
> + int i;
> +
> + assert(len >= 5);
> +
> + for (i = 0; i < 4; i++)
> + str[i] = bits2graph(format, i * 8);
> + str[i] = '\0';
> +}
> +
>  static void
>  print_shm_info(void *data)
>  {
> + char str[6];

6? not 5?

These are nit-picks, this patch already does what it says on the tin:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

>   struct shm_info *shm = data;
>   struct shm_format *format;
>  
> @@ -262,7 +288,8 @@ print_shm_info(void *data)
>   printf(" RGB565");
>   break;
>   default:
> - printf(" unknown(%08x)", format->format);
> + fmt2str(format->format, str, sizeof(str));
> + printf(" '%s'(0x%08x)", str, format->format);
>   break;
>   }
>  
> -- 
> 2.10.2
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] clients/simple-egl: add -d option

[Eric Engestrom]

> Subject: [PATCH weston] clients/simple-egl: add -d option

s/-d/delay/ in the commit title?

On Monday, 2017-02-20 16:13:48 +0200, Pekka Paalanen wrote:
> From: Eero Tamminen <eero.t.tammi...@intel.com>
> 
> This emulates extra drawing work by usleep().
> 
> This is an enhancement to reproduce the problem in the bug report.
> 
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=98833
> Signed-off-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> ---
>  clients/simple-egl.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/clients/simple-egl.c b/clients/simple-egl.c
> index 9b6fa1f..c5ee05d 100644
> --- a/clients/simple-egl.c
> +++ b/clients/simple-egl.c
> @@ -100,7 +100,7 @@ struct window {
>   struct ivi_surface *ivi_surface;
>   EGLSurface egl_surface;
>   struct wl_callback *callback;
> - int fullscreen, opaque, buffer_size, frame_sync;
> + int fullscreen, opaque, buffer_size, frame_sync, delay;
>   bool wait_for_configure;
>  };
>  
> @@ -548,6 +548,8 @@ redraw(void *data, struct wl_callback *callback, uint32_t 
> time)
>   glDisableVertexAttribArray(window->gl.pos);
>   glDisableVertexAttribArray(window->gl.col);
>  
> + usleep(window->delay);
> +
>   if (window->opaque || window->fullscreen) {
>   region = 
> wl_compositor_create_region(window->display->compositor);
>   wl_region_add(region, 0, 0,
> @@ -850,6 +852,7 @@ usage(int error_code)
>   "  -o\tCreate an opaque surface\n"
>   "  -s\tUse a 16 bpp EGL config\n"
>   "  -b\tDon't sync to compositor redraw (eglSwapInterval 0)\n"
> + "  -d \tBuffer swap delay in microseconds\n"
>   "  -h\tThis help text\n\n");
>  
>   exit(error_code);
> @@ -870,9 +873,12 @@ main(int argc, char **argv)
>   window.window_size = window.geometry;
>   window.buffer_size = 32;
>   window.frame_sync = 1;
> + window.delay = 0;
>  
>   for (i = 1; i < argc; i++) {
> - if (strcmp("-f", argv[i]) == 0)
> + if (strcmp("-d", argv[i]) == 0 && i+1 < argc)
> + window.delay = atoi(argv[++i]);

Options are displayed (help) in one order and parsed in another...
Kind of a nit-pick, but it looks weird to me.

With the more explicit commit title, patch is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> + else if (strcmp("-f", argv[i]) == 0)
>   window.fullscreen = 1;
>   else if (strcmp("-o", argv[i]) == 0)
>   window.opaque = 1;
> -- 
> 2.10.2
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland v2 2/4] wayland-util: split out private functionality to separate file

[Eric Engestrom]

On Tuesday, 2017-02-21 16:14:27 +, Emil Velikov wrote:
> From: Emil Velikov 
> 
> With next commit we'll make wayland-util a shared library (for reasons
> mentioned in the commit). As such we need to make sure that the private
> symbols are somewhere that they can be used internally. Otherwise we'll
> end up with link errors.
> 
> v2: Rebase.
> 
> Signed-off-by: Emil Velikov 
> ---
>  Makefile.am|   1 +
>  src/wayland-util-private.c | 303 
> +
>  src/wayland-util.c | 272 +---
>  3 files changed, 305 insertions(+), 271 deletions(-)
>  create mode 100644 src/wayland-util-private.c
> 

[snip]

> diff --git a/src/wayland-util.c b/src/wayland-util.c
> index cab7fc5..bb91aa7 100644
> --- a/src/wayland-util.c
> +++ b/src/wayland-util.c
> @@ -30,8 +30,8 @@
>  #include 
>  #include 
>  
> -#include "wayland-util.h"
>  #include "wayland-private.h"
> +#include "wayland-util.h"
>  
>  WL_EXPORT void
>  wl_list_init(struct wl_list *list)

Seeing this raises a red flag in my mind: why is this needed?
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH xserver] xwayland: add envvar XWAYLAND_NO_GLAMOR

[Eric Engestrom]

On Wednesday, 2017-03-01 17:45:12 +0100, Olivier Fourdan wrote:
> Not all compositors allow for customizing the Xwayland command line,
> gnome-shell/mutter for example have the command line and path to
> Xwayland binary hardcoded, which makes it harder for users to disable
> glamor acceleration in Xwayland (glamor being used by default).
> 
> Add an environment variable XWAYLAND_NO_GLAMOR t odiable glamor support

"to disable"

The change itself looks good to me.
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

As to whether it's a good idea to allow this, I'd say it is, but you
might want the opinion of someone who's more involved in this project :)

Cheers,
  Eric

> in Xwayland.
> 
> Signed-off-by: Olivier Fourdan <ofour...@redhat.com>
> ---
>  hw/xwayland/xwayland-glamor.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
> index b3d0aab..45de54f 100644
> --- a/hw/xwayland/xwayland-glamor.c
> +++ b/hw/xwayland/xwayland-glamor.c
> @@ -549,6 +549,13 @@ Bool
>  xwl_glamor_init(struct xwl_screen *xwl_screen)
>  {
>  ScreenPtr screen = xwl_screen->screen;
> +const char *no_glamor_env;
> +
> +no_glamor_env = getenv("XWAYLAND_NO_GLAMOR");
> +if (no_glamor_env && *no_glamor_env != '0') {

Nit: `XWAYLAND_NO_GLAMOR=` evaluates to true here, disabling glamor.
Not sure how much we care about this case though, or if we do want this
behaviour, but this would fix it:

if (no_glamor_env && *no_glamor_env && *no_glamor_env != '0') {

> +ErrorF("Disabling glamor and dri3 support, XWAYLAND_NO_GLAMOR is 
> set\n");
> +return FALSE;
> +}
>  
>  if (xwl_screen->egl_context == EGL_NO_CONTEXT) {
>  ErrorF("Disabling glamor and dri3, EGL setup failed\n");
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] doc: extend build instructions for dependencies

[Eric Engestrom]

On Thu, Sep 01, 2016 at 09:53:36AM +1000, Peter Hutterer wrote:
> Signed-off-by: Peter Hutterer 
> ---
> Mostly sending this out for any hints on how to deal with this in Arch or
> other distributions not listed here. Let me know and I'll add it to the
> list.

The Arch way would be to install the deps from the PKGBUILD (using the ABS [1]):
# abs
$ cd $(mktemp -d)
$ cp /var/abs/extra/libinput/PKGBUILD .
$ makepkg --syncdeps --nobuild
but these aren't the simplest instructions...

Cheers,
  Eric

[1] https://wiki.archlinux.org/index.php/Arch_Build_System#Build_package

> 
> Cheers,
>Peter
> 
>  doc/building.dox | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/building.dox b/doc/building.dox
> index 5b4ca52..a14eba7 100644
> --- a/doc/building.dox
> +++ b/doc/building.dox
> @@ -19,10 +19,27 @@ $> sudo make install
>  @note On Debian-based distributions including Ubuntu and its derivatives 
> skip the
>  ```--libdir=/usr/lib64``` argument.
>  
> -A description on how to fix "No package 'foo' found" errors during the
> -configure stage is available
> - href="https://who-t.blogspot.com.au/2014/05/configure-fails-with-no-package-foo.html;>in
> -this blog post here.
> +A successful build requires the @ref building_dependencies to be installed
> +at configure time.
> +
> +@subsection building_dependencies Build dependencies
> +
> +libinput has a few build-time dependencies that must be installed prior to
> +running configure. In most cases, it is sufficient to install the
> +dependencies that your distribution uses to build the libinput package.
> +These can be installed with one of the following commands:
> +
> +
> +Debian/Ubuntu based distributions: ```sudo apt-get build-dep
> +libinput```
> +Fedora 22 and later: ```sudo dnf builddep libinput```
> +SuSE/RHEL/CentOS/Fedora 21 and earlier: ```sudo yum-builddep 
> libinput```
> +
> +
> +If dependencies are missing, a message ```No package 'foo' found``` will be
> +shown during the configure stage. See
> + href="https://who-t.blogspot.com.au/2014/05/configure-fails-with-no-package-foo.html;>this
>  blog post here.
> +for instructions on how to fix it.
>  
>  @subsection building_libwacom Building without libwacom
>  
> -- 
> 2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] doc: extend build instructions for dependencies

[Eric Engestrom]

On Thu, Sep 01, 2016 at 11:18:12AM +0100, Eric Engestrom wrote:
> On Thu, Sep 01, 2016 at 09:53:36AM +1000, Peter Hutterer wrote:
> > Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> > ---
> > Mostly sending this out for any hints on how to deal with this in Arch or
> > other distributions not listed here. Let me know and I'll add it to the
> > list.
> 
> The Arch way would be to install the deps from the PKGBUILD (using the ABS 
> [1]):
>   # abs

Sorry, that should be:
# abs extra/libinput
(no need to sync everything if we only care about libinput)

>   $ cd $(mktemp -d)
>   $ cp /var/abs/extra/libinput/PKGBUILD .
>   $ makepkg --syncdeps --nobuild
> but these aren't the simplest instructions...
> 
> Cheers,
>   Eric
> 
> [1] https://wiki.archlinux.org/index.php/Arch_Build_System#Build_package
> 
> > 
> > Cheers,
> >Peter
> > 
> >  doc/building.dox | 25 +
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/doc/building.dox b/doc/building.dox
> > index 5b4ca52..a14eba7 100644
> > --- a/doc/building.dox
> > +++ b/doc/building.dox
> > @@ -19,10 +19,27 @@ $> sudo make install
> >  @note On Debian-based distributions including Ubuntu and its derivatives 
> > skip the
> >  ```--libdir=/usr/lib64``` argument.
> >  
> > -A description on how to fix "No package 'foo' found" errors during the
> > -configure stage is available
> > - > href="https://who-t.blogspot.com.au/2014/05/configure-fails-with-no-package-foo.html;>in
> > -this blog post here.
> > +A successful build requires the @ref building_dependencies to be installed
> > +at configure time.
> > +
> > +@subsection building_dependencies Build dependencies
> > +
> > +libinput has a few build-time dependencies that must be installed prior to
> > +running configure. In most cases, it is sufficient to install the
> > +dependencies that your distribution uses to build the libinput package.
> > +These can be installed with one of the following commands:
> > +
> > +
> > +Debian/Ubuntu based distributions: ```sudo apt-get build-dep
> > +libinput```
> > +Fedora 22 and later: ```sudo dnf builddep libinput```
> > +SuSE/RHEL/CentOS/Fedora 21 and earlier: ```sudo yum-builddep 
> > libinput```
> > +
> > +
> > +If dependencies are missing, a message ```No package 'foo' found``` will be
> > +shown during the configure stage. See
> > + > href="https://who-t.blogspot.com.au/2014/05/configure-fails-with-no-package-foo.html;>this
> >  blog post here.
> > +for instructions on how to fix it.
> >  
> >  @subsection building_libwacom Building without libwacom
> >  
> > -- 
> > 2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] tools: prefix debug output with '.' on device changes

[Eric Engestrom]

On Thu, Sep 08, 2016 at 03:55:47PM +1000, Peter Hutterer wrote:
> We print the sysname, but it's not always obvious when there's an event from
> another device within the stream from another device. Prefix it so it's easier
> to spot and search for.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1364850#c3 for an example of
> how such an event can hide.
> 
> We only use last_device for comparing pointer values so we don't need a
> reference to the device, it doesn't matter if the device itself goes out of
> scope.
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> ---
>  tools/event-debug.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/event-debug.c b/tools/event-debug.c
> index 67432b7..0e95611 100644
> --- a/tools/event-debug.c
> +++ b/tools/event-debug.c
> @@ -49,8 +49,11 @@ static unsigned int stop = 0;
>  static void
>  print_event_header(struct libinput_event *ev)
>  {
> + /* use for pointer value only, do not dereference */
> + static struct libinput_device *last_device = NULL;

I would make that a void* to make it clear that it's not meant to be
dereferenced.

>   struct libinput_device *dev = libinput_event_get_device(ev);
>   const char *type = NULL;
> + char prefix;
>  
>   switch(libinput_event_get_type(ev)) {
>   case LIBINPUT_EVENT_NONE:
> @@ -132,7 +135,14 @@ print_event_header(struct libinput_event *ev)
>   break;
>   }
>  
> - printf("%-7s%-16s ", libinput_device_get_sysname(dev), type);
> + prefix = (last_device != dev) ? '-' : ' ';

Nit: that line would be even shorter within the printf than here on
its own :)

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> +
> + printf("%c%-7s  %-16s ",
> +prefix,
> +libinput_device_get_sysname(dev),
> +type);
> +
> + last_device = dev;
>  }
>  
>  static void
> -- 
> 2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] tools: prefix debug output with '.' on device changes

[Eric Engestrom]

On Thu, Sep 15, 2016 at 07:55:54AM +1000, Peter Hutterer wrote:
> On Wed, Sep 14, 2016 at 05:48:26PM +0100, Eric Engestrom wrote:
> > On Thu, Sep 08, 2016 at 03:55:47PM +1000, Peter Hutterer wrote:
> > > diff --git a/tools/event-debug.c b/tools/event-debug.c
> > > index 67432b7..0e95611 100644
> > > --- a/tools/event-debug.c
> > > +++ b/tools/event-debug.c
> > > @@ -49,8 +49,11 @@ static unsigned int stop = 0;
> > >  static void
> > >  print_event_header(struct libinput_event *ev)
> > >  {
> > > + /* use for pointer value only, do not dereference */
> > > + static struct libinput_device *last_device = NULL;
> > 
> > I would make that a void* to make it clear that it's not meant to be
> > dereferenced.
> 
> I've already pushed this patch for 1.5 but I've changed this in a follow-up
> patch now. Thanks.

$DAYJOB's email infrastructure could be better... and because of that
I only read your reply just now... I guess you can discard my patch :)

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

[PATCH libinput] tools: change pointer to void

[Eric Engestrom]

This makes it clear that it's not meant to be dereferenced.

CC: Peter Hutterer <peter.hutte...@who-t.net>
Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
---
 tools/event-debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/event-debug.c b/tools/event-debug.c
index 0e95611..5cd5915 100644
--- a/tools/event-debug.c
+++ b/tools/event-debug.c
@@ -50,7 +50,7 @@ static void
 print_event_header(struct libinput_event *ev)
 {
/* use for pointer value only, do not dereference */
-   static struct libinput_device *last_device = NULL;
+   static void *last_device = NULL;
struct libinput_device *dev = libinput_event_get_device(ev);
const char *type = NULL;
char prefix;
-- 
Cheers,
  Eric

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

Re: [PATCH wayland v2 2/4] wl_array: Set data to invalid address after free

[Eric Engestrom]

On Thu, Sep 29, 2016 at 07:17:37AM -0700, Yong Bakos wrote:
> Thanks Eric. Would you mind cc'ing the wayland-devel list, so that Patchwork 
> catches your RB?

Sorry, on MLs I usually just hit reply-all without looking at the list
of recipients.

So, for patchwork:

The series is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

(BTW patchwork doesn't understand "the series is", does it? Or does it
have to be a specific keyword?)

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

Re: [PATCH weston] weston-launch: use custom error function

[Eric Engestrom]

On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> error.h is a gnu extension and not available in other
> popular libcs like musl. This patch provides a replacement.
> 
> Signed-off-by: Murray Calavera <murray.calav...@gmail.com>

How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
  [...]
  checking for library containing pam_open_session... no
  configure: error: weston-launch requires pam

The code looks good though (with one nit-pick), so even if I couldn't
test it, it is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  libweston/weston-launch.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
> index 140fde1..84f7d60 100644
> --- a/libweston/weston-launch.c
> +++ b/libweston/weston-launch.c
> @@ -33,7 +33,6 @@
>  #include 
>  #include 
>  
> -#include 
>  #include 
>  
>  #include 
> @@ -112,6 +111,25 @@ struct weston_launch {
>  
>  union cmsg_data { unsigned char b[4]; int fd; };
>  
> +static void
> +error(int status, int errnum, const char *msg, ...)
> +{
> + va_list args;
> +
> + fputs("weston-launch: ", stderr);
> + va_start(args, msg);
> + vfprintf(stderr, msg, args);
> + va_end(args);
> +
> + if (errnum)
> + fprintf(stderr, ": %s\n", strerror(errnum));
> + else
> + fputc('\n', stderr);

Why not `fprintf(stderr, "\n");`?
While fputc() is enough since this is a single char, the use of
a different function here looks... odd.

> +
> + if (status)
> + exit(status);
> +}
> +
>  static gid_t *
>  read_groups(void)
>  {
> -- 
> 2.10.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Compiling Weston with musl-libc (WAS: [PATCH weston] weston-launch: use custom error function)

[Eric Engestrom]

On Fri, Sep 30, 2016 at 02:07:38PM +0200, Armin Krezović wrote:
> On 30.09.2016 13:59, Eric Engestrom wrote:
> > On Fri, Sep 30, 2016 at 11:36:24AM +0100, Murray Calavera wrote:
> >> On 30 September 2016 at 11:10, Eric Engestrom <eric.engest...@imgtec.com>
> >> wrote:
> >>
> >>> On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> >>>> error.h is a gnu extension and not available in other
> >>>> popular libcs like musl. This patch provides a replacement.
> >>>>
> >>>> Signed-off-by: Murray Calavera <murray.calav...@gmail.com>
> >>>
> >>> How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
> >>>   [...]
> >>>   checking for library containing pam_open_session... no
> >>>   configure: error: weston-launch requires pam
> >>>
> >>
> >> Have you got libpam installed?
> >> I don't see how this patch could have affected the configure,
> >> does it configure without this patch?
> > 
> > I do have it, and it works fine with both `CC=gcc` and `CC=clang`, but
> > `CC=musl-gcc` and `CC=musl-clang` both fail with that error.
> > 
> > Is this also how you use musl?  If it is, then the issue is probably on
> > my side, I won't take anymore of your time with this :)
> > 
> 
> Did you apply the patch that was sent by him before this one? IIRC,
> we narrowed that issue yesterday on IRC.

I tried both with and without that patch, and it made no difference.

I think I'll just give up. Even though it would have been nice to test
with musl, I don't need it, so I'll leave it to those who do :P

Thanks for trying, guys :)

/me runs away and forgets all about musl
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Compiling Weston with musl-libc (WAS: [PATCH weston] weston-launch: use custom error function)

[Eric Engestrom]

On Fri, Sep 30, 2016 at 11:36:24AM +0100, Murray Calavera wrote:
> On 30 September 2016 at 11:10, Eric Engestrom <eric.engest...@imgtec.com>
> wrote:
> 
> > On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> > > error.h is a gnu extension and not available in other
> > > popular libcs like musl. This patch provides a replacement.
> > >
> > > Signed-off-by: Murray Calavera <murray.calav...@gmail.com>
> >
> > How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
> >   [...]
> >   checking for library containing pam_open_session... no
> >   configure: error: weston-launch requires pam
> >
> 
> Have you got libpam installed?
> I don't see how this patch could have affected the configure,
> does it configure without this patch?

I do have it, and it works fine with both `CC=gcc` and `CC=clang`, but
`CC=musl-gcc` and `CC=musl-clang` both fail with that error.

Is this also how you use musl?  If it is, then the issue is probably on
my side, I won't take anymore of your time with this :)

> 
> >
> > The code looks good though (with one nit-pick), so even if I couldn't
> > test it, it is:
> > Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
> >
> > > ---
> > >  libweston/weston-launch.c | 20 +++-
> > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
> > > index 140fde1..84f7d60 100644
> > > --- a/libweston/weston-launch.c
> > > +++ b/libweston/weston-launch.c
> > > @@ -33,7 +33,6 @@
> > >  #include 
> > >  #include 
> > >
> > > -#include 
> > >  #include 
> > >
> > >  #include 
> > > @@ -112,6 +111,25 @@ struct weston_launch {
> > >
> > >  union cmsg_data { unsigned char b[4]; int fd; };
> > >
> > > +static void
> > > +error(int status, int errnum, const char *msg, ...)
> > > +{
> > > + va_list args;
> > > +
> > > + fputs("weston-launch: ", stderr);
> > > + va_start(args, msg);
> > > + vfprintf(stderr, msg, args);
> > > + va_end(args);
> > > +
> > > + if (errnum)
> > > + fprintf(stderr, ": %s\n", strerror(errnum));
> > > + else
> > > + fputc('\n', stderr);
> >
> > Why not `fprintf(stderr, "\n");`?
> > While fputc() is enough since this is a single char, the use of
> > a different function here looks... odd.
> >
> 
> As you said, because I'm not printing formatted data
> there is no need to use printf.
> However if the consensus here is to use printf even
> when not needed, I can change that.

I just thought it looked odd, but I have no real argument one way or the
other, so feel free to leave it as is :)

Cheers,
  Eric

> 
> >
> > > +
> > > + if (status)
> > > + exit(status);
> > > +}
> > > +
> > >  static gid_t *
> > >  read_groups(void)
> > >  {
> > > --
> > > 2.10.0
> >
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/4] wl_array: Set data to null after free

[Eric Engestrom]

On Mon, Sep 19, 2016 at 11:59:03AM +0100, Eric Engestrom wrote:
> On Fri, Sep 16, 2016 at 03:37:37PM -0700, Yong Bakos wrote:
> > From: Yong Bakos <yba...@humanoriented.com>
> > 
> > Explicitly set the data member to NULL during wl_array_release, preventing 
> > the
> > dangling pointer but, more importantly, making it testable.
> > 
> > Signed-off-by: Yong Bakos <yba...@humanoriented.com>
> > ---
> >  src/wayland-util.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/wayland-util.c b/src/wayland-util.c
> > index 639ccf8..2efb133 100644
> > --- a/src/wayland-util.c
> > +++ b/src/wayland-util.c
> > @@ -102,6 +102,7 @@ WL_EXPORT void
> >  wl_array_release(struct wl_array *array)
> >  {
> > free(array->data);
> > +   array->data = NULL;
> 
> If we add
>   array->size = 0;
>   array->alloc = 0;
> 
> we can then remove this comment from patch #1, right?
>   \note Leaves the array in an invalid state.

I somehow missed your cover-letter, but at least we agree :P
I guess you'll send that as an independent patch later on?

Cheers,
  Eric

> 
> The series is good anyway, so it is:
> 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

Re: [PATCH] clients/stacking: Silence a compiler warning

[Eric Engestrom]

On Sat, Sep 10, 2016 at 10:55:21PM +0200, Armin Krezović wrote:
> clang doesn't support gnu_print attribute, so just
> leave it out when clang is used.
> 
> Signed-off-by: Armin Krezović 
> ---
>  clients/stacking.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/clients/stacking.c b/clients/stacking.c
> index 4285a17..dd3d338 100644
> --- a/clients/stacking.c
> +++ b/clients/stacking.c
> @@ -184,7 +184,11 @@ fullscreen_handler(struct window *window, void *data)
>  
>  static void
>  draw_string(cairo_t *cr,
> +#ifndef __clang__
>  const char *fmt, ...) __attribute__((format (gnu_printf, 2, 3)));
> +#else
> +const char *fmt, ...);
> +#endif

I would recommend avoiding structure elements (like `)` and `;`) in
#ifdef blocks, as some editors get confused by this.

I would also recommend avoiding code duplication, by moving the
__attribute__ to its own line and only #ifdef'ing that line.
(As a reminder, function attributes can also be added before the
function or between the return type and the function name, avoiding the
need for `;` to also be on its own line after the #ifdef).

But in this case, I think replacing `gnu_printf` with `printf` is
probably the best solution.
I was unable to find the difference between the two, though. Does anyone
know why one would want to use the gnu-prefixed version?

I also don't know which project this patch is for (you might want to run
`git config format.subjectprefix "PATCH $(basename "$PWD")"` in the root
of each of your local clones), but it might already have a __printf()-style
macro [1][2][3][4] to avoid using #ifdef everywhere.

Cheers,
  Eric

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h#n119
[2] https://cgit.freedesktop.org/mesa/mesa/tree/src/util/macros.h#n128
[3] https://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-util.h#n59
[4] https://cgit.freedesktop.org/wayland/libinput/tree/src/libinput.h#n36

>  
>  static void
>  draw_string(cairo_t *cr,
> -- 
> 2.10.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/4] wl_array: Set data to null after free

[Eric Engestrom]

On Fri, Sep 16, 2016 at 03:37:37PM -0700, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> Explicitly set the data member to NULL during wl_array_release, preventing the
> dangling pointer but, more importantly, making it testable.
> 
> Signed-off-by: Yong Bakos <yba...@humanoriented.com>
> ---
>  src/wayland-util.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/wayland-util.c b/src/wayland-util.c
> index 639ccf8..2efb133 100644
> --- a/src/wayland-util.c
> +++ b/src/wayland-util.c
> @@ -102,6 +102,7 @@ WL_EXPORT void
>  wl_array_release(struct wl_array *array)
>  {
>   free(array->data);
> + array->data = NULL;

If we add
array->size = 0;
array->alloc = 0;

we can then remove this comment from patch #1, right?
\note Leaves the array in an invalid state.

The series is good anyway, so it is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

>  }
>  
>  WL_EXPORT void *
> -- 
> 2.7.2
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/2] tests: Add test for wl_list_length

[Eric Engestrom]

On Thu, Aug 25, 2016 at 04:12:34PM -0700, Yong Bakos wrote:
> From: Yong Bakos <yba...@humanoriented.com>
> 
> list-test.c did not cover wl_list_length, so add one test that specifically
> tests this method.
> 
> Signed-off-by: Yong Bakos <yba...@humanoriented.com>
> ---
>  tests/list-test.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tests/list-test.c b/tests/list-test.c
> index 21ca4ec..0752618 100644
> --- a/tests/list-test.c
> +++ b/tests/list-test.c
> @@ -57,6 +57,19 @@ TEST(list_insert)
>   assert(e.link.prev == );
>  }
>  
> +TEST(list_length)
> +{
> + struct wl_list list;
> + struct element e;
> +
> + wl_list_init();
> + assert(wl_list_length() == 0);
> + wl_list_insert(, );
> + assert(wl_list_length() == 1);

What about a second insert(e) to make sure duplicates are handled
correctly?

With or without this, the series is
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> + wl_list_remove();
> + assert(wl_list_length() == 0);
> +}
> +
>  TEST(list_iterator)
>  {
>   struct wl_list list;
> -- 
> 2.7.2
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2] clients/stacking: Silence a compiler warning

[Eric Engestrom]

On Tue, Sep 27, 2016 at 12:35:55PM +0200, Armin Krezović wrote:
> This patch fixes a compiler warning when building with
> clang, since it doesn't support gnu_printf attribute.
> 
> v2:
> 
>  - Switch to WL_PRINTF per suggestion from Eric Engestrom.
> 
> Signed-off-by: Armin Krezović <krezovic.ar...@gmail.com>
> ---

Right now you get the WL_PRINTF macro because clients/stacking.c
includes "window.h", which includes "shared/platform.h", which includes
, which internally ends up including "wayland-util.h".

It would be better to directly include wayland-util.h in this file,
making the fragile include-chain irrelevant and making it explicit that
you use something in this file.

With that #include added, this is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Cheers!

>  clients/stacking.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/clients/stacking.c b/clients/stacking.c
> index 4285a17..7c711e4 100644
> --- a/clients/stacking.c
> +++ b/clients/stacking.c
> @@ -184,7 +184,7 @@ fullscreen_handler(struct window *window, void *data)
>  
>  static void
>  draw_string(cairo_t *cr,
> -const char *fmt, ...) __attribute__((format (gnu_printf, 2, 3)));
> +const char *fmt, ...) WL_PRINTF(2,3);
>  
>  static void
>  draw_string(cairo_t *cr,
> -- 
> 2.10.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland v2 2/4] wl_array: Set data to invalid address after free

[Eric Engestrom]

On Tue, Sep 27, 2016 at 01:03:48PM -0500, Yong Bakos wrote:
> From: Yong Bakos 
> 
> Explicitly set the data member to an invalid memory address during
> wl_array_release, such that re-using a freed wl_array without re-initializing
> causes a crash. In addition, this pointer assignment makes wl_array_release
> testable.
> 
> Define a constant for the invalid memory address, and add documentation about
> this behavior, starting at libwayland version 1.13.

I actually did a similar thing in our internal codebase recently
(although my focus was catching double-free).
I used a small stack var as a sentinel, and set freed vars to its
address with an assert first to make sure it wasn't already that.

My implementation translated here would be roughly:

in src/wayland-private.h:
#ifndef NDEBUG
  extern char wl_array_sentinel;
# define WL_ARRAY_POISON_PTR ((void*) _array_sentinel)
#else
# define WL_ARRAY_POISON_PTR NULL
#endif

in src/wayland-util.c:
#ifndef NDEBUG
char wl_array_sentinel;
#endif

in wl_array_release(), before `free(array->data)`:
assert(array->data != WL_ARRAY_POISON_PTR);
(same could be added in `wl_array_{add,copy}()`)

The benefit of this is that you know the address isn't used by something
else, and a char should be cheap enough to not have any impact :)

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

Re: [PATCH weston v2 1/3] compositor-drm: refactor destroy drm_fb function

[Eric Engestrom]

On Fri, Sep 30, 2016 at 06:28:51PM +0900, Tomohito Esaki wrote:
> The drm_fb destroy callback to mostly the same thing regardless of
> whether the buffer is a dumb buffer or gbm buffer. This patch refactors
> the common parts into a new function that can be called for both cases.
> 
> Signed-off-by: Tomohito Esaki <e...@igel.co.jp>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  libweston/compositor-drm.c | 31 ---
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 8319d7c..a707fc4 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -261,17 +261,23 @@ drm_sprite_crtc_supported(struct drm_output *output, 
> uint32_t supported)
>  }
>  
>  static void
> -drm_fb_destroy_callback(struct gbm_bo *bo, void *data)
> +drm_fb_destroy(struct drm_fb *fb, int fd)
>  {
> - struct drm_fb *fb = data;
> - struct gbm_device *gbm = gbm_bo_get_device(bo);
> -
>   if (fb->fb_id)
> - drmModeRmFB(gbm_device_get_fd(gbm), fb->fb_id);
> + drmModeRmFB(fd, fb->fb_id);
>  
>   weston_buffer_reference(>buffer_ref, NULL);
>  
> - free(data);
> + free(fb);
> +}
> +
> +static void
> +drm_fb_destroy_gbm_fb(struct gbm_bo *bo, void *data)
> +{
> + struct drm_fb *fb = data;
> + struct gbm_device *gbm = gbm_bo_get_device(bo);
> +
> + drm_fb_destroy(fb, gbm_device_get_fd(gbm));
>  }
>  
>  static struct drm_fb *
> @@ -370,22 +376,17 @@ static void
>  drm_fb_destroy_dumb(struct drm_fb *fb)
>  {
>   struct drm_mode_destroy_dumb destroy_arg;
> + int fd = fb->fd;
>  
>   if (!fb->map)
>   return;
>  
> - if (fb->fb_id)
> - drmModeRmFB(fb->fd, fb->fb_id);
> -
> - weston_buffer_reference(>buffer_ref, NULL);
> -
>   munmap(fb->map, fb->size);
> -
>   memset(_arg, 0, sizeof(destroy_arg));
>   destroy_arg.handle = fb->handle;
> - drmIoctl(fb->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _arg);
>  
> - free(fb);
> + drm_fb_destroy(fb, fd);
> + drmIoctl(fd, DRM_IOCTL_MODE_DESTROY_DUMB, _arg);
>  }
>  
>  static struct drm_fb *
> @@ -446,7 +447,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo,
>   goto err_free;
>   }
>  
> - gbm_bo_set_user_data(bo, fb, drm_fb_destroy_callback);
> + gbm_bo_set_user_data(bo, fb, drm_fb_destroy_gbm_fb);
>  
>   return fb;
>  
> -- 
> 2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

[Eric Engestrom]

On Fri, Sep 30, 2016 at 06:28:52PM +0900, Tomohito Esaki wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.
> 
> Signed-off-by: Tomohito Esaki 
> ---
>  libweston/compositor-drm.c | 125 
> ++---
>  1 file changed, 107 insertions(+), 18 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index a707fc4..b15fa01 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -151,6 +151,9 @@ struct drm_fb {
>  
>   /* Used by dumb fbs */
>   void *map;
> +
> + /* Used by dmabuf */
> + bool is_dmabuf;
>  };
>  
>  struct drm_edid {
> @@ -389,6 +392,76 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>   drmIoctl(fd, DRM_IOCTL_MODE_DESTROY_DUMB, _arg);
>  }
>  
> +static inline void
> +close_drm_handle(int fd, uint32_t handle)
> +{
> + struct drm_gem_close gem_close = { .handle = handle };
> + int ret;
> +
> + ret = drmIoctl(fd, DRM_IOCTL_GEM_CLOSE, _close);
> + if (ret)
> + weston_log("DRM_IOCTL_GEM_CLOSE failed.(%s)\n",
> +strerror(errno));
> +}
> +
> +static struct drm_fb *
> +drm_fb_create_dmabuf(struct linux_dmabuf_buffer *dmabuf,
> +  struct drm_backend *backend, uint32_t format)
> +{
> + struct drm_fb *fb = NULL;
> + uint32_t width, height, fb_id, handles[4] = {0};
> + int i, ret;
> +
> + if (!format)
> + return NULL;
> +
> + width = dmabuf->attributes.width;
> + height = dmabuf->attributes.height;
> + if (backend->min_width > width ||
> + width > backend->max_width ||
> + backend->min_height > height ||
> + height > backend->max_height)
> + return NULL;
> +
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + ret = drmPrimeFDToHandle(backend->drm.fd,
> +  dmabuf->attributes.fd[i],
> +  [i]);
> + if (ret)
> + goto done;
> + }
> +
> + ret = drmModeAddFB2(backend->drm.fd, width, height,
> + format, handles, dmabuf->attributes.stride,
> + dmabuf->attributes.offset, _id, 0);
> + if (ret)
> + goto done;
> +
> + fb = zalloc(sizeof *fb);
> + if (!fb)
> + goto done;

Nit: indentation

> +
> + fb->fb_id = fb_id;
> + fb->stride = dmabuf->attributes.stride[0];
> + fb->fd = backend->drm.fd;
> + fb->is_dmabuf = true;
> +
> +done:
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + if (!handles[i])
> + continue;
> + close_drm_handle(backend->drm.fd, handles[i]);

No need for `continue`, you can remove it and invert the if.

> + }
> +
> + return fb;
> +}
> +
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> + drm_fb_destroy(fb, fb->fd);
> +}

Not sure this is really needed :)

> +
>  static struct drm_fb *
>  drm_fb_get_from_bo(struct gbm_bo *bo,
>  struct drm_backend *backend, uint32_t format)
> @@ -475,6 +548,8 @@ drm_output_release_fb(struct drm_output *output, struct 
> drm_fb *fb)
>   if (fb->map &&
>  (fb != output->dumb[0] && fb != output->dumb[1])) {
>   drm_fb_destroy_dumb(fb);
> + } else if (fb->is_dmabuf) {
> + drm_fb_destroy_dmabuf(fb);
>   } else if (fb->bo) {
>   if (fb->is_client_buffer)
>   gbm_bo_destroy(fb->bo);
> @@ -486,12 +561,12 @@ drm_output_release_fb(struct drm_output *output, struct 
> drm_fb *fb)
>  
>  static uint32_t
>  drm_output_check_scanout_format(struct drm_output *output,
> - struct weston_surface *es, struct gbm_bo *bo)
> + struct weston_surface *es, uint32_t format)
>  {
> - uint32_t format;
>   pixman_region32_t r;
>  
> - format = gbm_bo_get_format(bo);
> + /* We relay on the GBM format enum and DRM format enum to be

"rely"

Otherwise it looks good :)

Cheers,
  Eric

> +identical */
>  
>   if (format == GBM_FORMAT_ARGB) {
>   /* We can scanout an ARGB buffer if the surface's
> @@ -521,12 +596,13 @@ drm_output_prepare_scanout_view(struct drm_output 
> *output,
>   struct drm_backend *b = to_drm_backend(output->base.compositor);
>   struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
>   struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
> - struct gbm_bo *bo;
> + struct linux_dmabuf_buffer *dmabuf;
> + struct gbm_bo *bo = NULL;
>   uint32_t format;
>  
>   if (ev->geometry.x != output->base.x ||
>   ev->geometry.y != output->base.y ||
> - buffer == NULL || b->gbm == NULL ||
> + buffer == NULL ||
>   buffer->width != output->base.current_mode->width ||
>   

Re: [PATCH libinput 1/3] Mark some internal log functions as printf-style function

[Eric Engestrom]

On Monday, 2016-10-24 11:45:17 +1000, Peter Hutterer wrote:
> Fixes the respective clang warnings
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> ---
>  src/libinput.c|  6 ++
>  test/litest.c | 16 
>  test/misc.c   |  7 +++
>  tools/event-gui.c |  8 
>  tools/shared.c|  7 +++
>  5 files changed, 44 insertions(+)
> 
> diff --git a/src/libinput.c b/src/libinput.c
> index 6958042..ec1c72a 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -163,6 +163,12 @@ static void
>  libinput_default_log_func(struct libinput *libinput,
> enum libinput_log_priority priority,
> const char *format, va_list args)
> + LIBINPUT_ATTRIBUTE_PRINTF(3, 0);
> +
> +static void
> +libinput_default_log_func(struct libinput *libinput,
> +   enum libinput_log_priority priority,
> +   const char *format, va_list args)
>  {

I'm not sure why you're adding a prototype here; is there also a warning
about this? (There shouldn't be, it's a static function.) If so, mention it
in the commit log?

Anyway, I'm pretty sure just adding this one line would fix the printf
warning too:

+LIBINPUT_ATTRIBUTE_PRINTF(3, 0)
 static void
 libinput_default_log_func(struct libinput *libinput,
  enum libinput_log_priority priority,
  const char *format, va_list args)
+/* You can also add the attribute here if you prefer */
 {

(Obviously, the same suggestion applies to all the functions in this patch.)

Patches 2 & 3 look good to me:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

>   const char *prefix;
>  
> diff --git a/test/litest.c b/test/litest.c
> index 4c301b5..940cf79 100644
> --- a/test/litest.c
> +++ b/test/litest.c
> @@ -270,6 +270,15 @@ litest_fail_condition(const char *file,
> const char *condition,
> const char *message,
> ...)
> + LIBINPUT_ATTRIBUTE_PRINTF(5, 6);
> +
> +void
> +litest_fail_condition(const char *file,
> +   int line,
> +   const char *func,
> +   const char *condition,
> +   const char *message,
> +   ...)
>  {
>   litest_log("FAILED: %s\n", condition);
>  
> @@ -761,6 +770,13 @@ litest_log_handler(struct libinput *libinput,
>  enum libinput_log_priority pri,
>  const char *format,
>  va_list args)
> +LIBINPUT_ATTRIBUTE_PRINTF(3, 0);
> +
> +static void
> +litest_log_handler(struct libinput *libinput,
> +enum libinput_log_priority pri,
> +const char *format,
> +va_list args)
>  {
>   const char *priority = NULL;
>  
> diff --git a/test/misc.c b/test/misc.c
> index 791ebc3..44c4502 100644
> --- a/test/misc.c
> +++ b/test/misc.c
> @@ -852,6 +852,13 @@ simple_log_handler(struct libinput *libinput,
>  enum libinput_log_priority priority,
>  const char *format,
>  va_list args)
> +LIBINPUT_ATTRIBUTE_PRINTF(3, 0);
> +
> +static void
> +simple_log_handler(struct libinput *libinput,
> +enum libinput_log_priority priority,
> +const char *format,
> +va_list args)
>  {
>   vfprintf(stderr, format, args);
>  }
> diff --git a/tools/event-gui.c b/tools/event-gui.c
> index b67ca45..e5fb26a 100644
> --- a/tools/event-gui.c
> +++ b/tools/event-gui.c
> @@ -110,6 +110,10 @@ struct window {
>  
>  static int
>  error(const char *fmt, ...)
> + LIBINPUT_ATTRIBUTE_PRINTF(1, 2);
> +
> +static int
> +error(const char *fmt, ...)
>  {
>   va_list args;
>   fprintf(stderr, "error: ");
> @@ -123,6 +127,10 @@ error(const char *fmt, ...)
>  
>  static void
>  msg(const char *fmt, ...)
> + LIBINPUT_ATTRIBUTE_PRINTF(1, 2);
> +
> +static void
> +msg(const char *fmt, ...)
>  {
>   va_list args;
>   printf("info: ");
> diff --git a/tools/shared.c b/tools/shared.c
> index 95655ba..f539957 100644
> --- a/tools/shared.c
> +++ b/tools/shared.c
> @@ -70,6 +70,13 @@ log_handler(struct libinput *li,
>   enum libinput_log_priority priority,
>   const char *format,
>   va_list args)
> + LIBINPUT_ATTRIBUTE_PRINTF(3, 0);
> +
> +static void
> +log_handler(struct libinput *li,
> + enum libinput_log_priority priority,
> + const char *format,
> + va_list args)
>  {
>   vprintf(format, args);
>  }
> -- 
> 2.9.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

[PATCH weston v2 1/2] config-parser: add support for more color formats

[Eric Engestrom]

Supported colour formats are:
- (0x)(AA)RRGGBB
- #RGB(A)
- #RRGGBB(AA)

Cc: Bryce Harrington <br...@osg.samsung.com>
Cc: Quentin Glidic <sardemff7+wayl...@sardemff7.net>
Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
v2: reduce the number of formats supported
read #-prefixed values the same way CSS would
---
 shared/config-parser.c | 54 ++
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index e2b5fa2..b3848a2 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -215,6 +215,12 @@ weston_config_section_get_uint(struct 
weston_config_section *section,
return 0;
 }
 
+/*
+ * Supported colour formats:
+ * - (0x)(AA)RRGGBB
+ * - #RGB(A)
+ * - #RRGGBB(AA)
+ */
 WL_EXPORT
 int
 weston_config_section_get_color(struct weston_config_section *section,
@@ -224,6 +230,10 @@ weston_config_section_get_color(struct 
weston_config_section *section,
struct weston_config_entry *entry;
int len;
char *end;
+   const char alpha[] = "FF"; /* Default alpha when unspecified */
+   const char *val;
+   const char *color_string;
+   enum { hex, css } colour_type;
 
entry = config_section_get_entry(section, key);
if (entry == NULL) {
@@ -232,19 +242,47 @@ weston_config_section_get_color(struct 
weston_config_section *section,
return -1;
}
 
+   val = entry->value;
+   len = strlen(val);
-   len = strlen(entry->value);
+
-   if (len == 1 && entry->value[0] == '0') {
+   if (len == 1 && val[0] == '0') {
*color = 0;
return 0;
-   } else if (len != 8 && len != 10) {
-   *color = default_color;
-   errno = EINVAL;
-   return -1;
}
 
+   if (len > 2 && val[0] == '0' && val[1] == 'x') {
+   val += 2;
+   len -= 2;
+   colour_type = hex;
+   }
+   else if (len > 1 && val[0] == '#') {
+   val += 1;
+   len -= 1;
+   colour_type = css;
+   }
+   else
+   colour_type = hex;
+
+   if (colour_type == hex)
+   switch (len) {
+   case 8: color_string = (char[]){   val[0],   val[1], val[2], 
val[3], val[4], val[5], val[6], val[7], '\0' }; break; /* (0x)AARRGGBB */
+   case 6: color_string = (char[]){ alpha[0], alpha[1], val[0], 
val[1], val[2], val[3], val[4], val[5], '\0' }; break; /*   (0x)RRGGBB */
+   default: goto invalid;
+   }
+
+   if (colour_type == css)
+   switch (len) {
+   case 8: color_string = (char[]){   val[6],   val[7], val[0], 
val[1], val[2], val[3], val[4], val[5], '\0' }; break; /* #RRGGBBAA */
+   case 6: color_string = (char[]){ alpha[0], alpha[1], val[0], 
val[1], val[2], val[3], val[4], val[5], '\0' }; break; /* #RRGGBB   */
+   case 4: color_string = (char[]){   val[3],   val[3], val[0], 
val[0], val[1], val[1], val[2], val[2], '\0' }; break; /* # R G B A */
+   case 3: color_string = (char[]){ alpha[0], alpha[1], val[0], 
val[0], val[1], val[1], val[2], val[2], '\0' }; break; /* # R G B   */
+   default: goto invalid;
+   }
+
errno = 0;
-   *color = strtoul(entry->value, , 16);
+   *color = strtoul(color_string, , 16);
-   if (errno != 0 || end == entry->value || *end != '\0') {
+   if (errno != 0 || end == color_string || *end != '\0') {
+invalid:
*color = default_color;
errno = EINVAL;
return -1;
-- 
Cheers,
  Eric
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 2/2] config-parser-test: test support for more color formats

[Eric Engestrom]

Add tests for all the supported colour formats

Cc: Bryce Harrington <br...@osg.samsung.com>
Cc: Quentin Glidic <sardemff7+wayl...@sardemff7.net>
Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
v2: reduce the number of formats supported
read #-prefixed values the same way CSS would
---
 tests/config-parser-test.c | 96 ++
 1 file changed, 96 insertions(+)

diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index 1cd..e35e2b8 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -127,6 +127,12 @@ static struct zuc_fixture config_test_t1 = {
"oct=01234567\n"
"dec=12345670\n"
"short=1234567\n"
+   "hex6=0x123456\n"
+   "hex8=0x12345678\n"
+   "css3=#123\n"
+   "css4=#1234\n"
+   "css6=#123456\n"
+   "css8=#12345678\n"
"\n"
"[stuff]\n"
"flag= true \n"
@@ -609,6 +615,96 @@ ZUC_TEST_F(config_test_t1, test028, data)
ZUC_ASSERT_EQ(ERANGE, errno);
 }
 
+ZUC_TEST_F(config_test_t1, test029, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "hex6", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff123456, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test030, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "hex8", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0x12345678, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test031, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "css3", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff112233, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test032, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "css4", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0x44112233, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test033, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "css6", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff123456, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test034, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "css8", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0x78123456, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
 ZUC_TEST_F(config_test_t2, doesnt_parse, data)
 {
struct weston_config *config = data;
-- 
Cheers,
  Eric
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 2/2] config-parser-test: test support for more color formats

[Eric Engestrom]

Test for all the possible lengths (up to 8), specifying which are
expected to be valid and how they should be interpreted.
Sprinkle some `#` and `0x` around randomly to make sure those work at
any length too.

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 tests/config-parser-test.c | 160 +
 1 file changed, 160 insertions(+)

diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index 1cd..a1038e5 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -127,6 +127,16 @@ static struct zuc_fixture config_test_t1 = {
"oct=01234567\n"
"dec=12345670\n"
"short=1234567\n"
+   "hash=#12345678\n"
+   "zerox=0x12345678\n"
+   "one=0x1\n"
+   "two=0x12\n"
+   "three=123\n"
+   "four=#1234\n"
+   "five=#12345\n"
+   "six=0x123456\n"
+   "seven=1234567\n"
+   "eight=12345678\n"
"\n"
"[stuff]\n"
"flag= true \n"
@@ -609,6 +619,156 @@ ZUC_TEST_F(config_test_t1, test028, data)
ZUC_ASSERT_EQ(ERANGE, errno);
 }
 
+ZUC_TEST_F(config_test_t1, test029, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "hash", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0x12345678, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test030, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "zerox", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0x12345678, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test031, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "one", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff11, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test032, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "two", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff121212, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test033, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "three", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff112233, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test034, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "four", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0x11223344, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test035, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "five", , 0xff336699);
+
+   ZUC_ASSERT_EQ(-1, r);
+   ZUC_ASSERT_EQ(0xff336699, n);
+   ZUC_ASSERT_EQ(EINVAL, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test036, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "colors", NULL, NULL);
+   r = weston_config_section_get_color(section, "six", , 0xff336699);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0xff123456, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test037, data)
+{
+   int r;
+   uint32_t n;
+   struct wes

[PATCH weston 1/2] config-parser: add support for more color formats

[Eric Engestrom]

Valid colours start with an optional '0x' or '#', followed by:
- AARRGGBB
-   RRGGBB
-  A R G B
-R G B
-   XYXYXY
-   XX

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---

I just stumbled back on this discussion from a few months ago, and
decided to give it a go.

Once again, I apologise for length of the lines in the switch, but
I believe they are much less readable when folded.

I feel like there should be a place where the supported colour formats$
are documented, but I couldn't find it?

---
 shared/config-parser.c | 39 +++
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index e2b5fa2..a00b2d6 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -224,6 +224,9 @@ weston_config_section_get_color(struct 
weston_config_section *section,
struct weston_config_entry *entry;
int len;
char *end;
+   const char alpha[] = "FF"; /* Default alpha when unspecified */
+   const char *val;
+   const char *color_string;
 
entry = config_section_get_entry(section, key);
if (entry == NULL) {
@@ -232,19 +235,39 @@ weston_config_section_get_color(struct 
weston_config_section *section,
return -1;
}
 
+   val = entry->value;
-   len = strlen(entry->value);
+   len = strlen(val);
+
-   if (len == 1 && entry->value[0] == '0') {
+   if (len == 1 && val[0] == '0') {
*color = 0;
return 0;
+   }
-   } else if (len != 8 && len != 10) {
-   *color = default_color;
-   errno = EINVAL;
-   return -1;
+
+   if (len > 2 && val[0] == '0' && val[1] == 'x')
+   {
+   val += 2;
+   len -= 2;
+   }
+   else if (len > 1 && val[0] == '#')
+   {
+   val += 1;
+   len -= 1;
+   }
+
+   switch (len) {
+   case 8: color_string = (char[]){   val[0],   val[1], val[2], val[3], 
val[4], val[5], val[6], val[7], '\0' }; break; /* AARRGGBB */
+   case 6: color_string = (char[]){ alpha[0], alpha[1], val[0], val[1], 
val[2], val[3], val[4], val[5], '\0' }; break; /*   RRGGBB */
+   case 4: color_string = (char[]){   val[0],   val[0], val[1], val[1], 
val[2], val[2], val[3], val[3], '\0' }; break; /*  A R G B */
+   case 3: color_string = (char[]){ alpha[0], alpha[1], val[0], val[0], 
val[1], val[1], val[2], val[2], '\0' }; break; /*R G B */
+   case 2: color_string = (char[]){ alpha[0], alpha[1], val[0], val[1], 
val[0], val[1], val[0], val[1], '\0' }; break; /*   XYXYXY */
+   case 1: color_string = (char[]){ alpha[0], alpha[1], val[0], val[0], 
val[0], val[0], val[0], val[0], '\0' }; break; /*   XX */
+   default: goto invalid;
}
 
errno = 0;
-   *color = strtoul(entry->value, , 16);
+   *color = strtoul(color_string, , 16);
-   if (errno != 0 || end == entry->value || *end != '\0') {
+   if (errno != 0 || end == color_string || *end != '\0') {
+invalid:
*color = default_color;
errno = EINVAL;
return -1;
-- 
Cheers,
  Eric

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

Re: [weston] gl-renderer: add support of WL_SHM_FORMAT_YUYV

[Eric Engestrom]

On Thursday, 2016-10-20 13:12:43 +0200, Vincent Abriou wrote:
> This patch allow gl-renderer to accept WL_SHM_FORMAT_YUYV buffers.
> This is the pixel format supported by most of the USB webcams.
> 
> Signed-off-by: Vincent Abriou <vincent.abr...@st.com>
> ---
>  libweston/gl-renderer.c | 56 
> +
>  1 file changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
> index 9747de5..a02cb3b 100644
> --- a/libweston/gl-renderer.c
> +++ b/libweston/gl-renderer.c
> @@ -162,7 +162,8 @@ struct gl_surface_state {
>  
>   /* Extension needed for SHM YUV texture */
>   int offset[3]; /* offset per plane */
> - int hvsub[3];  /* horizontal vertical subsampling per plane */
> + int hsub[3];  /* horizontal subsampling per plane */
> + int vsub[3];  /* vertical subsampling per plane */

This `3` and the related `4` are appearing quite often, and should
probably be #define'd. This is unrelated to this patch though :)

>  
>   struct weston_surface *surface;
>  
> @@ -1271,8 +1272,8 @@ gl_renderer_flush_damage(struct weston_surface *surface)
>   glBindTexture(GL_TEXTURE_2D, gs->textures[j]);
>   glTexImage2D(GL_TEXTURE_2D, 0,
>gs->gl_format[j],
> -  gs->pitch / gs->hvsub[j],
> -  buffer->height / gs->hvsub[j],
> +  gs->pitch / gs->hsub[j],
> +  buffer->height / gs->vsub[j],
>0,
>gs->gl_format[j],
>gs->gl_pixel_type,
> @@ -1293,8 +1294,8 @@ gl_renderer_flush_damage(struct weston_surface *surface)
>   glBindTexture(GL_TEXTURE_2D, gs->textures[j]);
>   glTexImage2D(GL_TEXTURE_2D, 0,
>gs->gl_format[j],
> -  gs->pitch / gs->hvsub[j],
> -  buffer->height / gs->hvsub[j],
> +  gs->pitch / gs->hsub[j],
> +  buffer->height / gs->vsub[j],
>0,
>gs->gl_format[j],
>gs->gl_pixel_type,
> @@ -1316,10 +1317,10 @@ gl_renderer_flush_damage(struct weston_surface 
> *surface)
>   for (j = 0; j < gs->num_textures; j++) {
>   glBindTexture(GL_TEXTURE_2D, gs->textures[j]);
>   glTexSubImage2D(GL_TEXTURE_2D, 0,
> - r.x1 / gs->hvsub[j],
> - r.y1 / gs->hvsub[j],
> - (r.x2 - r.x1) / gs->hvsub[j],
> - (r.y2 - r.y1) / gs->hvsub[j],
> + r.x1 / gs->hsub[j],
> + r.y1 / gs->hsub[j],
> + (r.x2 - r.x1) / gs->vsub[j],
> + (r.y2 - r.y1) / gs->vsub[j],

I think you got your horizontal and vertical subsampling crossed here.
Unless I'm mistaken, this should be:
r.x1 / gs->hsub[j],
r.y1 / gs->vsub[j],
(r.x2 - r.x1) / gs->hsub[j],
(r.y2 - r.y1) / gs->vsub[j],

The rest looks good to me, so with that fixed:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

>   gs->gl_format[j],
>   gs->gl_pixel_type,
>   data + gs->offset[j]);
> @@ -1373,7 +1374,8 @@ gl_renderer_attach_shm(struct weston_surface *es, 
> struct weston_buffer *buffer,
>  
>   num_planes = 1;
>   gs->offset[0] = 0;
> - gs->hvsub[0] = 1;
> + gs->hsub[0] = 1;
> + gs->vsub[0] = 1;
>  
>   switch (wl_shm_buffer_get_format(shm_buffer)) {
>   case WL_SHM_FORMAT_XRGB:
> @@ -1399,12 +1401,14 @@ gl_renderer_attach_shm(struct weston_surface *es, 
> struct weston_buffer *buffer,
>   pitch = wl_shm_buffer_get_stride(shm_buffer);
>   gl_pixel_type = GL_UNSIGNED_BYTE;
>   num_planes = 3;
> - gs->offset[1] = gs->offset[0] + (pitch / gs->hvsub[0]) *
> - (buffer->height / gs->hvsub[0]);
> - gs->hvsub[1] = 2;
> - gs->offset[2] = gs->

Re: [PATCH weston 1/2] config-parser: add support for more color formats

[Eric Engestrom]

On Friday, 2016-10-21 12:30:07 +0200, Quentin Glidic wrote:
> Hi,
> 
> On 20/10/2016 00:08, Eric Engestrom wrote:
> > Valid colours start with an optional '0x' or '#', followed by:
> > - AARRGGBB
> > -   RRGGBB
> > -  A R G B
> > -R G B
> > -   XYXYXY
> > -   XX
> 
> I think this is way too much, even with minimal code effort.
> 
> The well-known CSS formats are #RRGGBB and #RGB, and the backward-compatible
> extension are #RRGGBBAA and #RGBA.
> The current 0xAARRGGBB are directly derived from the internal usage of a
> colour as a number, because Weston devs are used to this. But *users* are
> not, they are used to CSS notation.
> 
> IMO, 0x should be used as-is, with a little clamping, so the old way
> continue to work. OTOH, the # notation should be fully compatible with CSS.
> 
> This code would lead to user-unexpected behaviour, and it’s easy to avoid
> that.

You're right, I didn't really think this through :)
I'll send a v2 with only commonly used formats/orders when
I have some more time (probably sunday):

- (0x)(AA)RRGGBB (backward compatibility)
- #RGB(A)
- #RRGGBB(AA)

Bryce, I think you wanted a single repeating char format; do you also
want the 2-char format?

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

Re: [PATCH weston] xwayland: Remove pointless newline in X11 lock

[Eric Engestrom]

On Wednesday, 2016-11-16 16:37:59 +, Daniel Stone wrote:
> The newline was always chopped off by snprintf: pid is only 11

Don't you mean 10?
Reviewed-by: Eric Engestrom <e...@engestrom.ch>

> characters long, and we were asking to print 11 characters plus the
> terminating NULL. Hence snprintf would always chop the newline anyway.
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> Cc: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> ---
>  xwayland/launcher.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xwayland/launcher.c b/xwayland/launcher.c
> index 97d7c6e..31599d6 100644
> --- a/xwayland/launcher.c
> +++ b/xwayland/launcher.c
> @@ -201,7 +201,7 @@ create_lockfile(int display, char *lockfile, size_t lsize)
>  
>   /* Subtle detail: we use the pid of the wayland
>* compositor, not the xserver in the lock file. */
> - size = snprintf(pid, sizeof pid, "%10d\n", getpid());
> + size = snprintf(pid, sizeof pid, "%10d", getpid());
>   if (write(fd, pid, size) != size) {
>   unlink(lockfile);
>   close(fd);
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] xwayland: Fix X11 lock file size confusion

[Eric Engestrom]

On Thursday, 2016-11-17 11:35:53 +, Daniel Stone wrote:
> The X11 lock file was somewhat opaque. Into a sized array of 16
> characters, we previously read 11 bytes. 61beda653b fixed the parsing of
> this input to ensure that we only considered the first 10 bytes: this
> has the effect of culling a LF byte at the end of the string.
> 
> This commit more explicitly NULLs the entire string before reading, and
> trims trailing LF characters only.
> 
> It also adds some documentation by way of resizing pid, an explicit size
> check on snprintf's return, and comments.
> 
> Related Mutter issue: https://bugzilla.gnome.org/show_bug.cgi?id=774613
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> Cc: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> Cc: Eric Engestrom <e...@engestrom.ch>
> ---
>  xwayland/launcher.c | 25 +++--
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/xwayland/launcher.c b/xwayland/launcher.c
> index 97d7c6e..56b949e 100644
> --- a/xwayland/launcher.c
> +++ b/xwayland/launcher.c
> @@ -148,13 +148,19 @@ bind_to_unix_socket(int display)
>  static int
>  create_lockfile(int display, char *lockfile, size_t lsize)
>  {
> - char pid[16];
> + /* 10 decimal characters, trailing LF and NUL byte; see comment
> +  * at end of function. */
> + char pid[12];
>   int fd, size;
>   pid_t other;
>  
>   snprintf(lockfile, lsize, "/tmp/.X%d-lock", display);
>   fd = open(lockfile, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444);
>   if (fd < 0 && errno == EEXIST) {
> + /* Clear our input buffer so we always have a NUL-terminated
> +  * string. */
> + memset(pid, 0, sizeof(pid));

Isn't that a bit of overkill? Clear all the bytes just before
overwriting almost all of them, just so the last one is null?
I know it's only 12 bytes, but it still looks weird to me :)

> +
>   fd = open(lockfile, O_CLOEXEC | O_RDONLY);
>   if (fd < 0 || read(fd, pid, 11) != 11) {
>   weston_log("can't read lock file %s: %s\n",
> @@ -166,8 +172,10 @@ create_lockfile(int display, char *lockfile, size_t 
> lsize)
>   return -1;
>   }
>  
> - /* Trim the newline, ensure terminated string. */
> - pid[10] = '\0';
> + /* If the last character is LF, trim it so safe_strtoint
> +  * doesn't explode. */
> + if (pid[10] == '\n')
> + pid[10] = '\0';

Is there any situation where this `if` isn't going to be true?

>  
>   if (!safe_strtoint(pid, )) {
>   weston_log("can't parse lock file %s\n",
> @@ -199,10 +207,15 @@ create_lockfile(int display, char *lockfile, size_t 
> lsize)
>   return -1;
>   }
>  
> - /* Subtle detail: we use the pid of the wayland
> -  * compositor, not the xserver in the lock file. */
> + /* Subtle detail: we use the pid of the wayland compositor, not the
> +  * xserver in the lock file. Another subtlety: snprintf accepts a

Nit: newline before "Another subtlety" to make it more prominent.

> +  * size value which includes the trailing NUL byte, and returns a
> +  * size value which does not (already off by one), which is the
> +  * number of characters it would've hypothetically written, rather
> +  * than the number it actually wrote. Here we don't want to write
> +  * the trailing NUL byte. */
>   size = snprintf(pid, sizeof pid, "%10d\n", getpid());
> - if (write(fd, pid, size) != size) {
> + if (size != 11 || write(fd, pid, size) != size) {

Why aren't we just doing this here?
size = dprintf(fd, "%10d\n", getpid());
if (size != 11) {

Other than that, big +1 on documenting things :)

Cheers,
  Eric

>   unlink(lockfile);
>   close(fd);
>   return -1;
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 5/9] clients/simple-dmabuf-intel: rename to simple-dmabuf-drm

[Eric Engestrom]

On Thursday, 2016-11-17 17:25:59 +0530, Varad Gautam wrote:
> From: Varad Gautam 
> 
> this will allow adding other drm backends later.
> 
> Signed-off-by: Varad Gautam 
> ---
>  Makefile.am   |  12 +-
>  clients/simple-dmabuf-drm.c   | 668 
> ++
>  clients/simple-dmabuf-intel.c | 668 
> --
>  configure.ac  |  22 +-
>  4 files changed, 685 insertions(+), 685 deletions(-)
>  create mode 100644 clients/simple-dmabuf-drm.c
>  delete mode 100644 clients/simple-dmabuf-intel.c

Hint: next time, you could use `git format-patch -M` to show renames,
instead of delete + create.
Makes reading the diff (and therefore reviewing) much easier :)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2] xwayland: Fix X11 lock file size confusion

[Eric Engestrom]

On Thursday, 2016-11-17 12:17:59 +, Daniel Stone wrote:
> The X11 lock file was somewhat opaque. Into a sized array of 16
> characters, we previously read 11 bytes. 61beda653b fixed the parsing of
> this input to ensure that we only considered the first 10 bytes: this
> has the effect of culling a LF byte at the end of the string.
> 
> This commit more explicitly NULLs the entire string before reading, and
> trims trailing LF characters only.
> 
> It also adds some documentation by way of resizing pid, an explicit size
> check on snprintf's return, and comments.
> 
> Verified manually that it emits lock files with a trailing \n, as Xorg
> does. Also verified manually that it ignores misformatted lock files,
> but accepts either \n or \0 in the trailing position.
> 
> Related Mutter issue: https://bugzilla.gnome.org/show_bug.cgi?id=774613
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> Cc: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> Cc: Eric Engestrom <e...@engestrom.ch>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  xwayland/launcher.c | 24 +---
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/xwayland/launcher.c b/xwayland/launcher.c
> index 97d7c6e..c3114f5 100644
> --- a/xwayland/launcher.c
> +++ b/xwayland/launcher.c
> @@ -148,13 +148,19 @@ bind_to_unix_socket(int display)
>  static int
>  create_lockfile(int display, char *lockfile, size_t lsize)
>  {
> - char pid[16];
> + /* 10 decimal characters, trailing LF and NUL byte; see comment
> +  * at end of function. */
> + char pid[11];
>   int fd, size;
>   pid_t other;
>  
>   snprintf(lockfile, lsize, "/tmp/.X%d-lock", display);
>   fd = open(lockfile, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444);
>   if (fd < 0 && errno == EEXIST) {
> + /* Clear our input buffer so we always have a NUL-terminated
> +  * string. */
> + memset(pid, 0, sizeof(pid));
> +
>   fd = open(lockfile, O_CLOEXEC | O_RDONLY);
>   if (fd < 0 || read(fd, pid, 11) != 11) {
>   weston_log("can't read lock file %s: %s\n",
> @@ -166,8 +172,10 @@ create_lockfile(int display, char *lockfile, size_t 
> lsize)
>   return -1;
>   }
>  
> - /* Trim the newline, ensure terminated string. */
> - pid[10] = '\0';
> + /* If the last character is LF, trim it so safe_strtoint
> +  * doesn't explode. */
> + if (pid[10] == '\n')
> + pid[10] = '\0';
>  
>   if (!safe_strtoint(pid, )) {
>   weston_log("can't parse lock file %s\n",
> @@ -199,10 +207,12 @@ create_lockfile(int display, char *lockfile, size_t 
> lsize)
>   return -1;
>   }
>  
> - /* Subtle detail: we use the pid of the wayland
> -  * compositor, not the xserver in the lock file. */
> - size = snprintf(pid, sizeof pid, "%10d\n", getpid());
> - if (write(fd, pid, size) != size) {
> + /* Subtle detail: we use the pid of the wayland compositor, not the
> +  * xserver in the lock file.
> +  * Also subtle is that we don't emit a trailing NUL to the file, so
> +  * our size here is 11 rather than 12. */
> + size = dprintf(fd, "%10d\n", getpid());
> + if (size != 11) {
>   unlink(lockfile);
>   close(fd);
>   return -1;
> -- 
> 2.9.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 1/2] gl-renderer: add support for EGL_KHR_swap_buffers_with_damage

[Eric Engestrom]

On Thursday, 2016-11-03 22:38:19 +, Emil Velikov wrote:
> Extension is identical to the EXT one, yet we need to check for the KHR
> abbreviated extension name + entry-point.
> 
> Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
> ---
>  libweston/gl-renderer.c | 29 +++--
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
> index 3e8e5ab..a585001 100644
> --- a/libweston/gl-renderer.c
> +++ b/libweston/gl-renderer.c
> @@ -2781,6 +2781,18 @@ renderer_setup_egl_client_extensions(struct 
> gl_renderer *gr)
>  static int
>  gl_renderer_setup_egl_extensions(struct weston_compositor *ec)
>  {
> + static const struct {
> + char *extension, *entrypoint;
> + } foo[] = {

Since the goal of this table is to get the entrypoint when you found an
extension, how about `swap_buffers_with_damage_ext_to_entrypoint`?
Rather long I'll concede, but explicit.
You can also drop the middle bit: `swap_damage_ext_to_entrypoint`

With an appropriate name here (and the other patch), the series is:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> + {
> + .extension = "EGL_EXT_swap_buffers_with_damage",
> + .entrypoint = "eglSwapBuffersWithDamageEXT",
> + },
> + {
> + .extension = "EGL_KHR_swap_buffers_with_damage",
> + .entrypoint = "eglSwapBuffersWithDamageKHR",
> + },
> + };
>   struct gl_renderer *gr = get_renderer(ec);
>   const char *extensions;
>   EGLBoolean ret;
> @@ -2815,12 +2827,17 @@ gl_renderer_setup_egl_extensions(struct 
> weston_compositor *ec)
>   weston_log("warning: EGL_EXT_buffer_age not supported. "
>  "Performance could be affected.\n");
>  
> - if (weston_check_egl_extension(extensions, 
> "EGL_EXT_swap_buffers_with_damage"))
> - gr->swap_buffers_with_damage =
> - (void *) 
> eglGetProcAddress("eglSwapBuffersWithDamageEXT");
> - else
> - weston_log("warning: EGL_EXT_swap_buffers_with_damage not "
> -"supported. Performance could be affected.\n");
> + for (unsigned i = 0; i < ARRAY_LENGTH(foo); i++) {

Yay for not forgetting `i<` :P

> + if (weston_check_egl_extension(extensions, foo[i].extension)) {
> + gr->swap_buffers_with_damage =
> + (void *) eglGetProcAddress(foo[i].entrypoint);
> + break;
> + }
> + }
> + if (!gr->swap_buffers_with_damage)
> + weston_log("warning: neither %s or %s is supported. "
> +"Performance could be affected.\n",
> +foo[0].extension, foo[1].extension);
>  
>   if (weston_check_egl_extension(extensions, 
> "EGL_MESA_configless_context"))
>   gr->has_configless_context = 1;
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3] clients: Add XKB compose key support

[Eric Engestrom]

On Mon, Oct 10, 2016 at 03:31:47PM -0700, Bryce Harrington wrote:
> This adds single-symbol compose support using libxkbcommon's compose
> functionality.  E.g., assuming you have the right alt key defined as
> your compose key, typing +i+' will produce í, and +y+= will
> produce ¥.  This makes compose key work for weston-editor,
> weston-terminal, weston-eventdemo, and any other clients that use
> Weston's window.* routines for accepting and managing keyboard input.
> 
> Compose sequences are loaded from the system's standard tables.  As
> well, libxkbcommon will transparently load custom sequences from the
> user's ~/.XCompose file.
> 
> Note that due to limitations in toytoolkit's key handler interface, only
> compose sequences resulting in single symbols are supported.  While
> libxkbcommon supports multi-symbol compose strings, support for passing
> text buffers to Weston clients is left as future work.
> 
> This largely obviates the need for the weston-simple-im input method
> client, which had provided a very limited compose functionality that was
> only available in clients implementing the zwp_input_method protocol,
> and with no mechanism to load system or user-specified compose keys.
> 
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=53648
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>
> Reviewed-by: Daniel Stone <dani...@collabora.com>
> 
> Signed-off-by: Bryce Harrington <br...@osg.samsung.com>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
> v3:  Don't die if compose can't be established
>Format a couple lengthy lines to 80 columns
>Also check LC_CTYPE and LANG for locale settings
> 
>  clients/window.c | 66 
> 
>  1 file changed, 66 insertions(+)
> 
> diff --git a/clients/window.c b/clients/window.c
> index 216ef96..1c53b5f 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -63,6 +63,7 @@ typedef void *EGLContext;
>  #endif /* no HAVE_CAIRO_EGL */
>  
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -372,6 +373,8 @@ struct input {
>   struct {
>   struct xkb_keymap *keymap;
>   struct xkb_state *state;
> + struct xkb_compose_table *compose_table;
> + struct xkb_compose_state *compose_state;
>   xkb_mod_mask_t control_mask;
>   xkb_mod_mask_t alt_mask;
>   xkb_mod_mask_t shift_mask;
> @@ -2979,6 +2982,9 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
> *keyboard,
>   struct input *input = data;
>   struct xkb_keymap *keymap;
>   struct xkb_state *state;
> + struct xkb_compose_table *compose_table;
> + struct xkb_compose_state *compose_state;
> + char *locale;
>   char *map_str;
>  
>   if (!data) {
> @@ -2997,6 +3003,7 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
> *keyboard,
>   return;
>   }
>  
> + /* Set up XKB keymap */
>   keymap = xkb_keymap_new_from_string(input->display->xkb_context,
>   map_str,
>   XKB_KEYMAP_FORMAT_TEXT_V1,
> @@ -3009,6 +3016,7 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
> *keyboard,
>   return;
>   }
>  
> + /* Set up XKB state */
>   state = xkb_state_new(keymap);
>   if (!state) {
>   fprintf(stderr, "failed to create XKB state\n");
> @@ -3016,6 +3024,37 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
> *keyboard,
>   return;
>   }
>  
> + /* Look up the preferred locale, falling back to "C" as default */
> + if (!(locale = getenv("LC_ALL")))
> + if (!(locale = getenv("LC_CTYPE")))
> + if (!(locale = getenv("LANG")))
> + locale = "C";
> +
> + /* Set up XKB compose table */
> + compose_table =
> + xkb_compose_table_new_from_locale(input->display->xkb_context,
> +   locale,
> +   XKB_COMPOSE_COMPILE_NO_FLAGS);
> + if (compose_table) {
> + /* Set up XKB compose state */
> + compose_state = xkb_compose_state_new(compose_table,
> +   XKB_COMPOSE_STATE_NO_FLAGS);
> + if (compose_state) {
> + xkb_compose_state_unref(input->xkb.compose_state);
> + xkb_compose_table_unref(input->xkb.compose_table);
> + 

Re: [PATCH weston] xwm: Add icon support to the frame

[Eric Engestrom]

On Sunday, 2016-12-04 21:20:47 +, Emmanuel Gil Peyrot wrote:
> This fetches the _NET_WM_ICON property of the X11 window, and use the
> first image found as the frame icon.
> 
> This has been tested with various X11 programs, and improves usability
> and user-friendliness a bit.
> 
> Signed-off-by: Emmanuel Gil Peyrot 
> ---
>  clients/window.c   |  4 +--
>  libweston/compositor-wayland.c |  2 +-
>  shared/cairo-util.h|  2 +-
>  shared/frame.c | 38 
>  xwayland/window-manager.c  | 56 
> +++---
>  5 files changed, 90 insertions(+), 12 deletions(-)
> 
> diff --git a/clients/window.c b/clients/window.c
> index ac35c3d..7e15969 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -2546,7 +2546,7 @@ window_frame_create(struct window *window, void *data)
>  
>   frame = xzalloc(sizeof *frame);
>   frame->frame = frame_create(window->display->theme, 0, 0,
> - buttons, window->title);
> + buttons, window->title, NULL);
>  
>   frame->widget = window_add_widget(window, frame);
>   frame->child = widget_add_widget(frame->widget, data);
> @@ -5444,7 +5444,7 @@ create_menu(struct display *display,
>   menu->user_data = user_data;
>   menu->widget = window_add_widget(menu->window, menu);
>   menu->frame = frame_create(window->display->theme, 0, 0,
> -FRAME_BUTTON_NONE, NULL);
> +FRAME_BUTTON_NONE, NULL, NULL);
>   fail_on_null(menu->frame, 0, __FILE__, __LINE__);
>   menu->entries = entries;
>   menu->count = count;
> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
> index d1e387d..4b053bf 100644
> --- a/libweston/compositor-wayland.c
> +++ b/libweston/compositor-wayland.c
> @@ -848,7 +848,7 @@ wayland_output_set_windowed(struct wayland_output *output)
>   }
>   }
>   output->frame = frame_create(b->theme, 100, 100,
> -  FRAME_BUTTON_CLOSE, title);
> +  FRAME_BUTTON_CLOSE, title, NULL);
>   free(title);
>   if (!output->frame)
>   return -1;
> diff --git a/shared/cairo-util.h b/shared/cairo-util.h
> index 84cf005..7893ca2 100644
> --- a/shared/cairo-util.h
> +++ b/shared/cairo-util.h
> @@ -126,7 +126,7 @@ enum {
>  
>  struct frame *
>  frame_create(struct theme *t, int32_t width, int32_t height, uint32_t 
> buttons,
> -  const char *title);
> + const char *title, cairo_surface_t *icon);
>  
>  void
>  frame_destroy(struct frame *frame);
> diff --git a/shared/frame.c b/shared/frame.c
> index eb0cd77..69f3e90 100644
> --- a/shared/frame.c
> +++ b/shared/frame.c
> @@ -131,6 +131,27 @@ frame_button_create(struct frame *frame, const char 
> *icon,
>   return button;
>  }
>  
> +static struct frame_button *
> +frame_button_create_from_surface(struct frame *frame, cairo_surface_t *icon,
> + enum frame_status status_effect,
> + enum frame_button_flags flags)
> +{
> + struct frame_button *button;

+if (!icon)
+   return NULL;

That way you could rewrite frame_button_create() as a simple:

return frame_button_create_from_surface(frame,
cairo_image_surface_create_from_png(icon),
status_effect, flags);

to avoid duplicating this code.

(While at it, I'd argue `frame` should also be null-checked, but
that's clearly unrelated to this patch.)

> +
> + button = calloc(1, sizeof *button);
> + if (!button)
> + return NULL;
> +
> + button->icon = icon;
> + button->frame = frame;
> + button->flags = flags;
> + button->status_effect = status_effect;
> +
> + wl_list_insert(frame->buttons.prev, >link);
> +
> + return button;
> +}
> +
>  static void
>  frame_button_destroy(struct frame_button *button)
>  {
> @@ -303,7 +324,7 @@ frame_destroy(struct frame *frame)
>  
>  struct frame *
>  frame_create(struct theme *t, int32_t width, int32_t height, uint32_t 
> buttons,
> -  const char *title)
> + const char *title, cairo_surface_t *icon)
>  {
>   struct frame *frame;
>   struct frame_button *button;
> @@ -330,10 +351,17 @@ frame_create(struct theme *t, int32_t width, int32_t 
> height, uint32_t buttons,
>   }
>  
>   if (title) {
> - button = frame_button_create(frame,
> -  DATADIR "/weston/icon_window.png",
> -  FRAME_STATUS_MENU,
> -  FRAME_BUTTON_CLICK_DOWN);
> + if (icon) {
> + button = frame_button_create_from_surface(frame,
> +   icon,
> +   

Re: [PATCH weston 1/2] os: Check for EINTR on posix_fallocate()

[Eric Engestrom]

On Thursday, 2017-03-23 11:59:22 -0500, Derek Foreman wrote:
> posix_fallocate() can return EINTR and need to be restarted - I've hit
> this when running weston-terminal under gdb.
> 
> Signed-off-by: Derek Foreman <der...@osg.samsung.com>

Both patches are:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

...and now I'm looking for these in $DAYJOB codebase ^^

> ---
>  shared/os-compatibility.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/shared/os-compatibility.c b/shared/os-compatibility.c
> index 551f2a99..6b2f3770 100644
> --- a/shared/os-compatibility.c
> +++ b/shared/os-compatibility.c
> @@ -178,7 +178,9 @@ os_create_anonymous_file(off_t size)
>   return -1;
>  
>  #ifdef HAVE_POSIX_FALLOCATE
> - ret = posix_fallocate(fd, 0, size);
> + do {
> + ret = posix_fallocate(fd, 0, size);
> + } while (ret == EINTR);
>   if (ret != 0) {
>   close(fd);
>   errno = ret;
> -- 
> 2.11.0
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland] connection-test: fix assert

[Eric Engestrom]

Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
---
 tests/connection-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/connection-test.c b/tests/connection-test.c
index 8be6c38..157e1bc 100644
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -603,8 +603,8 @@ suu_handler(void *data, struct wl_object *object,
int *done = data;
 
assert(strcmp(s, "foo") == 0);
-   assert(u1 = 500);
-   assert(u2 = 404040);
+   assert(u1 == 500);
+   assert(u2 == 404040);
*done = 1;
 }
 
-- 
Cheers,
  Eric

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

Re: [PATCH] [wayland] configure: add option to disable tests

[Eric Engestrom]

On Sunday, 2017-03-05 11:53:18 +0100, Yann E. MORIN wrote:
> When building for a product, tests are not needed.
> 
> Besides, one test requires a C++ compiler, which is not always
> available.
> 
> So, add an option to configure to disable building tests altogether.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1...@free.fr>
> ---
>  Makefile.am  | 3 ++-
>  configure.ac | 8 
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index d0c8bd3..9c2541d 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -143,7 +143,7 @@ libwayland_cursor_la_CFLAGS = \
>   -I$(top_srcdir)/src \
>   -DICONDIR=\"$(ICONDIR)\"
>  
> -
> +if ENABLE_TESTS
>  built_test_programs =\
>   array-test  \
>   client-test \
> @@ -258,6 +258,7 @@ os_wrappers_test_LDADD = libtest-runner.la
>  
>  exec_fd_leak_checker_SOURCES = tests/exec-fd-leak-checker.c
>  exec_fd_leak_checker_LDADD = libtest-runner.la
> +endif
>  
>  EXTRA_DIST += tests/scanner-test.sh  \
>   tests/data/example.xml  \
> diff --git a/configure.ac b/configure.ac
> index b583bef..96a5575 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -87,10 +87,18 @@ AC_ARG_ENABLE([dtd-validation],
> [],
> [enable_dtd_validation=yes])
>  
> +AC_ARG_ENABLE([tests],
> +   [AC_HELP_STRING([--disable-tests],
> +   [Disable compilation of test programs])],
> +   [],
> +   [enable_tests=yes])
> +
>  AM_CONDITIONAL(USE_HOST_SCANNER, test "x$with_host_scanner" = xyes)
>  
>  AM_CONDITIONAL(ENABLE_LIBRARIES, test "x$enable_libraries" = xyes)
>  
> +AM_CONDITIONAL(ENABLE_TESTS, test "x$enable_tests" = "yes")

`xyes` otherwise it'll never match, and you can drop the quotes :)

I think allowing to leave out tests is reasonable. I tested it a bit
too, to make sure it doesn't break `make check` or `make distcheck`, so
with the above fix, you can have my:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
Tested-by: Eric Engestrom <eric.engest...@imgtec.com>

Cheers,
  Eric

> +
>  AC_ARG_WITH(icondir, [  --with-icondir=Look for cursor icons here],
>[  ICONDIR=$withval],
>[  ICONDIR=${datadir}/icons])
> -- 
> 2.7.4
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] tools: add the libinput version to the man pages

[Eric Engestrom]

On Thursday, 2017-06-29 10:01:10 +1000, Peter Hutterer wrote:
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> ---
>  configure.ac   |  1 +
>  meson.build| 45 
> +++---
>  tools/Makefile.am  |  7 
>  ...ut-debug-events.1 => libinput-debug-events.man} |  2 +-
>  ...libinput-debug-gui.1 => libinput-debug-gui.man} |  2 +-
>  ...ut-list-devices.1 => libinput-list-devices.man} |  2 +-
>  ...pad-tap.1 => libinput-measure-touchpad-tap.man} |  2 +-
>  tools/{libinput-measure.1 => libinput-measure.man} |  2 +-
>  tools/{libinput.1 => libinput.man} |  2 +-
>  9 files changed, 53 insertions(+), 12 deletions(-)
>  rename tools/{libinput-debug-events.1 => libinput-debug-events.man} (97%)
>  rename tools/{libinput-debug-gui.1 => libinput-debug-gui.man} (94%)
>  rename tools/{libinput-list-devices.1 => libinput-list-devices.man} (94%)
>  rename tools/{libinput-measure-touchpad-tap.1 => 
> libinput-measure-touchpad-tap.man} (96%)
>  rename tools/{libinput-measure.1 => libinput-measure.man} (90%)
>  rename tools/{libinput.1 => libinput.man} (95%)
> 
> diff --git a/configure.ac b/configure.ac
> index 067c3e1a..a66371cd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -46,6 +46,7 @@ AC_USE_SYSTEM_EXTENSIONS
>  AC_PROG_CC_C99
>  AC_PROG_CXX # Only used by build C++ test
>  AC_PROG_GREP
> +AC_PROG_SED
>  
>  # Only used for testing the hwdb
>  AM_PATH_PYTHON([3.0],, [:])
> diff --git a/meson.build b/meson.build
> index bd9b6193..bc639718 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -357,6 +357,9 @@ lib_tools_shared = static_library('tools_shared',
>  dep_tools_shared = declare_dependency(link_with : lib_tools_shared,
> dependencies : deps_tools_shared)
>  
> +man_config = configuration_data()
> +man_config.set('LIBINPUT_VERSION', meson.project_version())
> +
>  deps_tools = [ dep_tools_shared, dep_libinput ]
>  libinput_debug_events_sources = [ 'tools/libinput-debug-events.c' ]
>  executable('libinput-debug-events',
> @@ -366,7 +369,12 @@ executable('libinput-debug-events',
>  install_dir : libinput_tool_path,
>  install : true
>  )
> -install_man('tools/libinput-debug-events.1')
> +configure_file(input : 'tools/libinput-debug-events.man',
> +output : 'libinput-debug-events.1',
> +configuration : man_config,

Well, it's a shame install_man() doesn't have a `configuration:`
argument :(

> +install : true,
> +install_dir : join_paths(get_option('mandir'), 'man1')

Took me a minute to find that get_option() allows you to query builtin
options as well [1]. Is there a list of those builtins somewhere?

[1] http://mesonbuild.com/Build-options.html

Assuming 'mandir' is indeed in that list,
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

Re: [PATCH libinput] tools: add the libinput version to the man pages

[Eric Engestrom]

On Friday, 2017-06-30 16:21:39 +0100, Eric Engestrom wrote:
> On Thursday, 2017-06-29 10:01:10 +1000, Peter Hutterer wrote:
> > Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> > ---
> >  configure.ac   |  1 +
> >  meson.build| 45 
> > +++---
> >  tools/Makefile.am  |  7 
> >  ...ut-debug-events.1 => libinput-debug-events.man} |  2 +-
> >  ...libinput-debug-gui.1 => libinput-debug-gui.man} |  2 +-
> >  ...ut-list-devices.1 => libinput-list-devices.man} |  2 +-
> >  ...pad-tap.1 => libinput-measure-touchpad-tap.man} |  2 +-
> >  tools/{libinput-measure.1 => libinput-measure.man} |  2 +-
> >  tools/{libinput.1 => libinput.man} |  2 +-
> >  9 files changed, 53 insertions(+), 12 deletions(-)
> >  rename tools/{libinput-debug-events.1 => libinput-debug-events.man} (97%)
> >  rename tools/{libinput-debug-gui.1 => libinput-debug-gui.man} (94%)
> >  rename tools/{libinput-list-devices.1 => libinput-list-devices.man} (94%)
> >  rename tools/{libinput-measure-touchpad-tap.1 => 
> > libinput-measure-touchpad-tap.man} (96%)
> >  rename tools/{libinput-measure.1 => libinput-measure.man} (90%)
> >  rename tools/{libinput.1 => libinput.man} (95%)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 067c3e1a..a66371cd 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -46,6 +46,7 @@ AC_USE_SYSTEM_EXTENSIONS
> >  AC_PROG_CC_C99
> >  AC_PROG_CXX # Only used by build C++ test
> >  AC_PROG_GREP
> > +AC_PROG_SED
> >  
> >  # Only used for testing the hwdb
> >  AM_PATH_PYTHON([3.0],, [:])
> > diff --git a/meson.build b/meson.build
> > index bd9b6193..bc639718 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -357,6 +357,9 @@ lib_tools_shared = static_library('tools_shared',
> >  dep_tools_shared = declare_dependency(link_with : lib_tools_shared,
> >   dependencies : deps_tools_shared)
> >  
> > +man_config = configuration_data()
> > +man_config.set('LIBINPUT_VERSION', meson.project_version())
> > +
> >  deps_tools = [ dep_tools_shared, dep_libinput ]
> >  libinput_debug_events_sources = [ 'tools/libinput-debug-events.c' ]
> >  executable('libinput-debug-events',
> > @@ -366,7 +369,12 @@ executable('libinput-debug-events',
> >install_dir : libinput_tool_path,
> >install : true
> >)
> > -install_man('tools/libinput-debug-events.1')
> > +configure_file(input : 'tools/libinput-debug-events.man',
> > +  output : 'libinput-debug-events.1',
> > +  configuration : man_config,
> 
> Well, it's a shame install_man() doesn't have a `configuration:`
> argument :(
> 
> > +  install : true,
> > +  install_dir : join_paths(get_option('mandir'), 'man1')
> 
> Took me a minute to find that get_option() allows you to query builtin
> options as well [1]. Is there a list of those builtins somewhere?

Ha! found it: when you run `mesonconf` with no argument, you get a list
of all the options, including the directories :)

> 
> [1] http://mesonbuild.com/Build-options.html
> 
> Assuming 'mandir' is indeed in that list,
> 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

Re: [PATCH libinput 3/5] test: allow running the symbols-leak-test.in script directly

[Eric Engestrom]

On Wednesday, 2017-04-26 12:20:39 +1000, Peter Hutterer wrote:
> With autotools, we replace the @top_srcdir@ during configure and then run teh
> resulting scripts.
> 
> With meson, it's easier to just pass top-srcdir it in as argument.
> 
> Signed-off-by: Peter Hutterer 
> ---
>  test/symbols-leak-test.in | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/test/symbols-leak-test.in b/test/symbols-leak-test.in
> index 448ef2f..69861c9 100755
> --- a/test/symbols-leak-test.in
> +++ b/test/symbols-leak-test.in
> @@ -2,6 +2,13 @@
>  
>  ### simple check for exported symbols
>  
> +# Allow running this after the configure.ac replacement or directly with an
> +# argument for the top_srcdir
> +TOP_SRCDIR="@top_srcdir@"
> +if [ "$TOP_SRCDIR" = "@top_srcdir@" ]; then

Isn't that always going to be true?

> + TOP_SRCDIR="$1"
> +fi
> +
>  # make bash exit if any command will exit with non-0 return value
>  set -e
>  
> @@ -9,9 +16,9 @@ set -e
>  cd `dirname $0`
>  
>  diff -a -u \
> - <(cat @top_srcdir@/src/libinput.sym | \
> + <(cat "$TOP_SRCDIR"/src/libinput.sym | \
>   grep '^\s\+libinput_.*' | \
>   sed -e 's/^\s\+\(.*\);/\1/' |  sort) \
> - <(cat @top_srcdir@/src/*.c | \
> + <(cat "$TOP_SRCDIR"/src/*.c | \
>   grep LIBINPUT_EXPORT -A 1 | grep '^libinput_.*' | \
>   sed -e 's/(.*//' | sort)
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput] libinput: rewrite loop to make it more readable

[Eric Engestrom]

Rewrite
  foo(); while(...) { foo(); }
into
  do { foo(); } while(...);
to avoid duplication and make the flow easier to read and understand.

Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
---
 src/libinput.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/libinput.c b/src/libinput.c
index 472b1c0..e84e3c3 100644
--- a/src/libinput.c
+++ b/src/libinput.c
@@ -54,15 +54,15 @@ check_event_type(struct libinput *libinput,
unsigned int type_permitted;
 
va_start(args, type_in);
-   type_permitted = va_arg(args, unsigned int);
 
-   while (type_permitted != (unsigned int)-1) {
+   do {
+   type_permitted = va_arg(args, unsigned int);
+
if (type_permitted == type_in) {
rc = true;
break;
}
-   type_permitted = va_arg(args, unsigned int);
-   }
+   } while (type_permitted != (unsigned int)-1);
 
va_end(args);
 
-- 
Cheers,
  Eric

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

Re: [PATCH libinput 5/5] evdev: fix a compiler warning about a missing field initializer

[Eric Engestrom]

On Wednesday, 2017-05-10 13:48:00 +1000, Peter Hutterer wrote:
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>

This series is
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  src/evdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index d24a5646..a2be6fce 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -92,7 +92,7 @@ static const struct evdev_udev_tag_match 
> evdev_udev_tag_matches[] = {
>   {"ID_INPUT_SWITCH", EVDEV_UDEV_TAG_SWITCH},
>  
>   /* sentinel value */
> - { 0 },
> + {0, 0},

I'm sending a patch to change the code using this to use ARRAY_SIZE()
instead, based on top of this patch.

>  };
>  
>  static inline bool
> -- 
> 2.12.2
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] evdev: replace null sentinel with ARRAY_SIZE

[Eric Engestrom]

On Wednesday, 2017-05-10 17:25:04 +0200, Jan Engelhardt wrote:
> 
> On Wednesday 2017-05-10 17:20, Dima Ryazanov wrote:
> >  +#define ARRAY_SIZE(a) (sizeof(a)/sizeof(a)[0])
> >
> >
> >I'm guessing this works, but "sizeof(a)[0]" looks very unintuitive to me. I 
> >think "sizeof(a[0])" is
> >the convention?
> 
> If going with the sizeof(T) syntax, then the answer would 
> be "sizeof((a)[0])", or alternatively, "sizeof(*(a))".

The syntax [1] is `sizeof(T)` for a type, and `sizeof E` for an
expression, which the array variable passed in is.
Since it's in a macro, you have to put brackets around arguments,
hence the `(a)` in both cases.

I can add unnecessary brackets if you want, but that would be purely
cosmetic, no actual machine purpose ;)

[1] http://en.cppreference.com/w/c/language/sizeof
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 4/4] tools: hook libinput-debug-events into the libinput tool

[Eric Engestrom]

On Wednesday, 2017-05-17 09:02:32 +1000, Peter Hutterer wrote:
> Signed-off-by: Peter Hutterer 
> ---
>  meson.build   |  7 +++--
>  tools/Makefile.am |  8 --
>  tools/libinput-debug-events.c | 15 +--
>  tools/libinput-tool.c |  8 ++
>  tools/libinput-tool.h |  1 +
>  tools/libinput.1  | 60 
> ++-
>  tools/shared.c| 36 ++
>  tools/shared.h|  5 ++--
>  8 files changed, 121 insertions(+), 19 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 090536b5..1606401c 100644
> --- a/meson.build
> +++ b/meson.build
[snip]
> @@ -360,11 +362,12 @@ executable('libinput-list-devices',
>  libinput_list_devices_sources,
>  dependencies : [ dep_libinput ],
>  include_directories : include_directories('src'),
> -c_args : [ '-DTOOLS_BUILD_STANDALONE=1' ]
> +c_args : [ '-DTOOLS_BUILD_STANDALONE=1' ],
>  install : true,
>  )

This hunk is a fix for a bug introduced in patch #3; squash?
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] meson: restore the SELinux context for our .so file on install

[Eric Engestrom]

On 21 June 2017 00:43:10 BST, Peter Hutterer <peter.hutte...@who-t.net> wrote:
>Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
>---
>A temporary workaround until the meson bit is fixed.

Reviewed-by: Eric Engestrom <e...@engestrom.ch

>
> meson.build |  6 ++
> src/Makefile.am |  2 +-
> src/libinput-restore-selinux-context.sh | 10 ++
> 3 files changed, 17 insertions(+), 1 deletion(-)
> create mode 100644 src/libinput-restore-selinux-context.sh
>
>diff --git a/meson.build b/meson.build
>index 217bf82a..e77f7d13 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -220,6 +220,12 @@ pkgconfig.generate(
>   libraries: lib_libinput
> )
> 
>+# Restore the SELinux context for the libinput.so.a.b.c on install
>+# meson bug https://github.com/mesonbuild/meson/issues/1967
>+meson.add_install_script('src/libinput-restore-selinux-context.sh',
>+   get_option('libdir'),
>+   lib_libinput.full_path())
>+
>  documentation 
> 
> if get_option('documentation')
>diff --git a/src/Makefile.am b/src/Makefile.am
>index 08e6aa1e..6723d5ae 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -77,4 +77,4 @@ pkgconfig_DATA = libinput.pc
> AM_CFLAGS = $(GCC_CFLAGS)
> 
> DISTCLEANFILES = libinput-version.h
>-EXTRA_DIST = libinput-version.h.in libinput.sym
>+EXTRA_DIST = libinput-version.h.in libinput.sym
>libinput-restore-selinux-context.sh
>diff --git a/src/libinput-restore-selinux-context.sh
>b/src/libinput-restore-selinux-context.sh
>new file mode 100644
>index ..7fd8fad0
>--- /dev/null
>+++ b/src/libinput-restore-selinux-context.sh
>@@ -0,0 +1,10 @@
>+#!/bin/sh
>+#
>+# $1: abs path to libdir
>+# $2: abs path to .so file
>+
>+libdir="$1"
>+sofile=$(basename "$2")
>+
>+echo "Restoring SELinux context on
>$MESON_INSTALL_DESTDIR_PREFIX/$libdir/$sofile"
>+restorecon "$MESON_INSTALL_DESTDIR_PREFIX/$libdir/$sofile"
>-- 
>2.13.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 1/3] meson: rename 'enable-tests' option to just 'tests'

[Eric Engestrom]

On Tuesday, 2017-06-20 12:26:24 +1000, Peter Hutterer wrote:
> All the other config options have a simple true/false as well.
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>

All 3 patches are:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  meson.build   | 2 +-
>  meson_options.txt | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 9df35341..aba7f48c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -436,7 +436,7 @@ executable('ptraccel-debug',
>  
>   tests 
>  
> -if get_option('enable-tests')
> +if get_option('tests')
>   dep_check = dependency('check', version : '>= 0.9.10')
>   valgrind = find_program('valgrind')
>   addr2line = find_program('addr2line')
> diff --git a/meson_options.txt b/meson_options.txt
> index f432e7ea..ad3095e3 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -10,7 +10,7 @@ option('debug-gui',
> type: 'boolean',
> default: true,
> description: 'Enable the "debug-gui" feature in the libinput tool 
> [default=true]')
> -option('enable-tests',
> +option('tests',
> type: 'boolean',
> default: true,
> description: 'Build the tests [default=true]')
> -- 
> 2.13.0
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] doc: add instructions for handling SELinux denials

[Eric Engestrom]

On Tuesday, 2017-06-20 12:45:15 +1000, Peter Hutterer wrote:
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> ---
>  doc/building.dox | 27 +++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/doc/building.dox b/doc/building.dox
> index 5ce21463..25594da8 100644
> --- a/doc/building.dox
> +++ b/doc/building.dox
> @@ -102,6 +102,33 @@ overwriting manually installed files.
>  Arch: ```sudo packman -S libinput```
>  
>  
> +@subsection building_selinux SELinux adjustments
> +
> +On systems with SELinux, overwriting the distribution-provided package with
> +a manually built libinput may cause SELinux denials. This usually manifests
> +when gdm does not start because it is denied access to libinput. The journal
> +shows a log message in the form of:
> +
> +
> +May 25 15:28:42 localhost.localdomain audit[23268]: AVC avc:  denied  { 
> execute } for  pid=23268 comm="gnome-shell" 
> path="/usr/lib64/libinput.so.10.12.2" dev="dm-0" ino=1709093 
> scontext=system_u:system_r:xdm_t:s0-s0:c0.c1023 
> tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=0
> +May 25 15:28:42 localhost.localdomain org.gnome.Shell.desktop[23270]: 
> /usr/bin/gnome-shell: error while loading shared libraries: libinput.so.10: 
> failed to map segment from shared object
> +
> +
> +The summary of this error message is that gdm's gnome-shell runs in the
> +```system_u:system_r:xdm_t``` context but libinput is installed with the
> +context ```unconfined_u:object_r:user_home_t```.
> +
> +To avoid this issue, restore the SELinux context for any system files.
> +
> +
> +$> sudo restorecon /usr/lib/libinput.so.*
> +$> sudo restorecon /usr/lib64/libinput.so.*
> +
> +
> +Pick whichever one is your libdir.

You don't need this note if you give that command instead :)

$> sudo restorecon /usr/lib*/libinput.so.*

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> +
> +This issue is tracked in https://github.com/mesonbuild/meson/issues/1967.
> +
>  @subsection building_dependencies Build dependencies
>  
>  libinput has a few build-time dependencies that must be installed prior to
> -- 
> 2.13.0
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput] doc: update build instructions for Arch

[Eric Engestrom]

`abs` has been deprecated, and shut down last month. [1]
`asp` replaces it, so rewrite the instructions to use this instead.

Also, add `--noextract` to the makepkg command, as there is no point
downloading and extracting the sources since they're not going to be
built here.

[1] https://www.archlinux.org/news/deprecation-of-abs/

Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
---
 doc/building.dox | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/doc/building.dox b/doc/building.dox
index 5ce2146..355c535 100644
--- a/doc/building.dox
+++ b/doc/building.dox
@@ -124,10 +124,11 @@ $> sudo zypper modifyrepo --disable `zypper repos | grep 
source | awk '{print $5
 
 Arch:
 
-$> abs extra/libinput
+$> sudo pacman -S asp
 $> cd $(mktemp -d)
+$> asp export libinput
-$> cp /var/abs/extra/libinput/PKGBUILD .
+$> cd libinput
-$> makepkg --syncdeps --nobuild
+$> makepkg --syncdeps --nobuild --noextract
 
 
 
-- 
Cheers,
  Eric

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

Re: [PATCH] test: use unique names for all the test suite names

[Eric Engestrom]

On Wednesday, 2017-06-07 11:28:28 +1000, Peter Hutterer wrote:
> This makes it possible to run multiple test suite simultaneously on the same
> host without messing up the other runs (provided that all instances use
> the same udev/hwdb files). Previously, removing the udev rules/hwdb at the end
> of a test run would cause test case failures in other runs that hadn't
> completed yet.
> 
> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> ---
>  test/litest.c | 25 -
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/test/litest.c b/test/litest.c
> index ef474455..8991ed54 100644
> --- a/test/litest.c
> +++ b/test/litest.c
> @@ -55,13 +55,13 @@
>  #define UDEV_RULE_PREFIX "99-litest-"
>  #define UDEV_HWDB_D "/etc/udev/hwdb.d"
>  #define UDEV_MODEL_QUIRKS_RULE_FILE UDEV_RULES_D \
> - "/91-litest-model-quirks-REMOVEME.rules"
> + "/91-litest-model-quirks-REMOVEME-XX.rules"
>  #define UDEV_MODEL_QUIRKS_HWDB_FILE UDEV_HWDB_D \
> - "/91-litest-model-quirks-REMOVEME.hwdb"
> + "/91-litest-model-quirks-REMOVEME-XX.hwdb"
>  #define UDEV_TEST_DEVICE_RULE_FILE UDEV_RULES_D \
> - "/91-litest-test-device-REMOVEME.rules"
> + "/91-litest-test-device-REMOVEME-XXX.rules"
>  #define UDEV_DEVICE_GROUPS_FILE UDEV_RULES_D \
> - "/80-libinput-device-groups-litest.rules"
> + "/80-libinput-device-groups-litest-XX.rules"
>  
>  static int jobs = 8;
>  static int in_debugger = -1;
> @@ -1125,17 +1125,20 @@ litest_copy_file(const char *dest, const char *src, 
> const char *header)
>  {
>   int in, out, length;
>   struct created_file *file;
> + long int suffixlen;

My man says `int suffixlen` (without `long`), but it doesn't really
matter. Everything else looks good to me:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

>  
>   file = zalloc(sizeof(*file));
>   litest_assert(file);
>   file->path = strdup(dest);
>   litest_assert(file->path);
>  
> - out = open(dest, O_CREAT|O_WRONLY, 0644);
> + suffixlen = file->path + strlen(file->path)  - rindex(file->path, '.');
> + out = mkstemps(file->path, suffixlen);
>   if (out == -1)
>   litest_abort_msg("Failed to write to file %s (%s)\n",
> -  dest,
> +  file->path,
>strerror(errno));
> + litest_assert_int_ne(chmod(file->path, 0644), -1);
>  
>   if (header) {
>   length = strlen(header);
> @@ -1228,6 +1231,7 @@ static char *
>  litest_init_device_udev_rules(struct litest_test_device *dev)
>  {
>   int rc;
> + int fd;
>   FILE *f;
>   char *path = NULL;
>  
> @@ -1235,7 +1239,7 @@ litest_init_device_udev_rules(struct litest_test_device 
> *dev)
>   return NULL;
>  
>   rc = xasprintf(,
> -   "%s/%s%s.rules",
> +   "%s/%s%s-XX.rules",
> UDEV_RULES_D,
> UDEV_RULE_PREFIX,
> dev->shortname);
> @@ -1243,8 +1247,11 @@ litest_init_device_udev_rules(struct 
> litest_test_device *dev)
>(int)(
>  strlen(UDEV_RULES_D) +
>  strlen(UDEV_RULE_PREFIX) +
> -strlen(dev->shortname) + 7));
> - f = fopen(path, "w");
> +strlen(dev->shortname) + 14));
> +
> + fd = mkstemps(path, 6);
> + litest_assert_int_ne(fd, -1);
> + f = fdopen(fd, "w");
>   litest_assert_notnull(f);
>   litest_assert_int_ge(fputs(dev->udev_rule, f), 0);
>   fclose(f);
> -- 
> 2.13.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 1/2] config-parser: fix `short_name` type

[Eric Engestrom]

On Wednesday, 2017-05-24 21:23:14 +0100, Eric Engestrom wrote:
> This field is populated with chars, compared to chars and printed as
> a char. It should probably be a char.
> 
> Signed-off-by: Eric Engestrom <e...@engestrom.ch>

Humble ping?

I don't have commit access either, so you'll have to push this for me :)

Cheers,
  Eric

> ---
>  shared/config-parser.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/shared/config-parser.h b/shared/config-parser.h
> index f18d2c0b..af3f66a2 100644
> --- a/shared/config-parser.h
> +++ b/shared/config-parser.h
> @@ -64,7 +64,7 @@ enum weston_option_type {
>  struct weston_option {
>   enum weston_option_type type;
>   const char *name;
> - int short_name;
> + char short_name;
>   void *data;
>  };
>  
> -- 
> Cheers,
>   Eric
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RESEND] [PATCH] weston: Add support for "--foo bar" style options

[Eric Engestrom]

On Friday, 2017-05-05 14:20:33 -0400, Lyude wrote:
> A little earlier today I ended up spending a lot of time trying to
> figure out why weston wasn't managing to launch over SSH and telling me
> that I did not have a --tty option specified, despite me passing the
> option strings ["--tty", "3"]. Turns out weston just doesn't support
> that.
> 
> So, add support for this kind of format in addition to "--foo=bar" to
> save others from making the same mistake I did.
> 
> Signed-off-by: Lyude <ly...@redhat.com>
> ---
> Resending since I realized I was no longer subscribed to this mailing list
> after the first time I sent this, whoops.
> 
>  shared/option-parser.c | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/shared/option-parser.c b/shared/option-parser.c
> index eee7546..82f2f31 100644
> --- a/shared/option-parser.c
> +++ b/shared/option-parser.c
> @@ -87,6 +87,28 @@ long_option(const struct weston_option *options, int 
> count, char *arg)
>  }
>  
>  static int
> +long_option_with_arg(const struct weston_option *options, int count, char 
> *arg, char *param)
> +{
> + int k, len;
> +
> + for (k = 0; k < count; k++) {
> + if (!options[k].name)
> + continue;
> +
> + len = strlen(options[k].name);
> + if (strncmp(options[k].name, arg + 2, len) != 0)
> + continue;
> +
> + if (options[k].type == WESTON_OPTION_BOOLEAN)
> + continue;

Am I understanding this right, that you're skipping this here because it
is already be handled by long_option()?
I think this is actually unreachable, in which case an assert might
better document this, or you can add a small comment.

With either of these, the patch is
Reviewed-by: Eric Engestrom <e...@engestrom.ch>

Thanks!

> +
> + return handle_option(options + k, param);
> + }
> +
> + return 0;
> +}
> +
> +static int
>  short_option(const struct weston_option *options, int count, char *arg)
>  {
>   int k;
> @@ -148,6 +170,12 @@ parse_options(const struct weston_option *options,
>   if (long_option(options, count, argv[i]))
>   continue;
>  
> + /* ...also handle --foo bar */
> + if (i + 1 < *argc &&
> + long_option_with_arg(options, count, 
> argv[i], argv[i+1])) {
> + i++;
> + continue;
> + }
>   } else {
>   /* Short option, e.g -f or -f42 */
>   if (short_option(options, count, argv[i]))
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v2 libinput 3/5] test: allow running the symbols-leak-test.in script directly

[Eric Engestrom]

Reviewed-by: Eric Engestrom <e...@engestrom.ch>

On 2 May 2017 01:01:32 BST, Peter Hutterer <peter.hutte...@who-t.net> wrote:
>With autotools, we replace the @top_srcdir@ during configure and then
>run
>the resulting scripts.
>
>With meson, it's easier to just pass top-srcdir it in as argument.
>
>Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
>---
>Changes to v1:
>- use a case statement to check the first character for @ to check
>whether
>  it was replaced
>
> test/symbols-leak-test.in | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/test/symbols-leak-test.in b/test/symbols-leak-test.in
>index 448ef2f..86792dc 100755
>--- a/test/symbols-leak-test.in
>+++ b/test/symbols-leak-test.in
>@@ -2,6 +2,17 @@
> 
> ### simple check for exported symbols
> 
>+# Allow running this after the configure.ac replacement or directly
>with an
>+# argument for the top_srcdir
>+TOP_SRCDIR="@top_srcdir@"
>+case "$TOP_SRCDIR" in
>+@*)
>+  TOP_SRCDIR="$1"
>+  ;;
>+*)
>+  ;;

I think that case is unnecessary, but it doesn't hurt.

>+esac
>+
> # make bash exit if any command will exit with non-0 return value
> set -e
> 
>@@ -9,9 +20,9 @@ set -e
> cd `dirname $0`
> 
> diff -a -u \
>-  <(cat @top_srcdir@/src/libinput.sym | \
>+  <(cat "$TOP_SRCDIR"/src/libinput.sym | \
>   grep '^\s\+libinput_.*' | \
>   sed -e 's/^\s\+\(.*\);/\1/' |  sort) \
>-  <(cat @top_srcdir@/src/*.c | \
>+  <(cat "$TOP_SRCDIR"/src/*.c | \
>   grep LIBINPUT_EXPORT -A 1 | grep '^libinput_.*' | \
>   sed -e 's/(.*//' | sort)
>-- 
>2.9.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

[PATCH weston 1/2] config-parser: fix `short_name` type

[Eric Engestrom]

This field is populated with chars, compared to chars and printed as
a char. It should probably be a char.

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 shared/config-parser.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/shared/config-parser.h b/shared/config-parser.h
index f18d2c0b..af3f66a2 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -64,7 +64,7 @@ enum weston_option_type {
 struct weston_option {
enum weston_option_type type;
const char *name;
-   int short_name;
+   char short_name;
void *data;
 };
 
-- 
Cheers,
  Eric

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

[PATCH weston 2/2] option-parser: replace int/0/1 with bool/false/true

[Eric Engestrom]

These are already used as bools by all callers, let's make that official.

Signed-off-by: Eric Engestrom <e...@engestrom.ch>
---
 shared/option-parser.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/shared/option-parser.c b/shared/option-parser.c
index e8d9b3b7..0f93464c 100644
--- a/shared/option-parser.c
+++ b/shared/option-parser.c
@@ -25,6 +25,7 @@
 
 #include "config.h"
 
+#include 
 #include 
 #include 
 #include 
@@ -35,7 +36,7 @@
 #include "config-parser.h"
 #include "string-helpers.h"
 
-static int
+static bool
 handle_option(const struct weston_option *option, char *value)
 {
char* p;
@@ -43,23 +44,23 @@ handle_option(const struct weston_option *option, char 
*value)
switch (option->type) {
case WESTON_OPTION_INTEGER:
if (!safe_strtoint(value, option->data))
-   return 0;
-   return 1;
+   return false;
+   return true;
case WESTON_OPTION_UNSIGNED_INTEGER:
errno = 0;
* (uint32_t *) option->data = strtoul(value, , 10);
if (errno != 0 || p == value || *p != '\0')
-   return 0;
-   return 1;
+   return false;
+   return true;
case WESTON_OPTION_STRING:
* (char **) option->data = strdup(value);
-   return 1;
+   return true;
default:
assert(0);
}
 }
 
-static int
+static bool
 long_option(const struct weston_option *options, int count, char *arg)
 {
int k, len;
@@ -76,17 +77,17 @@ long_option(const struct weston_option *options, int count, 
char *arg)
if (!arg[len + 2]) {
* (int32_t *) options[k].data = 1;
 
-   return 1;
+   return true;
}
} else if (arg[len+2] == '=') {
return handle_option(options + k, arg + len + 3);
}
}
 
-   return 0;
+   return false;
 }
 
-static int
+static bool
 long_option_with_arg(const struct weston_option *options, int count, char *arg,
 char *param)
 {
@@ -108,16 +109,16 @@ long_option_with_arg(const struct weston_option *options, 
int count, char *arg,
return handle_option(options + k, param);
}
 
-   return 0;
+   return false;
 }
 
-static int
+static bool
 short_option(const struct weston_option *options, int count, char *arg)
 {
int k;
 
if (!arg[1])
-   return 0;
+   return false;
 
for (k = 0; k < count; k++) {
if (options[k].short_name != arg[1])
@@ -127,25 +128,25 @@ short_option(const struct weston_option *options, int 
count, char *arg)
if (!arg[2]) {
* (int32_t *) options[k].data = 1;
 
-   return 1;
+   return true;
}
} else if (arg[2]) {
return handle_option(options + k, arg + 2);
} else {
-   return 0;
+   return false;
}
}
 
-   return 0;
+   return false;
 }
 
-static int
+static bool
 short_option_with_arg(const struct weston_option *options, int count, char 
*arg, char *param)
 {
int k;
 
if (!arg[1])
-   return 0;
+   return false;
 
for (k = 0; k < count; k++) {
if (options[k].short_name != arg[1])
@@ -157,7 +158,7 @@ short_option_with_arg(const struct weston_option *options, 
int count, char *arg,
return handle_option(options + k, param);
}
 
-   return 0;
+   return false;
 }
 
 int
-- 
Cheers,
  Eric

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

Re: [PATCH] Add m1bpp (monochrome, 1 bit/pixel) pixel format

[Eric Engestrom]

On Friday, 2017-09-15 11:17:29 -0400, Drew DeVault wrote:
> On 2017-09-15  6:06 PM, Pekka Paalanen wrote:
> > Indeed, 2 and 4 bit formats are also missing, but 8 and 16 bit ones are
> > already defined unless you meant some new variation.
> 
> The 8 and 16 bit ones, so far as I can tell, are 8/16 bits per component
> of an RGB(A) triple (quadruple), rather than 8/16 bits per pixel.

You're thinking about 24/32 bpp formats there, such as DRM_FORMAT_RGB888
or DRM_FORMAT_ARGB, but there are smaller ones. Since you only care
about one component, you could also use DRM_FORMAT_R8 [1] or
DRM_FORMAT_R16, which are 8 and 16 bits per pixel.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n41
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] clients/nested: fix boolean test

[Eric Engestrom]

weston_check_egl_extension() returns a bool, not a pointer.

Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
---
 clients/nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clients/nested.c b/clients/nested.c
index 173076a6..e9070e9b 100644
--- a/clients/nested.c
+++ b/clients/nested.c
@@ -748,7 +748,7 @@ nested_init_compositor(struct nested *nested)
 
nested->egl_display = display_get_egl_display(nested->display);
extensions = eglQueryString(nested->egl_display, EGL_EXTENSIONS);
-   if (weston_check_egl_extension(extensions, 
"EGL_WL_bind_wayland_display") == NULL) {
+   if (!weston_check_egl_extension(extensions, 
"EGL_WL_bind_wayland_display")) {
fprintf(stderr, "no EGL_WL_bind_wayland_display extension\n");
return -1;
}
-- 
Cheers,
  Eric

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

Re: [PATCH weston] clients/nested: fix boolean test

[Eric Engestrom]

On Monday, 2017-09-11 16:08:53 +0100, Emil Velikov wrote:
> On 11 September 2017 at 15:40, Eric Engestrom <eric.engest...@imgtec.com> 
> wrote:
> > On Monday, 2017-09-11 13:52:28 +0100, Eric Engestrom wrote:
> >> weston_check_egl_extension() returns a bool, not a pointer.
> >>
> Nicely spotted. Fortunately, code produced should be identical ;-)
> 
> >> Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
> >
> > Fixes: ce5b614c80b4dfe8e899 "clients/nested: use weston_check_egl_extension
> >  over strstr"
> > Cc: Emil Velikov <emil.veli...@collabora.com>
> 
> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

Thanks!
I don't have commit access to wayland (and I just checked, neither do
you); can someone push this for me?

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

Re: [PATCH weston] clients/nested: fix boolean test

[Eric Engestrom]

On Monday, 2017-09-11 13:52:28 +0100, Eric Engestrom wrote:
> weston_check_egl_extension() returns a bool, not a pointer.
> 
> Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>

Fixes: ce5b614c80b4dfe8e899 "clients/nested: use weston_check_egl_extension
 over strstr"
Cc: Emil Velikov <emil.veli...@collabora.com>

> ---
>  clients/nested.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/clients/nested.c b/clients/nested.c
> index 173076a6..e9070e9b 100644
> --- a/clients/nested.c
> +++ b/clients/nested.c
> @@ -748,7 +748,7 @@ nested_init_compositor(struct nested *nested)
>  
>   nested->egl_display = display_get_egl_display(nested->display);
>   extensions = eglQueryString(nested->egl_display, EGL_EXTENSIONS);
> - if (weston_check_egl_extension(extensions, 
> "EGL_WL_bind_wayland_display") == NULL) {
> + if (!weston_check_egl_extension(extensions, 
> "EGL_WL_bind_wayland_display")) {
>   fprintf(stderr, "no EGL_WL_bind_wayland_display extension\n");
>   return -1;
>   }
> -- 
> Cheers,
>   Eric
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] fallback: send key events out immediately upon receiving them

[Eric Engestrom]

On Friday, 2017-12-08 10:17:17 +1000, Peter Hutterer wrote:
> Commit db3b6fe5f7f8 "fallback: change to handle the state at EV_SYN time"
> introduced regressions for two types of event sequences.
> 
> One is a kernel bug - some devices/drivers like the asus-wireless send a key
> press + release within the same event frame which now cancels out and
> disappears into the ether. This should be fixed in the kernel drivers but
> there appear to be enough of them that we can't just pretend it's an outlier.
> 
> The second issue is a libinput bug. If we get two key events in the same frame
> (e.g. shift + A) we update the state correctly but the events are sent in the
> order of the event codes. KEY_A sorts before KEY_LEFTSHIFT and our shift + A
> becomes A + shift.
> 
> Fix this by treating key events as before db3b6fe5f7f8 - by sending them out
> as we get them.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=104030
> 
> Signed-off-by: Peter Hutterer 
> ---
>  src/evdev-fallback.c | 35 
>  test/test-keyboard.c | 56 
> 
>  2 files changed, 73 insertions(+), 18 deletions(-)
> 
> diff --git a/src/evdev-fallback.c b/src/evdev-fallback.c
> index 52a3bde1..0cd2497e 100644
> --- a/src/evdev-fallback.c
> +++ b/src/evdev-fallback.c
> @@ -501,6 +501,22 @@ fallback_process_key(struct fallback_dispatch *dispatch,
>   }
>  
>   hw_set_key_down(dispatch, e->code, e->value);
> +
> + switch (type) {
> + case KEY_TYPE_NONE:
> + break;
> + case KEY_TYPE_KEY:
> + fallback_keyboard_notify_key(
> +  dispatch,
> +  device,
> +  time,
> +  e->code,
> +  e->value ? LIBINPUT_KEY_STATE_PRESSED :
> + LIBINPUT_KEY_STATE_RELEASED);
> + break;
> + case KEY_TYPE_BUTTON:
> + break;
> + }
>  }
>  
>  static void
> @@ -834,27 +850,10 @@ fallback_handle_state(struct fallback_dispatch 
> *dispatch,
>   if (dispatch->pending_event & EVDEV_KEY) {
>   bool want_debounce = false;
>   for (unsigned int code = 0; code <= KEY_MAX; code++) {
> - bool new_state;
> -
>   if (!hw_key_has_changed(dispatch, code))
>   continue;
>  
> - new_state = hw_is_key_down(dispatch, code);
> -
> - switch (get_key_type(code)) {
> - case KEY_TYPE_NONE:
> - break;
> - case KEY_TYPE_KEY:
> - fallback_keyboard_notify_key(
> -  dispatch,
> -  device,
> -  time,
> -  code,
> -  new_state ?
> -  
> LIBINPUT_KEY_STATE_PRESSED :
> -  
> LIBINPUT_KEY_STATE_RELEASED);
> - break;
> - case KEY_TYPE_BUTTON:
> + if (get_key_type(code) == KEY_TYPE_BUTTON) {
>   want_debounce = true;
>   break;

This will break out of the `for()` now :)

>   }
> diff --git a/test/test-keyboard.c b/test/test-keyboard.c
> index dc2e630e..db836381 100644
> --- a/test/test-keyboard.c
> +++ b/test/test-keyboard.c
> @@ -365,6 +365,61 @@ START_TEST(keyboard_no_buttons)
>  }
>  END_TEST
>  
> +START_TEST(keyboard_frame_order)
> +{
> + struct litest_device *dev = litest_current_device();
> + struct libinput *li = dev->libinput;
> +
> + if (!libevdev_has_event_code(dev->evdev, EV_KEY, KEY_A) ||
> + !libevdev_has_event_code(dev->evdev, EV_KEY, KEY_LEFTSHIFT))
> + return;
> +
> + litest_drain_events(li);
> +
> + litest_event(dev, EV_KEY, KEY_LEFTSHIFT, 1);
> + litest_event(dev, EV_KEY, KEY_A, 1);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> + libinput_dispatch(li);
> +
> + litest_assert_key_event(li,
> + KEY_LEFTSHIFT,
> + LIBINPUT_KEY_STATE_PRESSED);
> + litest_assert_key_event(li, KEY_A, LIBINPUT_KEY_STATE_PRESSED);
> +
> + litest_event(dev, EV_KEY, KEY_LEFTSHIFT, 0);
> + litest_event(dev, EV_KEY, KEY_A, 0);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> + libinput_dispatch(li);
> +
> + litest_assert_key_event(li,
> + KEY_LEFTSHIFT,
> + LIBINPUT_KEY_STATE_RELEASED);
> + litest_assert_key_event(li, KEY_A, LIBINPUT_KEY_STATE_RELEASED);
> +
> + litest_event(dev, 

Re: [PATCH weston 1/2] config-parser: fix `short_name` type

[Eric Engestrom]

On Tuesday, 2017-12-05 13:47:06 +, Daniel Stone wrote:
> Hi Eric,
> 
> On 8 June 2017 at 22:20, Eric Engestrom <eric.engest...@imgtec.com> wrote:
> > On Wednesday, 2017-05-24 21:23:14 +0100, Eric Engestrom wrote:
> >> This field is populated with chars, compared to chars and printed as
> >> a char. It should probably be a char.
> >>
> >> Signed-off-by: Eric Engestrom <e...@engestrom.ch>
> >
> > Humble ping?
> >
> > I don't have commit access either, so you'll have to push this for me :)
> 
> Sorry for the hideous delay. Reviewed and pushed both now.

Haha, no worries.
To be honest, I had forgotten myself ¯\_(ツ)_/¯

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

Re: [PATCH wayland 4/6] wayland-egl: fail the symbol check if lib is missing

[Eric Engestrom]

On February 15, 2018 6:55:14 PM UTC, Emil Velikov <emil.l.veli...@gmail.com> 
wrote:
> From: Emil Velikov <emil.veli...@collabora.com>
> 
> Based on a similar patch (in Mesa) by Eric Engestrom.
> 
> Cc: Eric Engestrom <eric.engest...@imgtec.com>

Reviewed-by: Eric Engestrom <e...@engestrom.ch>

> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> ---
>  egl/wayland-egl-symbols-check | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/egl/wayland-egl-symbols-check
> b/egl/wayland-egl-symbols-check
> index e7105ea..93a3361 100755
> --- a/egl/wayland-egl-symbols-check
> +++ b/egl/wayland-egl-symbols-check
> @@ -1,6 +1,14 @@
>  #!/bin/sh
> +set -eu
>  
> -FUNCS=$(nm -D --defined-only ${1-.libs/libwayland-egl.so} | grep -o
> "T .*" | cut -c 3- | while read func; do
> +LIB=${1-.libs/libwayland-egl.so}
> +
> +if [ ! -f "$LIB" ]; then
> + echo "The test binary \"$LIB\" does no exist"
> + exit 1
> +fi
> +
> +FUNCS=$(nm -D --defined-only $LIB | grep -o "T .*" | cut -c 3- |
> while read func; do
>  ( grep -q "^$func$" || echo $func )  <  wl_egl_window_resize
>  wl_egl_window_create
> -- 
> 2.16.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 wayland] wayland-egl: use correct `nm` path when cross-compiling

[Eric Engestrom]

On Friday, 2018-02-23 17:31:53 +, Emil Velikov wrote:
> From: Emil Velikov <emil.veli...@collabora.com>
> 
> Inspired by Heiko Becker and Eric's work in libdrm and Mesa
> respectively.
> 
> Cc: Eric Engestrom <eric.engest...@imgtec.com>
> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>

Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

I haven't really followed that, but I think the meson stuff is in
a branch somewhere? I guess it should get the other half of this patch
so that it doesn't need fixing when it lands :)

> ---
>  configure.ac  | 1 +
>  egl/wayland-egl-symbols-check | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 2542243..91f837d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -29,6 +29,7 @@ AC_PROG_CC
>  AC_PROG_CXX
>  AC_PROG_GREP
>  AM_PROG_AS
> +AC_PROG_NM
>  
>  # check if we have C++ compiler. This is hacky workaround,
>  # for a reason why it is this way see
> diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
> index e107362..6ad28f3 100755
> --- a/egl/wayland-egl-symbols-check
> +++ b/egl/wayland-egl-symbols-check
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -FUNCS=$(nm -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | cut -c 3- 
> | while read func; do
> +FUNCS=$($NM -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | cut -c 
> 3- | while read func; do
>  ( grep -q "^$func$" || echo $func )  <  wl_egl_window_resize
>  wl_egl_window_create
> -- 
> 2.16.0
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland] TODO: remove "SDL port", it's been done by now

[Eric Engestrom]

Upstream SDL supports Wayland since v2.0.4 (June 2015):
https://forums.libsdl.org/viewtopic.php?t=11294

Just set SDL_VIDEODRIVER=wayland and SDL will do the right thing :)

Signed-off-by: Eric Engestrom 
---
 TODO | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/TODO b/TODO
index 8cb8d346ac73fa83a643..88fa5cc92b8a0be2a609 100644
--- a/TODO
+++ b/TODO
@@ -102,9 +102,6 @@ Clients and ports
 
  - Investigate DirectFB on Wayland (or is that Wayland on DirectFB?)
 
- - SDL port, bnf has work in progress here:
-   http://cgit.freedesktop.org/~bnf/sdl-wayland/
-
 
 Ideas
 
-- 
Cheers,
  Eric

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

Re: [PATCH wayland v3 1/3] wayland-egl: fail the symbol check if lib is missing

[Eric Engestrom]

On Monday, 2018-03-19 11:43:08 +, Emil Velikov wrote:
> On 19 March 2018 at 09:56, Pekka Paalanen <ppaala...@gmail.com> wrote:
> > On Fri, 16 Mar 2018 16:18:57 +
> > Emil Velikov <emil.l.veli...@gmail.com> wrote:
> >
> >> On 16 March 2018 at 13:52, Pekka Paalanen <ppaala...@gmail.com> wrote:
> >> > On Thu, 15 Mar 2018 14:30:27 +
> >> > Emil Velikov <emil.l.veli...@gmail.com> wrote:
> >> >
> >> >> From: Emil Velikov <emil.veli...@collabora.com>
> >> >>
> >> >> Based on a similar patch (in Mesa) by Eric Engestrom.
> >> >>
> >> >> v2: Rebase on top of $NM patch
> >> >> v3: Rebase
> >> >>
> >> >> Reviewed-by: Eric Engestrom <e...@engestrom.ch> (v1)
> >> >> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> >> >> ---
> >> >>  egl/wayland-egl-symbols-check | 10 +-
> >> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/egl/wayland-egl-symbols-check 
> >> >> b/egl/wayland-egl-symbols-check
> >> >> index 6ad28f3..8b3d711 100755
> >> >> --- a/egl/wayland-egl-symbols-check
> >> >> +++ b/egl/wayland-egl-symbols-check
> >> >> @@ -1,6 +1,14 @@
> >> >>  #!/bin/sh
> >> >> +set -eu
> >> >>
> >> >> -FUNCS=$($NM -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | 
> >> >> cut -c 3- | while read func; do
> >> >> +LIB=${WAYLAND_EGL_LIB}
> >> >> +
> >> >> +if [ ! -f "$LIB" ]; then
> >> >> + echo "The test binary \"$LIB\" does no exist"
> >> >> + exit 1
> >> >> +fi
> >> >> +
> >> >> +FUNCS=$($NM -D --defined-only $LIB | grep -o "T .*" | cut -c 3- | 
> >> >> while read func; do
> >> >>  ( grep -q "^$func$" || echo $func )  < >> >>  wl_egl_window_resize
> >> >>  wl_egl_window_create
> >> >
> >> > Hi Emil,
> >> >
> >> > this patch makes distcheck fail with:
> >> >
> >> > The test binary "./egl/.libs/libwayland-egl.so" does no exist
> >> > FAIL egl/wayland-egl-symbols-check (exit status: 1)
> >> >
> >> Right - I was aiming to remove the bonkers envvar and forgot about
> >> this preexisting bug.
> >> Patch (to be applied before the series) coming in a second.
> >
> > Hi Emil,
> >
> > because this set of four patches no longer regresses, and I think they
> > very much need to be in the release, I have pushed them:
> >f34af17..5f5945b  master -> master
> >
> > However, I did a quick test: I renamed wl_egl_window_resize() into
> > wl_egl_window_resize_uu() to try and trigger several kinds of errors:
> > symbol not present, extra symbol. The test suite still passes with
> > success.
> >
> > In egl/wayland-egl-symbols-check.log I have:
> >
> > ./egl/wayland-egl-symbols-check: line 11: NM: unbound variable
> >
> Seems like a 'fun' side-effect of the $NM patch. The series predates
> the patch by a week.
> First suggestion that comes to mind - use $(NM-nm}

yeah that would work, but I'm confused: what's the setup, why is NM
undefined? Both autotools and meson should be defining NM.

> 
> Eric any suggestions?
> 
> Thanks
> Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland v3 1/3] wayland-egl: fail the symbol check if lib is missing

[Eric Engestrom]

On Monday, 2018-03-19 12:11:50 +, Eric Engestrom wrote:
> On Monday, 2018-03-19 11:43:08 +, Emil Velikov wrote:
> > On 19 March 2018 at 09:56, Pekka Paalanen <ppaala...@gmail.com> wrote:
> > > On Fri, 16 Mar 2018 16:18:57 +
> > > Emil Velikov <emil.l.veli...@gmail.com> wrote:
> > >
> > >> On 16 March 2018 at 13:52, Pekka Paalanen <ppaala...@gmail.com> wrote:
> > >> > On Thu, 15 Mar 2018 14:30:27 +
> > >> > Emil Velikov <emil.l.veli...@gmail.com> wrote:
> > >> >
> > >> >> From: Emil Velikov <emil.veli...@collabora.com>
> > >> >>
> > >> >> Based on a similar patch (in Mesa) by Eric Engestrom.
> > >> >>
> > >> >> v2: Rebase on top of $NM patch
> > >> >> v3: Rebase
> > >> >>
> > >> >> Reviewed-by: Eric Engestrom <e...@engestrom.ch> (v1)
> > >> >> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> > >> >> ---
> > >> >>  egl/wayland-egl-symbols-check | 10 +-
> > >> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> > >> >>
> > >> >> diff --git a/egl/wayland-egl-symbols-check 
> > >> >> b/egl/wayland-egl-symbols-check
> > >> >> index 6ad28f3..8b3d711 100755
> > >> >> --- a/egl/wayland-egl-symbols-check
> > >> >> +++ b/egl/wayland-egl-symbols-check
> > >> >> @@ -1,6 +1,14 @@
> > >> >>  #!/bin/sh
> > >> >> +set -eu
> > >> >>
> > >> >> -FUNCS=$($NM -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | 
> > >> >> cut -c 3- | while read func; do
> > >> >> +LIB=${WAYLAND_EGL_LIB}
> > >> >> +
> > >> >> +if [ ! -f "$LIB" ]; then
> > >> >> + echo "The test binary \"$LIB\" does no exist"
> > >> >> + exit 1
> > >> >> +fi
> > >> >> +
> > >> >> +FUNCS=$($NM -D --defined-only $LIB | grep -o "T .*" | cut -c 3- | 
> > >> >> while read func; do
> > >> >>  ( grep -q "^$func$" || echo $func )  < > >> >>  wl_egl_window_resize
> > >> >>  wl_egl_window_create
> > >> >
> > >> > Hi Emil,
> > >> >
> > >> > this patch makes distcheck fail with:
> > >> >
> > >> > The test binary "./egl/.libs/libwayland-egl.so" does no exist
> > >> > FAIL egl/wayland-egl-symbols-check (exit status: 1)
> > >> >
> > >> Right - I was aiming to remove the bonkers envvar and forgot about
> > >> this preexisting bug.
> > >> Patch (to be applied before the series) coming in a second.
> > >
> > > Hi Emil,
> > >
> > > because this set of four patches no longer regresses, and I think they
> > > very much need to be in the release, I have pushed them:
> > >f34af17..5f5945b  master -> master
> > >
> > > However, I did a quick test: I renamed wl_egl_window_resize() into
> > > wl_egl_window_resize_uu() to try and trigger several kinds of errors:
> > > symbol not present, extra symbol. The test suite still passes with
> > > success.
> > >
> > > In egl/wayland-egl-symbols-check.log I have:
> > >
> > > ./egl/wayland-egl-symbols-check: line 11: NM: unbound variable
> > >
> > Seems like a 'fun' side-effect of the $NM patch. The series predates
> > the patch by a week.
> > First suggestion that comes to mind - use $(NM-nm}
> 
> yeah that would work, but I'm confused: what's the setup, why is NM
> undefined? Both autotools and meson should be defining NM.

Oh hold on, just as I sent the email I noticed this isn't Mesa, this is
Wayland...

I haven't followed wayland dev for a few months (maybe a year if I'm
honest), I've heard there was a move to meson but I think it hasn't
landed in master?

I guess my question remains though: what's the setup, ie how is the
check script being called? autotools? meson? other?

> 
> > 
> > Eric any suggestions?
> > 
> > Thanks
> > Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v4 2/4] simple-dmabuf-drm: use large enough buffer (freedreno)

[Eric Engestrom]

On Monday, 2018-03-19 17:45:19 +0100, Guido Günther wrote:
> Use stride instead of width for buffer calculation.
> 
> Signed-off-by: Guido Günther 
> ---
>  clients/simple-dmabuf-drm.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
> index efe3b7f5..a184d7e7 100644
> --- a/clients/simple-dmabuf-drm.c
> +++ b/clients/simple-dmabuf-drm.c
> @@ -222,13 +222,15 @@ fd_alloc_bo(struct buffer *buf)
>  {
>   int flags = DRM_FREEDRENO_GEM_CACHE_WCOMBINE;
>   int size = buf->width * buf->height * buf->bpp / 8;

You forgot to remove the initialisation here ^

> - buf->fd_dev = fd_device_new(buf->drm_fd);
>  
> + buf->fd_dev = fd_device_new(buf->drm_fd);
> + buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;
> + size = buf->stride * buf->height;
> + buf->fd_dev = fd_device_new(buf->drm_fd);
>   buf->fd_bo = fd_bo_new(buf->fd_dev, size, flags);
>  
>   if (!buf->fd_bo)
>   return 0;
> - buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;
>   return 1;
>  }
>  
> -- 
> 2.16.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 wayland] wayland-egl: Make symbol test fail on failure

[Eric Engestrom]

On Monday, 2018-03-19 15:13:14 +, Daniel Stone wrote:
> The previous rewrite of the wayland-egl ABI checker introduced checks
> for removed symbols as well as added symbols, but broke some failure
> conditions. Add an explict return-code variable set in failure paths,
> rather than chaining or conditions.
> 
> If we cannot find the binary or nm, we regard this as an error
> condition, rather than test failure.
> 
> v2: Don't test if we can execute $NM.
> 
> Signed-off-by: Daniel Stone 
> Reported-by: Pekka Paalanen 
> Fixes: 21b1f22eb056 ("wayland-egl: enhance the symbol test")
> Cc: Emil Velikov 
> ---
>  egl/wayland-egl-symbols-check | 27 +--
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
> index c47026b2..70fe1f4c 100755
> --- a/egl/wayland-egl-symbols-check
> +++ b/egl/wayland-egl-symbols-check
> @@ -1,11 +1,17 @@
>  #!/bin/sh
>  set -eu
>  
> +RET=0
>  LIB=${WAYLAND_EGL_LIB}
>  
> -if [ ! -f "$LIB" ]; then
> - echo "The test binary \"$LIB\" does no exist"
> - exit 1
> +if ! test -f "$LIB"; then
> + echo "Test binary \"$LIB\" does not exist"
> + exit 99
> +fi
> +
> +if ! test -n "$NM"; then
> + echo "nm environment variable not set"
> + exit 99

99? Were you looking for the "skip this test" 77?

>  fi
>  
>  AVAIL_FUNCS="$($NM -D --format=bsd --defined-only $LIB | awk '{print $3}')"
> @@ -32,7 +38,11 @@ NEW_ABI=$(echo "$AVAIL_FUNCS" | while read func; do
>  echo $func
>  done)
>  
> -test ! -n "$NEW_ABI" || echo "New ABI detected - If intentional, update the 
> test."; echo "$NEW_ABI"
> +if test -n "$NEW_ABI"; then
> + echo "New ABI detected - If intentional, update the test."
> + echo "$NEW_ABI"
> + RET=1
> +fi
>  
>  REMOVED_ABI=$(echo "$REQ_FUNCS" | while read func; do
>  echo "$AVAIL_FUNCS" | grep -q "^$func$" && continue
> @@ -40,5 +50,10 @@ REMOVED_ABI=$(echo "$REQ_FUNCS" | while read func; do
>  echo $func
>  done)
>  
> -test ! -n "$REMOVED_ABI" || echo "ABI break detected - Required symbol(s) no 
> longer exported!"; echo "$REMOVED_ABI"
> -test ! -n "$NEW_ABI" || test ! -n "$REMOVED_ABI"
> +if test -n "$REMOVED_ABI"; then
> + echo "ABI break detected - Required symbol(s) no longer exported!"
> + echo "$REMOVED_ABI"
> + RET=1
> +fi
> +
> +exit $RET
> -- 
> 2.16.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] wayland-egl: Make symbol test fail on failure

[Eric Engestrom]

On Monday, 2018-03-19 16:10:57 +, Daniel Stone wrote:
> Hi,
> 
> On 19 March 2018 at 16:08, Eric Engestrom <eric.engest...@imgtec.com> wrote:
> > On Monday, 2018-03-19 15:13:14 +, Daniel Stone wrote:
> >> +if ! test -f "$LIB"; then
> >> + echo "Test binary \"$LIB\" does not exist"
> >> + exit 99
> >> +fi
> >> +
> >> +if ! test -n "$NM"; then
> >> + echo "nm environment variable not set"
> >> + exit 99
> >
> > 99? Were you looking for the "skip this test" 77?
> 
> I did mean 99 'some kind of inexplicable internal error happened'
> rather than 77 skip, but I have no strong opinion on it and am happy
> to change to whatever is suggested.

I guess "don't have the tools to test this, skipping" would be fine, but
I'm not really involved in the wayland project so my opinion isn't the
one that matters the most :P

"I have no strong feelings one way or the other"

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

Re: [PATCH weston v4 3/4] simple-dmabuf-drm: 0 is a valid fd (freedreno)

[Eric Engestrom]

On Monday, 2018-03-19 17:45:20 +0100, Guido Günther wrote:
> Signed-off-by: Guido Günther <a...@sigxcpu.org>

It is indeed :)
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

> ---
>  clients/simple-dmabuf-drm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
> index a184d7e7..22891639 100644
> --- a/clients/simple-dmabuf-drm.c
> +++ b/clients/simple-dmabuf-drm.c
> @@ -244,10 +244,10 @@ static int
>  fd_bo_export_to_prime(struct buffer *buf)
>  {
>   buf->dmabuf_fd = fd_bo_dmabuf(buf->fd_bo);
> - if (buf->dmabuf_fd > 0)
> - return 0;
> + if (buf->dmabuf_fd < 0)
> + return 1;
>  
> - return 1;
> + return 0;

Nit: could be simplified as `return buf->dmabuf_fd < 0;`

>  }
>  
>  static int
> -- 
> 2.16.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: [RFC] Allow fd.o to join forces with X.Org

[Eric Engestrom]

On October 15, 2018 2:50:13 PM UTC, Harry Wentland  
wrote:
> The leadership of freedesktop.org (fd.o) has recently expressed
> interest
> in having an elected governing body. Given the tight connection
> between
> fd.o and X.Org and the fact that X.Org has such a governing body it
> seemed obvious to consider extending X.Org's mandate to fd.o.
> 
> Quite a bit of background on fd.o leading up to this has been covered
> by
> Daniel Stone at XDC 2018 and was covered really well by Jake Edge of
> LWN [1].

If you'd like to watch Daniel's presentation, the recording is available on 
YouTube:
https://youtu.be/s22B3E7rUTs

The slides are linked in the description.

> 
> One question that is briefly addressed in the LWN article and was
> thoroughly discussed by members of the X.Org boards, Daniel Stone, and
> others in hallway discussions is the question of whether to extend the
> X.Org membership to projects hosted on fd.o but outside the purpose of
> the X.Org foundation as enacted in its bylaws.
> 
> Most people I talked to would prefer not to dilute X.Org's mission and
> extend membership only to contributors of projects that follow X.Org's
> purpose as enacted in its bylaws. Other projects can continue to be
> hosted on fd.o but won't receive X.Org membership for the mere reason
> of
> being hosted on fd.o.

With my member hat on, I think this is the best choice.
Acked-by: Eric Engestrom 

> 
> [1] https://lwn.net/Articles/767258/
> 
> v2:
>  - Subject line that better describes the intention
>  - Briefly describe reasons behind this change
>  - Drop expanding membership eligibility
> ---
> 
> We're looking for feedback and comments on this patch. If it's not
> widely controversial the final version of the patch will be put to a
> vote at the 2019 X.Org elections.
> 
> The patch applies to the X.Org bylaws git repo, which can be found at
> https://gitlab.freedesktop.org/xorgfoundation/bylaws
> 
> Happy commenting.
> 
> Harry
> 
> bylaws.tex | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/bylaws.tex b/bylaws.tex
> index 4ab35a4f7745..44ff4745963b 100644
> --- a/bylaws.tex
> +++ b/bylaws.tex
> @@ -14,7 +14,7 @@ BE IT ENACTED AND IT IS HEREBY ENACTED as a By-law
> of the X.Org Foundation
>  
>  The purpose of the X.Org Foundation shall be to:
>  \begin{enumerate}[(i)\hspace{.2cm}]
> - \item Research, develop, support, organize, administrate,
> standardize,
> + \item \label{1} Research, develop, support, organize, administrate,
> standardize,
>   promote, and defend a free and open accelerated graphics stack. This
>   includes, but is not limited to, the following projects: DRM, Mesa,
>   Wayland and the X Window System,
> @@ -24,6 +24,11 @@ The purpose of the X.Org Foundation shall be to:
>  
>   \item Support and educate the general community of users of this
>   graphics stack.
> +
> + \item Support free and open source projects through the
> freedesktop.org
> + infrastructure. For projects outside the scope of item (\ref{1})
> support
> + extends to project hosting only.
> +
>  \end{enumerate}
>  
>  \article{INTERPRETATION}
> -- 
> 2.19.1
> 
> ___
> memb...@foundation.x.org: X.Org Foundation Members
> Archives: https://foundation.x.org/cgi-bin/mailman/private/members
> Info: https://foundation.x.org/cgi-bin/mailman/listinfo/members
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Retiring the GitHub mirrors

[Eric Engestrom]

Hello everyone,

For many years now, we have maintained mirrors of numerous FDo projects
on GitHub, under the following organisations:
https://github.com/freedesktop
https://github.com/mesa3d
https://github.com/wayland-project

A bit over a month ago, a new security feature in git started preventing
our script from updating these repos, which wasn't noticed right away
due to people being on holiday. The question that has come up as
a result is whether there is any need to keep working on these mirrors,
or whether they should be deleted instead.

Among the people present in this discussion, the consensus was that we
should delete them.

If you need to keep the mirror for your repo alive, you can set up
push mirroring from GitLab [1], and let us know by *February 20th*
that we should not delete your repo.

Cheers,
  Eric, on behalf of the FDo admins

[1] https://docs.gitlab.com/ee/user/project/repository/mirror/push.html


signature.asc
Description: PGP signature