* [PATCH 1/2] staging: Reduce verbosity of log messages
@ 2017-09-22 16:41 Richard Purdie
2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie
2017-09-22 17:00 ` ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more Patchwork
0 siblings, 2 replies; 6+ messages in thread
From: Richard Purdie @ 2017-09-22 16:41 UTC (permalink / raw)
To: openembedded-core
The staging changes were very verbose in their logging and whilst this is useful
when staging issues occur, those thankfully seem rare now and we can tune down
the logging to a sane level. This improves the readability of error messages from
functions that fail.
The code is still verbose when its replacing things in the sysroot.
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
meta/classes/staging.bbclass | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
index 6185aef..ea831e0 100644
--- a/meta/classes/staging.bbclass
+++ b/meta/classes/staging.bbclass
@@ -373,7 +373,8 @@ python extend_recipe_sysroot() {
msgbuf.append("Following dependency on %s" % setscenedeps[datadep][0])
next = new
- bb.note("\n".join(msgbuf))
+ # This logging is too verbose for day to day use sadly
+ #bb.debug(2, "\n".join(msgbuf))
depdir = recipesysrootnative + "/installeddeps"
bb.utils.mkdirhier(depdir)
@@ -442,6 +443,8 @@ python extend_recipe_sysroot() {
os.unlink(fl)
os.unlink(fl + ".complete")
+ msg_exists = []
+ msg_adding = []
for dep in configuredeps:
c = setscenedeps[dep][0]
if c not in installed:
@@ -452,7 +455,7 @@ python extend_recipe_sysroot() {
if os.path.exists(depdir + "/" + c):
lnk = os.readlink(depdir + "/" + c)
if lnk == c + "." + taskhash and os.path.exists(depdir + "/" + c + ".complete"):
- bb.note("%s exists in sysroot, skipping" % c)
+ msg_exists.append(c)
continue
else:
bb.note("%s exists in sysroot, but is stale (%s vs. %s), removing." % (c, lnk, c + "." + taskhash))
@@ -463,6 +466,8 @@ python extend_recipe_sysroot() {
elif os.path.lexists(depdir + "/" + c):
os.unlink(depdir + "/" + c)
+ msg_adding.append(c)
+
os.symlink(c + "." + taskhash, depdir + "/" + c)
d2 = d
@@ -559,6 +564,9 @@ python extend_recipe_sysroot() {
continue
staging_copyfile(l, targetdir, dest, postinsts, seendirs)
+ bb.note("Installed into sysroot: %s" % str(msg_adding))
+ bb.note("Skipping as already exists in sysroot: %s" % str(msg_exists))
+
for f in fixme:
if f == '':
staging_processfixme(fixme[f], recipesysroot, recipesysroot, recipesysrootnative, d)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] pseudo: Add fastop reply fix
2017-09-22 16:41 [PATCH 1/2] staging: Reduce verbosity of log messages Richard Purdie
@ 2017-09-22 16:41 ` Richard Purdie
2017-09-22 16:52 ` Seebs
2017-09-22 17:00 ` ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more Patchwork
1 sibling, 1 reply; 6+ messages in thread
From: Richard Purdie @ 2017-09-22 16:41 UTC (permalink / raw)
To: openembedded-core
This changes the pseudo FASTOP functionality so that a reply to the
operation is required. This means we then cannot lose data if a connection
is closed. This in turn stops corruption if we run out of file handles
and have to close connections.
This tweaks the connection closure patch to update the comment there which
is now outdated.
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
.../pseudo/files/fastopreply.patch | 74 ++++++++++++++++++++++
.../pseudo/files/toomanyfiles.patch | 7 +-
meta/recipes-devtools/pseudo/pseudo_1.8.2.bb | 1 +
3 files changed, 77 insertions(+), 5 deletions(-)
create mode 100644 meta/recipes-devtools/pseudo/files/fastopreply.patch
diff --git a/meta/recipes-devtools/pseudo/files/fastopreply.patch b/meta/recipes-devtools/pseudo/files/fastopreply.patch
new file mode 100644
index 0000000..c8e7b08
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/files/fastopreply.patch
@@ -0,0 +1,74 @@
+Ensure FASTOP messages get an ACK reply so that the client can be sure the server
+recieved them. This means if connections are terminated, data isn't lost.
+
+RP 2017/9/22
+
+Index: pseudo-1.8.2/pseudo_client.c
+===================================================================
+--- pseudo-1.8.2.orig/pseudo_client.c
++++ pseudo-1.8.2/pseudo_client.c
+@@ -1331,21 +1331,19 @@ pseudo_client_request(pseudo_msg_t *msg,
+ * indicating a successful send.
+ */
+ pseudo_debug(PDBGF_CLIENT | PDBGF_VERBOSE, "sent!\n");
+- if (msg->type != PSEUDO_MSG_FASTOP) {
+- response = pseudo_msg_receive(connect_fd);
+- if (!response) {
+- pseudo_debug(PDBGF_CLIENT, "expected response did not occur; retrying\n");
++ response = pseudo_msg_receive(connect_fd);
++ if (!response) {
++ pseudo_debug(PDBGF_CLIENT, "expected response did not occur; retrying\n");
++ } else {
++ if (response->type != PSEUDO_MSG_ACK) {
++ pseudo_debug(PDBGF_CLIENT, "got non-ack response %d\n", response->type);
++ return 0;
++ } else if (msg->type != PSEUDO_MSG_FASTOP) {
++ pseudo_debug(PDBGF_CLIENT | PDBGF_VERBOSE, "got response type %d\n", response->type);
++ return response;
+ } else {
+- if (response->type != PSEUDO_MSG_ACK) {
+- pseudo_debug(PDBGF_CLIENT, "got non-ack response %d\n", response->type);
+- return 0;
+- } else {
+- pseudo_debug(PDBGF_CLIENT | PDBGF_VERBOSE, "got response type %d\n", response->type);
+- return response;
+- }
++ return 0;
+ }
+- } else {
+- return 0;
+ }
+ }
+ pseudo_diag("pseudo: server connection persistently failed, aborting.\n");
+Index: pseudo-1.8.2/pseudo_server.c
+===================================================================
+--- pseudo-1.8.2.orig/pseudo_server.c
++++ pseudo-1.8.2/pseudo_server.c
+@@ -463,6 +463,11 @@ close_client(int client) {
+ --highest_client;
+ }
+
++static pseudo_msg_t server_fastop_reply = {
++ .type = PSEUDO_MSG_ACK,
++ .op = OP_NONE,
++};
++
+ /* Actually process a request.
+ */
+ static int
+@@ -515,8 +520,14 @@ serve_client(int i) {
+ * pseudo_server_response.
+ */
+ if (in->type != PSEUDO_MSG_SHUTDOWN) {
+- if (in->type == PSEUDO_MSG_FASTOP)
++ if (in->type == PSEUDO_MSG_FASTOP) {
+ send_response = 0;
++ /* For fastops we reply now to say we got the data */
++ if ((rc = pseudo_msg_send(clients[i].fd, &server_fastop_reply, 0, NULL)) != 0) {
++ pseudo_debug(PDBGF_SERVER, "failed to send fastop ack to client %d [%d]: %d (%s)\n",
++ i, (int) clients[i].pid, rc, strerror(errno));
++ }
++ }
+ /* most messages don't need these, but xattr may */
+ response_path = 0;
+ response_pathlen = -1;
diff --git a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
index 7319ab2..d014ad5 100644
--- a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
+++ b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
@@ -31,15 +31,12 @@ Index: pseudo-1.8.2/pseudo_server.c
pseudo_debug(PDBGF_SERVER, "server loop started.\n");
if (listen_fd < 0) {
-@@ -663,10 +665,18 @@ pseudo_server_loop(void) {
+@@ -663,10 +665,15 @@ pseudo_server_loop(void) {
message_time.tv_usec -= 1000000;
++message_time.tv_sec;
}
+ } else if (hitmaxfiles) {
-+ /* In theory there is a potential race here where if we close a client,
-+ it may have sent us a fastop message which we don't act upon.
-+ If we don't close a filehandle we'll loop indefinitely thought.
-+ Only close one per loop iteration in the interests of caution */
++ /* Only close one per loop iteration in the interests of caution */
+ close_client(i);
+ hitmaxfiles = 0;
}
diff --git a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
index 81853e9..73ef572 100644
--- a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
@@ -7,6 +7,7 @@ SRC_URI = "http://downloads.yoctoproject.org/releases/pseudo/${BPN}-${PV}.tar.bz
file://moreretries.patch \
file://efe0be279901006f939cd357ccee47b651c786da.patch \
file://b6b68db896f9963558334aff7fca61adde4ec10f.patch \
+ file://fastopreply.patch \
file://toomanyfiles.patch \
file://0001-Use-epoll-API-on-Linux.patch \
"
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] pseudo: Add fastop reply fix
2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie
@ 2017-09-22 16:52 ` Seebs
2017-09-22 17:12 ` Alexander Kanavin
0 siblings, 1 reply; 6+ messages in thread
From: Seebs @ 2017-09-22 16:52 UTC (permalink / raw)
To: Richard Purdie; +Cc: openembedded-core
On Fri, 22 Sep 2017 17:41:17 +0100
Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> This changes the pseudo FASTOP functionality so that a reply to the
> operation is required. This means we then cannot lose data if a
> connection is closed. This in turn stops corruption if we run out of
> file handles and have to close connections.
>
> This tweaks the connection closure patch to update the comment there
> which is now outdated.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
This looks reasonable to me. I did some testing with a very similar
patch and concluded that, while it slowed performance slightly, it
didn't slow it nearly as much as the pre-fastop behavior did.
-s
^ permalink raw reply [flat|nested] 6+ messages in thread
* ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more
2017-09-22 16:41 [PATCH 1/2] staging: Reduce verbosity of log messages Richard Purdie
2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie
@ 2017-09-22 17:00 ` Patchwork
1 sibling, 0 replies; 6+ messages in thread
From: Patchwork @ 2017-09-22 17:00 UTC (permalink / raw)
To: Richard Purdie; +Cc: openembedded-core
== Series Details ==
Series: "staging: Reduce verbosity of l..." and 1 more
Revision: 1
URL : https://patchwork.openembedded.org/series/9077/
State : failure
== Summary ==
Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:
* Issue Added patch file is missing Upstream-Status in the header [test_upstream_status_presence]
Suggested fix Add Upstream-Status: <status> to the header of meta/recipes-devtools/pseudo/files/fastopreply.patch (possible values: Pending, Submitted, Accepted, Backport, Denied, Inappropriate)
* Issue Series does not apply on top of target branch [test_series_merge_on_head]
Suggested fix Rebase your series on top of targeted branch
Targeted branch master (currently at acc5036a6b)
* Issue A patch file has been added, but does not have a Signed-off-by tag [test_signed_off_by_presence]
Suggested fix Sign off the added patch file (meta/recipes-devtools/pseudo/files/fastopreply.patch)
If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).
---
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] pseudo: Add fastop reply fix
2017-09-22 16:52 ` Seebs
@ 2017-09-22 17:12 ` Alexander Kanavin
2017-09-22 19:19 ` Otavio Salvador
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Kanavin @ 2017-09-22 17:12 UTC (permalink / raw)
To: Seebs, Richard Purdie; +Cc: openembedded-core
On 09/22/2017 07:52 PM, Seebs wrote:
>> This changes the pseudo FASTOP functionality so that a reply to the
>> operation is required. This means we then cannot lose data if a
>> connection is closed. This in turn stops corruption if we run out of
>> file handles and have to close connections.
>>
>> This tweaks the connection closure patch to update the comment there
>> which is now outdated.
>>
>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>
> This looks reasonable to me. I did some testing with a very similar
> patch and concluded that, while it slowed performance slightly, it
> didn't slow it nearly as much as the pre-fastop behavior did.
Also, the issue this patch is fixing is not theoretical. Setting max fd
limit to a low value (192) is causing errors on my machine:
dpkg-deb: error: maintainer script 'postinst' has bad permissions 644
(must be >=0555 and <=0775)
and with the patch they seem to go away.
Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] pseudo: Add fastop reply fix
2017-09-22 17:12 ` Alexander Kanavin
@ 2017-09-22 19:19 ` Otavio Salvador
0 siblings, 0 replies; 6+ messages in thread
From: Otavio Salvador @ 2017-09-22 19:19 UTC (permalink / raw)
To: Alexander Kanavin; +Cc: Patches and discussions about the oe-core layer
Richard,
On Fri, Sep 22, 2017 at 2:12 PM, Alexander Kanavin
<alexander.kanavin@linux.intel.com> wrote:
> On 09/22/2017 07:52 PM, Seebs wrote:
>>>
>>> This changes the pseudo FASTOP functionality so that a reply to the
>>> operation is required. This means we then cannot lose data if a
>>> connection is closed. This in turn stops corruption if we run out of
>>> file handles and have to close connections.
>>>
>>> This tweaks the connection closure patch to update the comment there
>>> which is now outdated.
>>>
>>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>>
>>
>> This looks reasonable to me. I did some testing with a very similar
>> patch and concluded that, while it slowed performance slightly, it
>> didn't slow it nearly as much as the pre-fastop behavior did.
>
> Also, the issue this patch is fixing is not theoretical. Setting max fd
> limit to a low value (192) is causing errors on my machine:
>
> dpkg-deb: error: maintainer script 'postinst' has bad permissions 644 (must
> be >=0555 and <=0775)
>
> and with the patch they seem to go away.
When queueing it, please make sure to add the proper patch headers.
--
Otavio Salvador O.S. Systems
http://www.ossystems.com.br http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-22 19:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-22 16:41 [PATCH 1/2] staging: Reduce verbosity of log messages Richard Purdie
2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie
2017-09-22 16:52 ` Seebs
2017-09-22 17:12 ` Alexander Kanavin
2017-09-22 19:19 ` Otavio Salvador
2017-09-22 17:00 ` ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox