Re: [PATCH libinput] meson.build: add more overrides for coverity
Reviewed-by: Matheus Santana
On Tue, Jun 19, 2018 at 9:28 PM, Peter Hutterer
wrote:
> "/usr/include/math.h", line 381: error #20: identifier "_Float32" is
> undefined
> # define _Mdouble__Float32
>
> Same for a few others. Since we don't actually need those anyway, we can
> just
> cast those to the some close-enough sizes. We don't have stdint.h in
> config.h
> and meson cannot have a custom #include line in the config object. So
> let's go
> with what does the job for now.
>
> Signed-off-by: Peter Hutterer
> ---
> The extra additions are probably caused by my update to F28, but something
> is wrong on the coverity side here and I'd rather #define random things
> away than not run coverity...
>
> meson.build | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/meson.build b/meson.build
> index 54c5bbe0..b650bbde 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -52,6 +52,10 @@ endif
> # be removed when coverity fixes this again.
> if get_option('coverity')
> config_h.set('_Float128', '__uint128_t')
> + config_h.set('_Float32', 'int')
> + config_h.set('_Float32x', 'int')
> + config_h.set('_Float64', 'long')
> + config_h.set('_Float64x', 'long')
> endif
>
> # Dependencies
> --
> 2.17.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 libinput] util: abort if we try to allocate more than a MB
Great. I wasn't aware of the test case. Thanks for the heads up!
Best regards,
Matheus
On Tue, Jun 19, 2018 at 9:32 PM, Peter Hutterer
wrote:
> On Tue, Jun 19, 2018 at 09:22:52PM -0300, Matheus Santana wrote:
> > Reviewed-by: Matheus Santana
> >
> > The check for negatives isn't needed anymore?
>
> you mean zalloc_overflow? good point. I'll leave it in though because it
> does test a valid error case. I've added more tests for zalloc(some large
> number) though.
>
> diff --git a/test/litest-selftest.c b/test/litest-selftest.c
> index 72bdabac..ab185d2a 100644
> --- a/test/litest-selftest.c
> +++ b/test/litest-selftest.c
> @@ -350,6 +350,19 @@ START_TEST(zalloc_overflow)
> }
> END_TEST
>
> +START_TEST(zalloc_max_size)
> +{
> + /* Built-in alloc maximum */
> + zalloc(1024 * 1024);
> +}
> +END_TEST
> +
> +START_TEST(zalloc_too_large)
> +{
> + zalloc(1024 * 1024 + 1);
> +}
> +END_TEST
> +
> static Suite *
> litest_assert_macros_suite(void)
> {
> @@ -415,7 +428,9 @@ litest_assert_macros_suite(void)
> suite_add_tcase(s, tc);
>
> tc = tcase_create("zalloc ");
> + tcase_add_test(tc, zalloc_max_size);
> tcase_add_test_raise_signal(tc, zalloc_overflow, SIGABRT);
> + tcase_add_test_raise_signal(tc, zalloc_too_large, SIGABRT);
> suite_add_tcase(s, tc);
>
> return s;
>
> Cheers,
>Peter
>
> >
> > On Tue, Jun 19, 2018 at 8:44 PM, Peter Hutterer <
> peter.hutte...@who-t.net>
> > wrote:
> >
> > > The ssize_t cast upsets coverity for some reason but we can be a lot
> more
> > > restrictive here anyway. Quick analysis of the zalloc calls in the test
> > > suite
> > > show the largest allocation is 9204 bytes.
> > >
> > > Let's put a cap on for one MB, anything above that is likely some
> memory
> > > corruption and should be caught early.
> > >
> > > Signed-off-by: Peter Hutterer
> > > ---
> > > src/libinput-util.h | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libinput-util.h b/src/libinput-util.h
> > > index 8c67dcbd..4f60e8ea 100644
> > > --- a/src/libinput-util.h
> > > +++ b/src/libinput-util.h
> > > @@ -142,7 +142,9 @@ zalloc(size_t size)
> > > {
> > > void *p;
> > >
> > > - if ((ssize_t)size < 0)
> > > + /* We never need to alloc anything even near one MB so we can
> > > assume
> > > +* if we ever get above that something's going wrong */
> > > + if (size > 1024 * 1024)
> > > abort();
> > >
> > > p = calloc(1, size);
> > > --
> > > 2.17.1
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] util: abort if we try to allocate more than a MB
On Tue, Jun 19, 2018 at 09:22:52PM -0300, Matheus Santana wrote:
> Reviewed-by: Matheus Santana
>
> The check for negatives isn't needed anymore?
you mean zalloc_overflow? good point. I'll leave it in though because it
does test a valid error case. I've added more tests for zalloc(some large
number) though.
diff --git a/test/litest-selftest.c b/test/litest-selftest.c
index 72bdabac..ab185d2a 100644
--- a/test/litest-selftest.c
+++ b/test/litest-selftest.c
@@ -350,6 +350,19 @@ START_TEST(zalloc_overflow)
}
END_TEST
+START_TEST(zalloc_max_size)
+{
+ /* Built-in alloc maximum */
+ zalloc(1024 * 1024);
+}
+END_TEST
+
+START_TEST(zalloc_too_large)
+{
+ zalloc(1024 * 1024 + 1);
+}
+END_TEST
+
static Suite *
litest_assert_macros_suite(void)
{
@@ -415,7 +428,9 @@ litest_assert_macros_suite(void)
suite_add_tcase(s, tc);
tc = tcase_create("zalloc ");
+ tcase_add_test(tc, zalloc_max_size);
tcase_add_test_raise_signal(tc, zalloc_overflow, SIGABRT);
+ tcase_add_test_raise_signal(tc, zalloc_too_large, SIGABRT);
suite_add_tcase(s, tc);
return s;
Cheers,
Peter
>
> On Tue, Jun 19, 2018 at 8:44 PM, Peter Hutterer
> wrote:
>
> > The ssize_t cast upsets coverity for some reason but we can be a lot more
> > restrictive here anyway. Quick analysis of the zalloc calls in the test
> > suite
> > show the largest allocation is 9204 bytes.
> >
> > Let's put a cap on for one MB, anything above that is likely some memory
> > corruption and should be caught early.
> >
> > Signed-off-by: Peter Hutterer
> > ---
> > src/libinput-util.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libinput-util.h b/src/libinput-util.h
> > index 8c67dcbd..4f60e8ea 100644
> > --- a/src/libinput-util.h
> > +++ b/src/libinput-util.h
> > @@ -142,7 +142,9 @@ zalloc(size_t size)
> > {
> > void *p;
> >
> > - if ((ssize_t)size < 0)
> > + /* We never need to alloc anything even near one MB so we can
> > assume
> > +* if we ever get above that something's going wrong */
> > + if (size > 1024 * 1024)
> > abort();
> >
> > p = calloc(1, size);
> > --
> > 2.17.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput] meson.build: add more overrides for coverity
"/usr/include/math.h", line 381: error #20: identifier "_Float32" is undefined
# define _Mdouble__Float32
Same for a few others. Since we don't actually need those anyway, we can just
cast those to the some close-enough sizes. We don't have stdint.h in config.h
and meson cannot have a custom #include line in the config object. So let's go
with what does the job for now.
Signed-off-by: Peter Hutterer
---
The extra additions are probably caused by my update to F28, but something
is wrong on the coverity side here and I'd rather #define random things
away than not run coverity...
meson.build | 4
1 file changed, 4 insertions(+)
diff --git a/meson.build b/meson.build
index 54c5bbe0..b650bbde 100644
--- a/meson.build
+++ b/meson.build
@@ -52,6 +52,10 @@ endif
# be removed when coverity fixes this again.
if get_option('coverity')
config_h.set('_Float128', '__uint128_t')
+ config_h.set('_Float32', 'int')
+ config_h.set('_Float32x', 'int')
+ config_h.set('_Float64', 'long')
+ config_h.set('_Float64x', 'long')
endif
# Dependencies
--
2.17.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] util: abort if we try to allocate more than a MB
Reviewed-by: Matheus Santana
The check for negatives isn't needed anymore?
On Tue, Jun 19, 2018 at 8:44 PM, Peter Hutterer
wrote:
> The ssize_t cast upsets coverity for some reason but we can be a lot more
> restrictive here anyway. Quick analysis of the zalloc calls in the test
> suite
> show the largest allocation is 9204 bytes.
>
> Let's put a cap on for one MB, anything above that is likely some memory
> corruption and should be caught early.
>
> Signed-off-by: Peter Hutterer
> ---
> src/libinput-util.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/libinput-util.h b/src/libinput-util.h
> index 8c67dcbd..4f60e8ea 100644
> --- a/src/libinput-util.h
> +++ b/src/libinput-util.h
> @@ -142,7 +142,9 @@ zalloc(size_t size)
> {
> void *p;
>
> - if ((ssize_t)size < 0)
> + /* We never need to alloc anything even near one MB so we can
> assume
> +* if we ever get above that something's going wrong */
> + if (size > 1024 * 1024)
> abort();
>
> p = calloc(1, size);
> --
> 2.17.1
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput] util: abort if we try to allocate more than a MB
The ssize_t cast upsets coverity for some reason but we can be a lot more
restrictive here anyway. Quick analysis of the zalloc calls in the test suite
show the largest allocation is 9204 bytes.
Let's put a cap on for one MB, anything above that is likely some memory
corruption and should be caught early.
Signed-off-by: Peter Hutterer
---
src/libinput-util.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/libinput-util.h b/src/libinput-util.h
index 8c67dcbd..4f60e8ea 100644
--- a/src/libinput-util.h
+++ b/src/libinput-util.h
@@ -142,7 +142,9 @@ zalloc(size_t size)
{
void *p;
- if ((ssize_t)size < 0)
+ /* We never need to alloc anything even near one MB so we can assume
+* if we ever get above that something's going wrong */
+ if (size > 1024 * 1024)
abort();
p = calloc(1, size);
--
2.17.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] client: Allow send error recovery without an abort
Hi Pekka, On Tue, Jun 19, 2018 at 5:12 AM Pekka Paalanen wrote: > I think the aim should be to remove the abort(). I do not see any > reason to leave it in even as an option. > Thank you. Will proceeed in that direction. > The soft-buffer in libwayland-client is quite small, the kernel socket > buffers are usually much larger in bytes. EAGAIN is triggered only when > the flush to the kernel fails, so I don't think that would happen > needlessly. > Ack. > If I'm reading the code right, MAX_FDS_OUT is only a limit for a single > sendmsg() call. The fds_out buffer is capable of holding 1k fds. > > We have 4kB message data buffer (what I've been calling the > soft-buffer, as opposed to in-kernel buffers). A minimal request to > carry an fd would be header (8B) plus fd (4B), so that's 12B. That > makes at most 341 fds until the soft-buffer is full. That should well > fit into the 1k fds that fds_out can hold. > > One could have more fds in a single request. Let's say four. The > request size becomes 24B carrying 4 fds. That makes 680 fds for a full > soft-buffer. Should still be fine. The limit is both the maximum amount per sendmsg() call, as well as the maximum amount that can be sent by a single send request. The fd's are added once at a time by a call to wl_connection_put_fd(), which forces a flush once the limit is reached. Those calls are made by copy_fds_to_connection(), called once per message send from wl_closure_send(). On a flush (forced or not), the fd's are closed by close_fds(), which then advances the tail to the head, emptying the ring buffer again. That the output fd butter is over-large at 4k just allows much of the ring buffer code to be reused. It will never actually hold 1024 fds enqueued for output though. (The input fd buffer may allow 1k fd's. I did not examine that carefully to be sure). I think this limit is still a problem. I could change it to actually allow 1k fd's to be queued up, but then that will make the dup() more likely to fail (more in the other reply). > However, wl_connection_flush() attempts to write out at most > MAX_FDS_OUT fds at a time, but all of the message data. OTOH, one > cannot even try to flush out fds without any message data, IIRC > sendmsg() doesn't support that. So it can run out of message data while > fds accumulate and finally fill fds_out up, causing irrecoverable > failure. > > Do your observations support this theory? > I haven't observed a problem case like that. Any normal individual request will probably only use a few fd's, and I don't think there is a way for a request to not carry some non-fd data. I can imagine someone creating a new protocol message that does take more than 28 fd's in a request, where that would then fail to flush if there were no other normal message data also already queued up. But that seems a very abnormal use. wl_closure_marshal() would be the best place for a check (it already checks for null objects -- see "goto err_null"), and I didn't find other bound check on the number of fd's in a request on a quick look through the code. But I'll study the code a bit more in case there is some subtlety that escapes me. It seems we should somehow flush less message data so that we can call > sendmsg() enough times to handle all the fds and avoid them piling up. > But if we do so, we must be very careful with any potential assumptions > on receiving side on the ordering between message data and related fds. > I expect that the receiving code will just pull the next fd out of the fds_in ring buffer when turning the messages back into wl_closures for each message, as it realizes an fd is needed for any argument. But I haven't looked at the code. Another thing I will check to be safe. > Even the author of the client would usually not know. It would make a > very hard API to use correctly. > Ok, I won't introduce anything like that. > My objection is to the automatic blocking in general. We have a pretty > nicely asynchronous protocol and we try to teach programmers to write > their apps in a non-blocking way, so blocking inside libwayland-client > is not nice. We have very few functions that are documented as > potentially blocking. Extending that to all request sending functions > is not acceptable in my mind. > I agree and appreciate that the core library is non-blocking, and really don't want to change that, which is why my original patch made that blocking at least opt-in, and even then NOT even part of the core client code. - Lloyd ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] contributing: add review guidelines
Hi Pekka, On 18 June 2018 at 14:42, Pekka Paalanen wrote: > +- Stable ABI or API is not broken. > + I think I've just caught one of those ;-) Thanks for the vast, yet concise writeup. Fwiw Reviewed-by: Emil Velikov -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 4/4] tests: Reshuffle IVI layout tests
On 18 June 2018 at 15:40, Pekka Paalanen wrote: > From: Daniel Stone > > Rename the IVI tests to be more consistent with the others, and invert > the naming of plugin/client to make it slightly more clear what's going > to happen. Handle the renaming by using wet_get_binary_path to rewrite > the local binaries. > > As a side-effect, weston.ini ivi-shell-user-interface no longer needs to > be given as an absolute path. > > Signed-off-by: Daniel Stone > > v2: > > Call ivi-layout.ivi as ivi-layout-test-client.ivi to keep the same name > in both the file and the lookup, so that the module map does not need to > change the name. > > Update code comments to reflect the new names. > > Rename ivi_layout-test-plugin.c to ivi-layout-test-plugin.c. > About half of the patch is the mechanical rename - worth keeping that separate from the rest? Either way, patch looks spot on and is Reviewed-by: Emil Velikov -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/4] tests: Don't rely on build directory layout
Hi Pekka,
On 18 June 2018 at 15:40, Pekka Paalanen wrote:
> From: Daniel Stone
>
> Rather than having a hardcoded dependency on the build-directory layout,
> use an explicit module-map environment variable, which rewrites requests
> for modules and helper/libexec binaries to specific paths.
>
> This will help with migration to Meson where setting up the paths
> according to autotools would be painful and unnecessary.
>
Surely one didn't need a build system, to make nice improvements.
I guess, sometimes, tools do need to give us a wake-up call.
> +WL_EXPORT size_t
> +weston_module_path_from_env(const char *name, char *path, size_t path_len)
> +{
> + const char *mapping = getenv("WESTON_MODULE_MAP");
> + const char *end;
> +
strlen(name) once and use it throughout?
> + if (!mapping)
> + return 0;
> +
> + end = mapping + strlen(mapping);
> + while (mapping < end && *mapping) {
> + const char *filename, *next;
> +
> + /* early out: impossibly short string */
> + if ((size_t)(end - mapping) < strlen(name) + 1)
> + return 0;
> +
> + filename = [strlen(name) + 1];
> + next = strchrnul(mapping, ';');
> +
> + if (strncmp(mapping, name, strlen(name)) == 0 &&
> + mapping[strlen(name)] == '=') {
> + size_t file_len = next - filename; /* no trailing NUL
> */
> + if (file_len >= path_len)
> + return 0;
> + strncpy(path, filename, file_len);
> + path[file_len] = '\0';
A simple stat() can, more or less, ensure that we're not giving random
rubbish to the caller.
Worth it, or more of an overkill?
-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 2/4] shared: remove weston_config_get_libexec_dir()
On 18 June 2018 at 15:40, Pekka Paalanen wrote: > From: Pekka Paalanen > > Now that WESTON_MODULE_MAP supersedes WESTON_BUILD_DIR for libexec > binaries, we don't need to check in WESTON_BUILD_DIR anymore. > > There was only one user of weston_config_get_libexec_dir(), so remove > the whole function. There is no reason to export it. > Removal of public API should be accompanied with changing the API/ABI version. Bump of the DSO ABI is one way to handle it. -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 2/2] contributing: commit rights
On 18 June 2018 at 14:42, Pekka Paalanen wrote: > From: Pekka Paalanen > > This has been copied from > https://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/CONTRIBUTING?id=eccae1360d6d01e73c6af2bd97122cef708207ef > and slightly edited to better with Wayland and Weston. > > The intention is to make it easier to give out commit access to new > people, let them know what is expected of them, and help the community > to grow. Hopefully this will in time improve the patch review throughput > and timeliness. > > The original text was introduced in > https://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/CONTRIBUTING?id=0350f0e7f6a0e07281445fc3082aa70419f4aac7 > > Signed-off-by: Pekka Paalanen > --- > CONTRIBUTING.md | 33 + > 1 file changed, 33 insertions(+) > > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md > index 6e74b6d..70d0eca 100644 > --- a/CONTRIBUTING.md > +++ b/CONTRIBUTING.md > @@ -246,5 +246,38 @@ clarity suffers. > - The code adheres to the style guidelines. > > > +Commit rights > += > + > +Commit rights will be granted to anyone who requests them and fulfills the > +below criteria: > + > +- Submitted some (10 as a rule of thumb) non-trivial (not just simple > + spelling fixes and whitespace adjustment) patches that have been merged > + already. > + > +- Are actively participating in public discussions about their work (on the > + mailing list or IRC). This should not be interpreted as a requirement to > + review other peoples patches but just make sure that patch submission isn't > + one-way communication. Cross-review is still highly encouraged. > + > +- Will be regularly contributing further patches. This includes regular > + contributors to other parts of the open source graphics stack who only > + do the occasional development in this project. > + > +- Agrees to use their commit rights in accordance with the documented merge > + criteria, tools, and processes. > + > +To apply for commit rights, create a new issue in gitlab for the respective > +project and give it the "accounts" label. > + > +Committers are encouraged to request their commit rights get removed when > they > +no longer contribute to the project. Commit rights will be reinstated when > they > +come back to the project. > + > +Maintainers and committers should encourage contributors to request commit > +rights, especially junior contributors tend to underestimate their skills. > + > + Yay, glad to see some documentation on the topic. I wonder when it would have came out, if it wasn't for the gitlab move. Either way, thanks for doing this. Fwiw: Reviewed-by: Emil Velikov Aside: it would be a good idea who's review is considered sufficient to get merge patches. It would be great to spare the fun experiences of wayland-egl. Thanks Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCHv2 wayland 0/8] wayland-scanner: produce code with c99 initializers
On 18 June 2018 at 11:36, Pekka Paalanen wrote: > On Thu, 14 Jun 2018 16:49:37 +0100 > Emil Velikov wrote: > >> Hi all, >> >> Here's a take v2 of the series, with the following changes: >> - don't trim trailing NULL entries from the wl_interfaces* array >> - updated tests - separate patches to ease review, to be squashed >> >> On the question of why, despite the aesthetics these patches make the >> generated files actually understandable by a human being... > > Hi Emil, > > on the previous round, this concern was raised: > Thanks, did not spot that one. > On Tue, 13 Feb 2018 13:36:06 + > Daniel Stone wrote: > >> But that being said, my worry is that we don't actually control the >> compilation environment for the scanner output. Scanner output >> currently compiles with '-pedantic -ansi -Wall -Wextra' (at least, >> when inline is defined). This patch changes that requirement, and I >> worry that - like previous discussions on changing scanner output - >> that upgrading Wayland would lead to people hitting compilation >> failures. > > What is your rationale for that being a non-issue? > According to [1] GCC has supported designated initializers since v3.0, released some 17 years ago [2]. Clang has supported them from a very early age. On the Windows front MSVC 2013 introduced support and it EOL. Other less common compilers (say the Sun/Oracle or Intel ones) are fine as well - although I cannot give you exact details. In other words unless someone does one of the following two they're perfectly fine. - uses unsupported (ancient?) compiler, or - explicitly sets -pedantic -ansi _and_ -Werror In the case they do, they should seriously reconsider what they're inflicting on themselves. Both from functionality and security POV. -Emil [1] https://gcc.gnu.org/c99status.html [2] https://gcc.gnu.org/releases.html ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
On Tue, 19 Jun 2018 at 16:21:14 +0200, Michel Dänzer wrote: > On 2018-06-19 02:22 PM, Simon McVittie wrote: > > This indirect launching also avoids doing a fork-and-exec in the > > compositor or session manager, which can be problematic under low-memory > > conditions: the kernel needs to allocate enough virtual memory space for > > a second copy of the parent process (even though the physical memory that > > backs it is going to be copy-on-write, and in fork-and-exec it only exists > > briefly and is mostly unmodified) > > FWIW, /dev/dri/* files are supposed to be always opened with O_CLOEXEC, > to prevent the file descriptors from leaking to children. Sure, but that's close-on-exec, not close-on-fork. The kernel doesn't know you aren't going to continue executing application-level code in both parent and child[1]. > There were > bugs in libdrm meaning this didn't always happen, but those are fixed in > current upstream. Just about everything is *meant to be* opened O_CLOEXEC, but that doesn't mean it is :-) smcv [1] Never do this if your process might be multi-threaded, and probably don't do this even if it's known to be single-threaded ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] contributing: add review guidelines
On Tue, 19 Jun 2018 11:45:24 +0100 Daniel Stone wrote: > Hi Pekka, > > On Mon, 18 Jun 2018 at 14:43, Pekka Paalanen wrote: > > This sets up the standards for patch review, and defines when a patch > > can be merged. I believe these are the practises we have been using > > already for a long time, now they are just written down explicitly. > > > > It's not an exhaustive list of criteria and likely cannot ever be, but > > it should give a good idea of what level of review we want to have. > > > > It has been written in general terms, so that we can easily apply the > > same text not just to Wayland, but also Weston and other projects as > > necessary. > > > > This addition is not redundant with > > https://wayland.freedesktop.org/reviewing.html . > > > > The web page is a friendly introduction and encouragement for people to > > get involved. The guidelines here are more specific and aimed for people > > who seek commit rights or maintainership. > > Thanks a lot for tackling this! Having it written down is fantastic, > and I like how you've put it. I have two minor suggestions below: > > > +- The code is correct and does not ignore corner-cases. > > In general this is what we aim for, but the presence of 'XXX'/'FIXME' > comments in the code shows we don't always live up to that ideal. ;) > Maybe this could instead be something like: > > The code is correct and does not introduce new failures for existing > users, nor add new corner-case bugs. Yes, that was my intention. > When fixing bugs, 'root cause' fixes are preferred rather than hiding > symptoms: identify why the issue has occurred, and fix it as close as > possible to the source. If it is not practical to completely fix the > issue, partial fixes should be accompanied by a comment in proximate > code, as well as a new issue opened in GitLab clearly noting known > corner cases with a suggestion on how to fix them. A good point. > > > +- The code does what it says in the commit message and changes nothing > > else. > > Which leads naturally to something like: > > If the commit message starts getting too long and addresses multiple > points, this may be a sign that the commit should be broken into > smaller individual commits. Yes. > I'm happy to take alternate wording, since what you've written here is > far more concise than mine. Anyway, both patches are: > Reviewed-by: Daniel Stone On one hand I'd like it to be very concise, so that people don't get put off by it. The list already seemed a bit long, and one could write books about it. On the other hand I'd like to expand on all points. Should we favour short bullet points or longer explanations? The Markdown formatting also sets some restrictions, and I think it's likely the list will only grow longer. I suppose your intention is that we can land these two patches as is, and make amendments in follow-ups, right? I'd like to let this soak on the mailing list for a while and see what other comments and R-bs we get. > Could this later be used as the basis for similar Weston patches? I assumed we just point here from Weston, if we don't already, at least until differences between Wayland and Weston emerge. Thanks, pq pgpyRkZ2mMMuL.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
RE: [PATCH weston 0/4] Preparing for Meson
Hi Pekka,
All your patches are reviewed by me:
Reviewed-by: Emre Ucan
Don't relying on build directory has the major advantage that it is lot easier
to run weston unit-tests in target environment if weston is cross compiled.
Best regards
Emre Ucan
Engineering Software Base (ADITG/ESB)
Tel. +49 5121 49 6937
> -Original Message-
> From: wayland-devel [mailto:wayland-devel-
> boun...@lists.freedesktop.org] On Behalf Of Pekka Paalanen
> Sent: Montag, 18. Juni 2018 16:41
> To: wayland-devel@lists.freedesktop.org
> Cc: Pekka Paalanen
> Subject: [PATCH weston 0/4] Preparing for Meson
>
> From: Pekka Paalanen
>
> Hi,
>
> these changes are intended to make using Meson easier. Daniel's patches
> are v2, mine are v1.
>
>
> Daniel Stone (2):
> tests: Don't rely on build directory layout
> tests: Reshuffle IVI layout tests
>
> Pekka Paalanen (2):
> shared: remove weston_config_get_libexec_dir()
> tests: remove WESTON_BUILD_DIR from env
>
> Makefile.am| 16 +++---
> compositor/main.c | 25 +++---
> compositor/text-backend.c | 6 +--
> compositor/weston-screenshooter.c | 8 ++-
> compositor/weston.h| 3 ++
> desktop-shell/shell.c | 9 +---
> ivi-shell/hmi-controller.c | 12 -
> ivi-shell/weston.ini.in| 8 +--
> libweston/compositor.c | 57
> --
> libweston/compositor.h | 3 ++
> shared/config-parser.c | 12 -
> shared/config-parser.h | 2 -
> ...-internal-test.c => ivi-layout-internal-test.c} | 0
> ...{ivi_layout-test.c => ivi-layout-test-client.c} | 8 +--
> ...yout-test-plugin.c => ivi-layout-test-plugin.c} | 26 +-
> tests/weston-tests-env | 21 ++--
> 16 files changed, 140 insertions(+), 76 deletions(-)
> rename tests/{ivi_layout-internal-test.c => ivi-layout-internal-test.c}
> (100%)
> rename tests/{ivi_layout-test.c => ivi-layout-test-client.c} (97%)
> rename tests/{ivi_layout-test-plugin.c => ivi-layout-test-plugin.c} (98%)
>
> --
> 2.16.4
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Dual display in clone mode or extended mode with Weston
Hello All, I have read queries for dual display on freedesktop.org I need your help in the same context. Currently we are creating one application to support multiple displays with Wayland. We are unaware that one compositor will be sufficient for dual display. We need to know about how virtual framebuffer is created per display. As DRM supports only one compositor, How to display same content on second display OR can we have different user events on second display monitor. Please help me to find the solution. Regards, Ashvini Deshmukh ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
EXT: [PATCH weston 0/1] Allow forcing outputs on
Hi, While I was testing this patch I ended up with: interface: 'wl_output', version: 3, name: 14 x: 2732, y: 0, scale: 1, This happened when I used force-on on second "HDMI-A-2" display. When I used force-on on HDMI-A-1 everything worked as expected. anx x=y=0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
On 2018-06-19 02:22 PM, Simon McVittie wrote: > > This indirect launching also avoids doing a fork-and-exec in the > compositor or session manager, which can be problematic under low-memory > conditions: the kernel needs to allocate enough virtual memory space for > a second copy of the parent process (even though the physical memory that > backs it is going to be copy-on-write, and in fork-and-exec it only exists > briefly and is mostly unmodified), and when a compositor has your GPU's > address space mmapped, that's actually a significant practical problem. > I think Endless have had trouble with this in GNOME Shell on 32-bit - > there's easily enough address space to have the GPU mmapped once, but > if GNOME Shell does a fork-and-exec, the child process briefly has the > GPU mmapped for a second time, and there isn't always enough address > space free to do that. FWIW, /dev/dri/* files are supposed to be always opened with O_CLOEXEC, to prevent the file descriptors from leaking to children. There were bugs in libdrm meaning this didn't always happen, but those are fixed in current upstream. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
On 2018/6月/19 01:39, Simon McVittie wrote:
> On Tue, 19 Jun 2018 at 13:56:22 +0200, Markus Ongyerth wrote:
> > P.S. I just thought about this ab it more, and something else came to my
> > mind:
> > How is env passed with dbus activation? afaik the session bus does not run
> > in
> > under the compositor, so if we aren't on wayland-0 (which can happen very
> > well
> > in dev situations, and multi seats, and ...) we'd have to tell the client
> > as
> > well.
>
> The session bus can run either inside or outside the compositor. It
> depends how you have designed your OS, and what you consider "the session"
> to mean. Desktop infrastructure can tell the dbus-daemon what environment
> variables it should pass to its child processes (but there can only be
> one answer at a time).
>
> In the terminology of
> https://lists.freedesktop.org/archives/dbus/2015-January/016522.html
> a "session = login session" world would have one of these:
>
> user session for uid 1000 (owns XDG_RUNTIME_DIR)
> \- login session c1
> \- compositor
> \- compositor-launched app
> \- session bus
> \- D-Bus-activated app
>
> or
>
> user session for for uid 1000 (owns XDG_RUNTIME_DIR)
> \- login session c1
> \- session bus
> \- D-Bus-activated app
> \- compositor
> \- compositor-launched app
>
> (in the latter case you'd probably want to use
> UpdateActivationEnvironment() to send the WAYLAND_DISPLAY to the session
> bus)
>
> while a "session = user session" world would have this:
>
> user session for for uid 1000 (owns XDG_RUNTIME_DIR)
> \- systemd --user (optional)
> \- systemd-launched app (possibly done via D-Bus)
> \- session bus
> \- traditionally D-Bus-activated app
> \- login session c1
> \- compositor
> \- compositor-launched app
>
> (again, you could use UpdateActivationEnvironment() to send the
> WAYLAND_DISPLAY to the session bus if necessary)
Sounds racy if there's more than one session on the same bus.
Locking ourselves into a single-compositor setup is quite annoying. And IMO we
shouldn't rely on changes that affect the entire system setup.
It would at the very least increase the hurdle to get new devs, maybe even
deter existing devs from touching any feature there.
>
> The Activate() method defined by the Desktop Entry Specification passes
> a key:value map of arbitrary "platform data" to the activated process.
> It would be reasonable to include something like
> {'wayland_display':<'wayland-1'>}, meaning "use this Wayland display if
> possible, overriding the environment". (However, if the app only supports
> connecting to one Wayland display at a time, you're still stuck.)
I wonder if it's that much better to define wayland specific variables on that
end, than it is to specify dbus specific things on this end.
In the optimal case they would be largely independant.
>
> > Thinking about clients that may have multiple instances, but not shared
> > sessions, e.g. some IDE with two projects opened.
> > iiuc the activation would start one process, which could then either
> > restore,
> > or open as normal, but when another project is openend, the process is
> > already
> > running and has to start another instance.
> > Over just having a single point of entry where it's started with
> > restoration,
> > or without.
>
> Clients with multiple instances (an IDE with a process per project) are a
> better fit for a fork-and-exec design, yes.
>
> Conversely, clients with a single instance (an IDE with one big process
> that can host multiple projects each in their own top-level window,
> like emacs with emacsclient, or GNOME Builder) are a better fit for an
> instance-as-a-service design.
Those have their own private IPC, and as far as I have seen, every application
supports launching it and passing the arguments over an internal channel.
Which might be configured to not do this, or evaluate over some other env
(e.g. WAYLAND_DISPLAY) if it should do that.
> Clients with a single top-level process
> that manages a hierarchy of internal "helper" subprocesses, like web
> browsers, are probably also a better fit for the design.
There's also an issue with the protocol design here. We can only have one
restoration id per wayland client. So this needs to be changed for those
either way.
>
> smcv
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
On 2018/6月/19 01:22, Simon McVittie wrote: > On Tue, 19 Jun 2018 at 11:18:17 +0200, Markus Ongyerth wrote: > > On 2018/6月/18 05:05, Roman Gilg wrote: > > > * using D-Bus interface only to secure against sandboxed clients > > What? Why exactly? When I first read this, I expected that the client is > > supposed to use the portal stuff to call out of a sandbox, but reading > > through > > the actual protocol, I think the only usage of D-Bus is to launch the > > applications from the compositor? > > Any app implementing DBusActivatable (see the Desktop Entry Specification Why would we arbitrarily limit this mechanism to this subset of applications? This is a wayland extension protocol, and unless there's a good reason to limit it further, this should be avoided > for more details) needs to be able to receive method calls from desktop > infrastructure outside its sandbox anyway. > For instance, Flatpak always > allows method calls into the sandbox, while restricting method calls > out of the sandbox according to app-specific rules. > > > Since this needs explicit client support either way, IMO this could be > > implmented by adding a required cmdline argument (e.g. > > --org-freedsktop-restore=UID) instead. > > In desktop environments that support it, it's advantageous to launch apps > as D-Bus services, or even as systemd user services if they support > being launched like that (dbus-daemon will automatically delegate > activation to systemd --user where supported), so that they always run > in a reasonably consistent execution environment. If various different > components (compositor, main menu if it's separate from the compositor, > random app that wants to open a web page, etc.) launch an app using > fork-and-exec with the Exec= command line from its .desktop file, then > the app will inherit its execution environment (environment variables, > file descriptors, rlimits, other process flags) from an arbitrary parent > process, depending on the precise details of how they were launched. If > they're DBusActivatable, they'll all be children of systemd --user (if > supported) or dbus-daemon --session (if not), making their execution > environment a lot more predictable. Most of the compositor env makes sense for applications. Pushing open fds (outside std fds) into the child might be a compositor bug, or may be intended. E.g. for preconfigured wl_clients. Some others make sense to prevent, like the rlimits, i agree. But the compositor isn't some arbitrary client either. > > This indirect launching also avoids doing a fork-and-exec in the > compositor or session manager, which can be problematic under low-memory > conditions: the kernel needs to allocate enough virtual memory space for > a second copy of the parent process (even though the physical memory that > backs it is going to be copy-on-write, and in fork-and-exec it only exists > briefly and is mostly unmodified), and when a compositor has your GPU's > address space mmapped, that's actually a significant practical problem. > I think Endless have had trouble with this in GNOME Shell on 32-bit - > there's easily enough address space to have the GPU mmapped once, but > if GNOME Shell does a fork-and-exec, the child process briefly has the > GPU mmapped for a second time, and there isn't always enough address > space free to do that. Yea, that's indeed a problem. > > The DBusActivatable protocol doesn't support sending arbitrary > command-line arguments to the launched app, because in general, > command-line arguments are not meaningful for an app that is already > running. This is for restoration, and the activation is only used for exactly restoring the application after it's closed. Sending this to an already running application is the weird case that only exists because of using dbus here. > (It does support sending the URLs of files to be opened.) > Assuming we want to avoid always having to fall back to fork-and-exec, > which is undesirable for the reasons above, we'll need some way to > pass the state to restore via D-Bus - either a new method call as Roman > proposed here, or the platform_data argument to Activate() as I suggested. > > It would be possible to have a D-Bus preferred path and a fork-and-exec > fallback path that are defined to be equivalent, like the Desktop > Entry Specification does now for ordinary launching and for "Desktop > Actions". That would have to work via either a command-line argument or an > environment variable, because in fork-and-exec world those are the only > tools we have. However, the meanings (and even syntax) of command-line > arguments are conceptually "owned" by the app author, not by some third > party like freedesktop.org; and environment variables are undesirable > here because they inherit to child processes. This is opt in either way, it can't just be supported without a client knowing about this, so it's not like we'll get arbitrary collisions with existing flags. And if it's
Re: [PATCH] client: Allow send error recovery without an abort
On Mon, 18 Jun 2018 19:54:23 -0700 Lloyd Pique wrote: > Let me take things back a step. I was a bit too hasty in suggesting > something that would work for me for the fact that MAX_FDS_OUT is small. In > our client the buffer creation ends up being serialized, and so only one > thread will be creating buffers with the linux-dmabuf protocol at a time. > > That might not be true for other clients, and so checking if the fd buffer > is almost full won't work. In fact I'm not so certain checking if it is > half full would work either... having more than 14 threads trying to create > such buffers at the same time does not seem unreasonable, and if more than > one fd were involved in each request (multi-plane formats), the number of > threads needed to hit the wl_abort anyway drops quickly. And there may be > some other protocol I'm not aware of that is worse. > > Increasing the fd buffer would alleviate that. However it would introduce > another problem. Hi, as I explained in my other email, the fd buffer can already hold 1k fds. The problem in wl_connection_flush() seems to be that it runs out of message data to flush out all the fds, so the fds can just keep piling up. That seems like a problem worth solving on its own right. Once that is solved, thresholding on the message data buffer (soft-buffer) should be enough to keep the fd buffer never growing out of bounds. Maybe then this would work: - ABI to query a "flush recommended" flag; This flag would be set when the soft-buffer is at least half-full, and cleared when it drops to... below half? empty? - When a client is doing lots of request sending without returning to its main loop which would call wl_display_flush() anyway, it can query the flag to see if it needs to flush. - If flush ever fails, stop all request sending, poll for writable and try again. How to do this is left for the application. Most importantly, the application could set some state and return to its main event loop to do other stuff in the mean while. You're right that this wouldn't help an application that sends requests from multiple threads a lot. They would need to be checking the flag practically for every few requests, but at least that would be cheaper than calling wl_display_flush() outright. > We also have wl_abort()'s from the call to wl_os_dupfd_cloexec() in > wl_closure_marshal() failing. Not very often, and we are willing to pass on > finding a fix for it for now, but increasing the number of fd's being held > for the transfer is definitely going to make that worse. True. Can you think of any way to recover from dupfd failure without disconnecting? The very least it could be made to disconnect instead of abort. > Perhaps making the writes be blocking is the only reasonable way after all? > > What do you think? In my mind that would be a huge regression, so I wouldn't like it. If we exhaust all other options, we could see about that. Thanks, pq pgpf3Y97U0OZs.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
On Tue, 19 Jun 2018 at 13:56:22 +0200, Markus Ongyerth wrote:
> P.S. I just thought about this ab it more, and something else came to my mind:
> How is env passed with dbus activation? afaik the session bus does not run in
> under the compositor, so if we aren't on wayland-0 (which can happen very
> well
> in dev situations, and multi seats, and ...) we'd have to tell the client as
> well.
The session bus can run either inside or outside the compositor. It
depends how you have designed your OS, and what you consider "the session"
to mean. Desktop infrastructure can tell the dbus-daemon what environment
variables it should pass to its child processes (but there can only be
one answer at a time).
In the terminology of
https://lists.freedesktop.org/archives/dbus/2015-January/016522.html
a "session = login session" world would have one of these:
user session for uid 1000 (owns XDG_RUNTIME_DIR)
\- login session c1
\- compositor
\- compositor-launched app
\- session bus
\- D-Bus-activated app
or
user session for for uid 1000 (owns XDG_RUNTIME_DIR)
\- login session c1
\- session bus
\- D-Bus-activated app
\- compositor
\- compositor-launched app
(in the latter case you'd probably want to use
UpdateActivationEnvironment() to send the WAYLAND_DISPLAY to the session
bus)
while a "session = user session" world would have this:
user session for for uid 1000 (owns XDG_RUNTIME_DIR)
\- systemd --user (optional)
\- systemd-launched app (possibly done via D-Bus)
\- session bus
\- traditionally D-Bus-activated app
\- login session c1
\- compositor
\- compositor-launched app
(again, you could use UpdateActivationEnvironment() to send the
WAYLAND_DISPLAY to the session bus if necessary)
The Activate() method defined by the Desktop Entry Specification passes
a key:value map of arbitrary "platform data" to the activated process.
It would be reasonable to include something like
{'wayland_display':<'wayland-1'>}, meaning "use this Wayland display if
possible, overriding the environment". (However, if the app only supports
connecting to one Wayland display at a time, you're still stuck.)
> Thinking about clients that may have multiple instances, but not shared
> sessions, e.g. some IDE with two projects opened.
> iiuc the activation would start one process, which could then either restore,
> or open as normal, but when another project is openend, the process is
> already
> running and has to start another instance.
> Over just having a single point of entry where it's started with restoration,
> or without.
Clients with multiple instances (an IDE with a process per project) are a
better fit for a fork-and-exec design, yes.
Conversely, clients with a single instance (an IDE with one big process
that can host multiple projects each in their own top-level window,
like emacs with emacsclient, or GNOME Builder) are a better fit for an
instance-as-a-service design. Clients with a single top-level process
that manages a hierarchy of internal "helper" subprocesses, like web
browsers, are probably also a better fit for the design.
smcv
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
On Tue, 19 Jun 2018 at 11:18:17 +0200, Markus Ongyerth wrote: > On 2018/6月/18 05:05, Roman Gilg wrote: > > * using D-Bus interface only to secure against sandboxed clients > What? Why exactly? When I first read this, I expected that the client is > supposed to use the portal stuff to call out of a sandbox, but reading > through > the actual protocol, I think the only usage of D-Bus is to launch the > applications from the compositor? Any app implementing DBusActivatable (see the Desktop Entry Specification for more details) needs to be able to receive method calls from desktop infrastructure outside its sandbox anyway. For instance, Flatpak always allows method calls into the sandbox, while restricting method calls out of the sandbox according to app-specific rules. > Since this needs explicit client support either way, IMO this could be > implmented by adding a required cmdline argument (e.g. > --org-freedsktop-restore=UID) instead. In desktop environments that support it, it's advantageous to launch apps as D-Bus services, or even as systemd user services if they support being launched like that (dbus-daemon will automatically delegate activation to systemd --user where supported), so that they always run in a reasonably consistent execution environment. If various different components (compositor, main menu if it's separate from the compositor, random app that wants to open a web page, etc.) launch an app using fork-and-exec with the Exec= command line from its .desktop file, then the app will inherit its execution environment (environment variables, file descriptors, rlimits, other process flags) from an arbitrary parent process, depending on the precise details of how they were launched. If they're DBusActivatable, they'll all be children of systemd --user (if supported) or dbus-daemon --session (if not), making their execution environment a lot more predictable. This indirect launching also avoids doing a fork-and-exec in the compositor or session manager, which can be problematic under low-memory conditions: the kernel needs to allocate enough virtual memory space for a second copy of the parent process (even though the physical memory that backs it is going to be copy-on-write, and in fork-and-exec it only exists briefly and is mostly unmodified), and when a compositor has your GPU's address space mmapped, that's actually a significant practical problem. I think Endless have had trouble with this in GNOME Shell on 32-bit - there's easily enough address space to have the GPU mmapped once, but if GNOME Shell does a fork-and-exec, the child process briefly has the GPU mmapped for a second time, and there isn't always enough address space free to do that. The DBusActivatable protocol doesn't support sending arbitrary command-line arguments to the launched app, because in general, command-line arguments are not meaningful for an app that is already running. (It does support sending the URLs of files to be opened.) Assuming we want to avoid always having to fall back to fork-and-exec, which is undesirable for the reasons above, we'll need some way to pass the state to restore via D-Bus - either a new method call as Roman proposed here, or the platform_data argument to Activate() as I suggested. It would be possible to have a D-Bus preferred path and a fork-and-exec fallback path that are defined to be equivalent, like the Desktop Entry Specification does now for ordinary launching and for "Desktop Actions". That would have to work via either a command-line argument or an environment variable, because in fork-and-exec world those are the only tools we have. However, the meanings (and even syntax) of command-line arguments are conceptually "owned" by the app author, not by some third party like freedesktop.org; and environment variables are undesirable here because they inherit to child processes. Having two code paths also means that a high-quality implementation has to test and support both (potentially forever), while in a lower-quality implementation the less-preferred code path will probably regress at some point, which is bad if your compositor or app is in the minority that is relying on it. smcv ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] client: Allow send error recovery without an abort
On Mon, 18 Jun 2018 18:41:28 -0700 Lloyd Pique wrote: > Hi Pekka! > > Thank you for the feedback. In the end, I think we have a good basic > agreement about the patch, and after reading your linked bug ( > https://gitlab.freedesktop.org/wayland/wayland/issues/12), which I was > previously unaware of, it sounds like I will be just stripping out the > callback+retry case from my patch. > > I will probably go ahead and add a function to check for the half-full > output buffer, and one you didn't think of to check whether MAX_FDS_OUT is > about to be exceeded by the next call (that count is small enough to > actually be a concern). Sounds good. I'll reply to your other email too. > At this point I think the only real question I have is one of checking the > intent behind the bug you filed. Should the design be that ONLY behavior > is to set a fatal error on the display context, or should there be a public > API to select between the previous/default abort and the detal error? I think the aim should be to remove the abort(). I do not see any reason to leave it in even as an option. > > I'll send out a revised patch after hearing back with that bit of guidance. > > In case it matters, I did also include my responses to your points inline > below. I don't expect to convince that this patch should be used as is, but > perhaps it will give you or someone else some clarify and insight into why > I suggested the change I did. Nice, thank you for that. Some more below. > On Mon, Jun 18, 2018 at 4:58 AM Pekka Paalanen wrote: > > > On Tue, 5 Jun 2018 18:14:54 -0700 > > Lloyd Pique wrote: > > > > > Introduce a new call wl_display_set_error_handler_client(), which allows > > > a client to register a callback function which is invoked if there is an > > > error (possibly transient) while sending messages to the server. > > > > > > This allows a Wayland client that is actually a nested server to try and > > > recover more gracefully from send errors, allowing it one to disconnect > > > and reconnect to the server if necessary. > > > > > > The handler is called with the wl_display and the errno, and is expected > > > to return one of WL_SEND_ERROR_ABORT, WL_SEND_ERROR_FATAL, or > > > WL_SEND_ERROR_RETRY. The core-client code will then respectively abort() > > > the process (the default if no handler is set), set the display context > > > into an error state, or retry the send operation that failed.7 > > > > > > The existing display test is extended to exercise the new function. > > > > Hi Lloyd, > > > > technically this looks like a quality submission, thank you for that. I > > am particularly happy to see all the new cases are added into the test > > suite. I agree that calling wl_abort() is nasty and it would be good to > > have something else. > > > > However, I'm not quite convinced of the introduced complexity here. > > More comments inline. > > If you know you are sending lots of requests, you should be calling > > wl_display_flush() occasionally. It will return -1 if flushing fails to > > let you know you need to wait. The main problem I see with that is when > > to attempt to flush. > > > > We actually do have a call to wl_display_flush() in our larger > implementation, called frequently as part of ensuring everything is > committed to the display. It does currently handle a return of -1, but by > waking up the input event processing thread in case the error is fatal, as > that thread handles disconnecting and reconnecting. > > On the surface, making it detect an EAGAIN and wait kind of sounds > reasonable. But I kind of wonder how often in normal use it would return > EAGAIN, and so would force a wait that wouldn't need to, not when there is > still some space in the core client output buffer still. My reason for > adding the callback and the retry option were specifically to only make the > client block if there really was no other way to avoid an error state. The soft-buffer in libwayland-client is quite small, the kernel socket buffers are usually much larger in bytes. EAGAIN is triggered only when the flush to the kernel fails, so I don't think that would happen needlessly. > That said, I actually am willing to detect EAGAIN on wl_display_flush(), > and explicitly wait then. In trying out this patch, I only ended up > artificially reproducing an EAGAIN error (taking a few seconds to trigger) > when I magnified the request traffic by a factor of about 1000x. That is a > pretty unreasonable magnification even for a nested server, so while it > could happen, I don't think this is actually that important to my project. > Thinking about it fresh, Maybe that is reasonable as a general option for > clients. The one caveat I could think of has to do with MAX_FDS_OUT and it > being only 28. My project does use the linux-dmabuf-unstable-v1 protocol, > and having more than 14 surfaces in a nested server seems like it could > actually happen often. And on second thought, I could see my
Re: Session suspension and restoration protocol
On 2018/6月/19 11:18, Markus Ongyerth wrote: > Hey Roman, > > first a general remark: > I don't see any mention of which state is supposed to be restored. The > workspace (virtual desktops), geometry (session-restore), just client-side > window (the embedded usecase)? > Is this left implementation-defined intentionally? If so, I think it would be > nice to have a note about that. > > While the sleep version of this has a way for a client ot overwrite and > restore itself, I'm missing that functionality on quit/shutdown. > The current state is always compositor triggered on those. While it might > make > sense for e.g. multi-window applications or other fancy things to use a > restoration protocol (at least for window geometry) on their own startup. > It *should* work as is, but I'd like to see it explicitly. > > Some more remarks inline. > > Cheers, > ongy > > > On 2018/6月/18 05:05, Roman Gilg wrote: > > Hi all, > > > > I have worked the past few days on a protocol for client session > > suspension and restoration and I need to get some feedback if I'm > > running in the right direction. I did get some input from Sway and > > GNOME devs at the start on what they would want such a protocol to do > > in general, and I did try to respect that when working on the > > protocol. Main features of the protocol as pasted below are: > > * provides an extensible object creation mechanism to let the > > compositor announce support for a subset of three increasing levels of > > suspension and let the client opt-in to some or all of these supported > > suspension levels > > * these three levels are: > > 1. sleep: allows destroying surfaces and their later restoration > > 2. quit: allows connection closing and restart of the client by the > > compositor via an implementation-independent D-Bus interface > > 3. shutdown: client will get restored after a server shutdown in a > > new Wayland session via the same D-Bus interface. > > * using D-Bus interface only to secure against sandboxed clients > What? Why exactly? When I first read this, I expected that the client is > supposed to use the portal stuff to call out of a sandbox, but reading > through > the actual protocol, I think the only usage of D-Bus is to launch the > applications from the compositor? > Since this needs explicit client support either way, IMO this could be > implmented by adding a required cmdline argument (e.g. > --org-freedsktop-restore=UID) instead. > Parsing .desktop files, and launching applications in a helper shouldn't bee > too much of a burdon. > I know that GNOME/KDE employ D-Bus either way, but others want to avoid it as > much as possible. P.S. I just thought about this ab it more, and something else came to my mind: How is env passed with dbus activation? afaik the session bus does not run in under the compositor, so if we aren't on wayland-0 (which can happen very well in dev situations, and multi seats, and ...) we'd have to tell the client as well. A second concern here would be iplementation cost in the client. Thinking about clients that may have multiple instances, but not shared sessions, e.g. some IDE with two projects opened. iiuc the activation would start one process, which could then either restore, or open as normal, but when another project is openend, the process is already running and has to start another instance. Over just having a single point of entry where it's started with restoration, or without. > > * if the client opts-in to level 2, 3 or both, the compositor might > > try to also restore the client after a client or compositor crash > > * the program flow would be always: > > 1. compositor announces supported suspension levels > > 2. client opts-in to one or several of the supported suspension > > levels by creating protocol objects for the respective levels > > 3. when the compositor wants to suspend the client (for example on > > low memory or shutdown) it sends an event to the respective > > suspension object, which the client can acknowledge or it must > > destroy the object to cancel the suspension. > > So while client and server agree on a subset of usable suspension > > levels, in the end only the compositor activates a suspension. > > > > The protocol is written from scratch, but it shares some similarities > > with Mike's proposed xdg-session-management protocol > > (https://lists.freedesktop.org/archives/wayland-devel/2018-February/037236.html). > > > > In advance thank you for your feedback! > > > > > > > > > > > > Copyright © 2018 Roman Gilg > > > > 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
Re: [PATCH weston 0/4] Preparing for Meson
Hi Pekka, On Mon, 18 Jun 2018 at 15:41, Pekka Paalanen wrote: > these changes are intended to make using Meson easier. Daniel's patches > are v2, mine are v1. > > Daniel Stone (2): > tests: Don't rely on build directory layout > tests: Reshuffle IVI layout tests > > Pekka Paalanen (2): > shared: remove weston_config_get_libexec_dir() > tests: remove WESTON_BUILD_DIR from env Thanks a lot for picking these up; series is: Reviewed-by: Daniel Stone Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] contributing: add review guidelines
Hi Pekka, On Mon, 18 Jun 2018 at 14:43, Pekka Paalanen wrote: > This sets up the standards for patch review, and defines when a patch > can be merged. I believe these are the practises we have been using > already for a long time, now they are just written down explicitly. > > It's not an exhaustive list of criteria and likely cannot ever be, but > it should give a good idea of what level of review we want to have. > > It has been written in general terms, so that we can easily apply the > same text not just to Wayland, but also Weston and other projects as > necessary. > > This addition is not redundant with > https://wayland.freedesktop.org/reviewing.html . > > The web page is a friendly introduction and encouragement for people to > get involved. The guidelines here are more specific and aimed for people > who seek commit rights or maintainership. Thanks a lot for tackling this! Having it written down is fantastic, and I like how you've put it. I have two minor suggestions below: > +- The code is correct and does not ignore corner-cases. In general this is what we aim for, but the presence of 'XXX'/'FIXME' comments in the code shows we don't always live up to that ideal. ;) Maybe this could instead be something like: The code is correct and does not introduce new failures for existing users, nor add new corner-case bugs. When fixing bugs, 'root cause' fixes are preferred rather than hiding symptoms: identify why the issue has occurred, and fix it as close as possible to the source. If it is not practical to completely fix the issue, partial fixes should be accompanied by a comment in proximate code, as well as a new issue opened in GitLab clearly noting known corner cases with a suggestion on how to fix them. > +- The code does what it says in the commit message and changes nothing else. Which leads naturally to something like: If the commit message starts getting too long and addresses multiple points, this may be a sign that the commit should be broken into smaller individual commits. I'm happy to take alternate wording, since what you've written here is far more concise than mine. Anyway, both patches are: Reviewed-by: Daniel Stone Could this later be used as the basis for similar Weston patches? Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Session suspension and restoration protocol
Hey Roman, first a general remark: I don't see any mention of which state is supposed to be restored. The workspace (virtual desktops), geometry (session-restore), just client-side window (the embedded usecase)? Is this left implementation-defined intentionally? If so, I think it would be nice to have a note about that. While the sleep version of this has a way for a client ot overwrite and restore itself, I'm missing that functionality on quit/shutdown. The current state is always compositor triggered on those. While it might make sense for e.g. multi-window applications or other fancy things to use a restoration protocol (at least for window geometry) on their own startup. It *should* work as is, but I'd like to see it explicitly. Some more remarks inline. Cheers, ongy On 2018/6月/18 05:05, Roman Gilg wrote: > Hi all, > > I have worked the past few days on a protocol for client session > suspension and restoration and I need to get some feedback if I'm > running in the right direction. I did get some input from Sway and > GNOME devs at the start on what they would want such a protocol to do > in general, and I did try to respect that when working on the > protocol. Main features of the protocol as pasted below are: > * provides an extensible object creation mechanism to let the > compositor announce support for a subset of three increasing levels of > suspension and let the client opt-in to some or all of these supported > suspension levels > * these three levels are: > 1. sleep: allows destroying surfaces and their later restoration > 2. quit: allows connection closing and restart of the client by the > compositor via an implementation-independent D-Bus interface > 3. shutdown: client will get restored after a server shutdown in a > new Wayland session via the same D-Bus interface. > * using D-Bus interface only to secure against sandboxed clients What? Why exactly? When I first read this, I expected that the client is supposed to use the portal stuff to call out of a sandbox, but reading through the actual protocol, I think the only usage of D-Bus is to launch the applications from the compositor? Since this needs explicit client support either way, IMO this could be implmented by adding a required cmdline argument (e.g. --org-freedsktop-restore=UID) instead. Parsing .desktop files, and launching applications in a helper shouldn't bee too much of a burdon. I know that GNOME/KDE employ D-Bus either way, but others want to avoid it as much as possible. > * if the client opts-in to level 2, 3 or both, the compositor might > try to also restore the client after a client or compositor crash > * the program flow would be always: > 1. compositor announces supported suspension levels > 2. client opts-in to one or several of the supported suspension > levels by creating protocol objects for the respective levels > 3. when the compositor wants to suspend the client (for example on > low memory or shutdown) it sends an event to the respective > suspension object, which the client can acknowledge or it must > destroy the object to cancel the suspension. > So while client and server agree on a subset of usable suspension > levels, in the end only the compositor activates a suspension. > > The protocol is written from scratch, but it shares some similarities > with Mike's proposed xdg-session-management protocol > (https://lists.freedesktop.org/archives/wayland-devel/2018-February/037236.html). > > In advance thank you for your feedback! > > > > > > Copyright © 2018 Roman Gilg > > 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. > > > A protocol for suspending and restoring clients. Multiple modes allow > clients and server to match their common support for different levels of > suspension from suspending and restoring only the
