Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a serving of gourmet, artisan prepared insect appetizers #2151

Merged
merged 8 commits into from May 8, 2019

Conversation

@garlick
Copy link
Member

commented May 7, 2019

Here is a batch of minor bug fixes peeled off of various in-progress PR's I've been working on, covering:

  • The hardwired /usr/bin/rsh encountered by IBM
  • Incorrect error reporting when the initial program dies due to a signal
  • Poor NULL argument handling in the flux_respond*() series of functions
  • Noise building python bindings
  • Sadness if aux destructors call aux_set(), aux_get() (not actually seen in nature)
  • Refactor hand-built PROTO codec in message.c for clarity/changeability
garlick added 3 commits May 2, 2019
Problem: when run level 2 terminates due to a signal,
128 is added to the signal number before it is passed to
strsignal() for decoding.

Decode the signal before adding 128 to it to make the exit code.
Problem: aux_destroy() is not safe against destructors
calling aux_set() or aux_get(), because destructor
is called with the item being destroyed still linked
into the list.

Specifically, aux_set() could result in the new item
being lost, and aux_get() could allow access to the
item that is being destroyed.

Modify aux_destroy() to safely unlink an item
before calling its destructor.

Add unit tests to cover these situations.
Problem: old def of flux_msg_t is there, commented out,
in the header.  In the code, definition is within the
begin/end manual codec comments.

Drop the commented out definition in the header, and
relocate the definition in the code.
@@ -107,6 +107,10 @@ else
[deepbind is unsupported with asan, musl and so-forth])

This comment has been minimized.

Copy link
@chu11

chu11 May 7, 2019

Contributor

mismatch in comments, you say HAVE_SSH, but it's PATH_SSH.

return -1;
break;
case FLUX_MSGTYPE_KEEPALIVE:
if (proto_set_bigint (data, len, 0) < 0)
if (proto_set_u32 (data, len, PROTO_IND_AUX1, 0) < 0)

This comment has been minimized.

Copy link
@chu11

chu11 May 7, 2019

Contributor

shouldn't do the PROTO_IND_AUX1 setting? You've already got IND_STATUS and IND_ERRNUM below this.

This comment has been minimized.

Copy link
@garlick

garlick May 7, 2019

Author Member

Nice catch! That was a rebase fail from a protocol change I was working on.

@garlick garlick changed the title small bug fix assortment a serving of gourmet, artisan prepared insect appetizers May 7, 2019
garlick added 4 commits May 7, 2019
Problem: flux_respond*() segfaults if flux_t
handle or response arguments are NULL.

Add code to check for NULL and fail with EINVAL.

Add unit test coverage for these cases.
Problem: There is duplicate code for accessing u32 values in
PROTO block, and the assignment of fields depending on message
type is confusing.

Combine rolemask, userid, bigint and bigint2 into a "u32 array",
with common accessors.

Add convenience macros mapping field names to positions in the array.

This will make changing PROTO easier in the future.
Problem: build of python bindings emits warnings like this:

_core.c:2328:3: warning: missing initializer for field ‘reserved1’
    of ‘struct _cffi_externpy_s’ [-Wmissing-field-initializers]

This is generated code, so suppress the warning by changing
-Wno-error=missing-field-initializers to
-Wno-missing-field-initializers

Fixes #2147
Problem: If the default path to ssh cannot be found
by the ssh connector, flux_open() only gets ENOENT
back in errno, which isn't much to go on.

Emit the failed path and a hint about overriding by
setting FLUX_SSH on stderr if the popen() of ssh
fails.

Fixes #2140
@garlick garlick force-pushed the garlick:misc_fixes branch from da6d740 to b58c561 May 8, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Forced a push that addressed those two comments.

@grondo

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

LGTM! My only comment is that the docs for AC_PATH_PROGS strongly suggests that the argument to AC_PATH_PROG and related macros be declared precious

The other benefit do declaring SSH with AC_ARG_VAR() would be that a user could override the value during configuration with ./configure SSH=/path/to/ssh,

e.g.

diff --git a/configure.ac b/configure.ac
index 7644a1b0d..78abd1ff5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -108,6 +108,7 @@ else
 fi
 
 # N.B. /usr/bin/rsh is a symlink to preferred remote shell on some systems
+AC_ARG_VAR(SSH, [The path to preferred remote shell])
 AC_PATH_PROGS(SSH, [rsh ssh], [/usr/bin/rsh])
 AC_DEFINE_UNQUOTED([PATH_SSH], "$SSH",
           [Define remote shell program to be used by the ssh:// connector])

and then

$ ./configure --help | grep SSH
  SSH         The path to preferred remote shell
$ ./configure SSH=/usr/bin/ssh
...
$ grep SSH config/config.h
#define PATH_SSH "/usr/bin/ssh"
@garlick

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Ah, I did not know all that. Thanks! Forcing a push that adds that AC_ARG_VAR line.

@codecov-io

This comment has been minimized.

Copy link

commented May 8, 2019

Codecov Report

Merging #2151 into master will increase coverage by 0.04%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master    #2151      +/-   ##
==========================================
+ Coverage   80.38%   80.43%   +0.04%     
==========================================
  Files         200      200              
  Lines       31769    31757      -12     
==========================================
+ Hits        25539    25544       +5     
+ Misses       6230     6213      -17
Impacted Files Coverage Δ
src/common/libutil/aux.c 90.74% <100%> (ø) ⬆️
src/common/libflux/message.c 80.83% <100%> (-0.69%) ⬇️
src/broker/runlevel.c 81% <100%> (ø) ⬆️
src/connectors/ssh/ssh.c 85.9% <80%> (+0.19%) ⬆️
src/common/libflux/response.c 87.86% <91.66%> (+8.61%) ⬆️
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️
src/common/libutil/veb.c 98.85% <0%> (+0.57%) ⬆️
src/modules/barrier/barrier.c 80.53% <0%> (+2.01%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

If you want to squash 8e7472e I'll merge this one.

Problem: default remote shell is hardwired in the ssh
connector source to "/usr/bin/rsh".  On systems that don't
install "mrsh" (TOSS) and don't use debian alternatives,
/usr/bin/rsh is not a symlink to the preferred remote shell.

Have autoconf perform the search for a suitable remote
shell program, and put the results in PATH_SSH, then
use that in the ssh connector source.

Configure searches in the current path for "rsh", then "ssh",
then falls back to /usr/bin/rsh as before if none found.
@garlick garlick force-pushed the garlick:misc_fixes branch from 8e7472e to 51e31cf May 8, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Oops, thought I had! OK, just did.

@grondo grondo merged commit 9bd0d55 into flux-framework:master May 8, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Thanks!

@garlick garlick deleted the garlick:misc_fixes branch May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.