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

flux-{start,broker}: propagate argument quoting #931

Merged
merged 1 commit into from Dec 23, 2016

Conversation

Projects
None yet
4 participants
@trws
Copy link
Member

trws commented Dec 21, 2016

Kinda random, but after running into this setting up little one-liner tests a lot lately I thought it would be worth fixing. I'm open to adjusting this if it doesn't behave the way others think it should, but it makes one-off testing rather more ergonomic at least for me.

Both flux-start and flux-broker have classically taken the user command
through an argz_stringify, causing all arguments to be flattened into a
single argument and requiring odd double quoting to make many commands
work. This makes flux-start propagate all arguments as-is through to
the next stage. The flux-broker case is a bit more complicated, because
of the shell default, but is currently set up to default to a shell with
no command, use ${shell} -c <arg> when there is only one argument, and
directly invoke ... when there are multiple. Basic result
is that commands like flux start -s 2 flux wreckrun -n 4 sh -c './something_per_rank | grep stuff | ...' do something sensible. Old
behavior caused that to run sh -c 'sh -c ./something_per_rank ...' which
just set the arguments in the inner sh to the rest of the values, losing
them in the command.

@garlick garlick added the review label Dec 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.01%) to 76.343% when pulling db54830 on trws:argument-quoting into a1fe845 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 21, 2016

Current coverage is 76.07% (diff: 88.13%)

Merging #931 into master will decrease coverage by <.01%

@@             master       #931   diff @@
==========================================
  Files           149        149          
  Lines         25936      25951    +15   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19733      19742     +9   
- Misses         6203       6209     +6   
  Partials          0          0          
Diff Coverage File Path
•••••• 60% src/common/libsubprocess/subprocess.c
••••••••• 95% src/cmd/flux-start.c
•••••••••• 100% src/broker/runlevel.c
•••••••••• 100% src/broker/broker.c

Powered by Codecov. Last update 24973c8...011e2b2

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 21, 2016

Sounds good! I can't think of any reason this would be anything but an improvement and the fact that it hasn't broken any test cases is a good data point. Let me build this and poke at it a bit just to see if I can make it do anything that I would consider unexpected.

I was going to criticize the TRYP/TRYI macros in runlevel.c since attempts to use the preprocessor to turn C into something else generally make C code harder to follow when looking at it cold, but here, when I look at the whole file and not just the diff, I think they actually do improve clarity so I will restrain myself :-)

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 21, 2016

Honestly I wouldn't mind reworking the TRYI/TRYP bit into something else, I just had to rework that big master if condition, and that was the first thing that came to mind to make the conditions separable without a veritable forest of if(...) goto error; statements. It's actually probably because I've been poking at rust lately, which uses a macro that turns try!(...) into match(... ) { Error(e) => return e, Result(r) => r} to do error propagation in destructuring. I was actually tempted to add them to macros.h as something like the following, but decided to leave it local for now as I wasn't sure what you and @grondo would think.

#define TRY_I(EXPR, LABEL) if ((EXPR) < 0) goto LABEL
#define TRY_P(EXPR, LABEL) if ((EXPR) == NULL) goto LABEL

Really, if we can say we're GNU source this would be more powerful, which we are because of argz and certain attributes we use anyway, but still it's a question.

#define TRY_I(EXPR, LABEL) ({ \
            typeof(EXPR) __ret = (EXPR); \
            if (__ret < 0) goto LABEL;\
            __ret; \
            })
#define TRY_P(EXPR, LABEL) ({ \
            typeof(EXPR) __ret = (EXPR); \
            if (__ret == NULL) goto LABEL;\
            __ret; \
            })

Lets this kind of fun stuff work: p = TRY_P (subprocess_create (r->sm), error);, could embed a return the same way for simple propagation, but I'm not sure that's much of a win...

Sorry for the rambling, I'm an abstractions guy at heart I guess. =P

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 21, 2016

"In C there is no try, there is only goto" --yoda

No seriously, this would take some convincing. I hope I'm not being curmudgeonly, though it's not just me - example: LK coding style has this to say on macros:

Things to avoid when using macros:

1) macros that affect control flow:

.. code-block:: c

	#define FOO(x)					\
		do {					\
			if (blah(x) < 0)		\
				return -EBUGGERED;	\
		} while (0)

is a **very** bad idea.  It looks like a function call but exits the ``calling``
function; don't break the internal parsers of those who will read the code.

This last clause about people's internal parsers is the thing to watch out for IMHO.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 21, 2016

No worries, and no pressure really. As I said, I wasn't sure what the reaction would be, it's very clearly a preference question and if it makes anyone uncomfortable it's best to just not do it. For me things like that are a good way to make sure I don't accidentally screw up the boilerplate to end up with something like the, once famous, Apple SSL fail.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 21, 2016

Anyway, aside from poking at it, would you like me to factor out the TRY* macros @garlick?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 21, 2016

Just did a little poking and this seems good to me. Thanks!

Yes I think probably the TRY* macros should be taken out, sorry.

Also I think the argz_cmd, argz_cmd_len naming in flux-start adds clarity. Maybe do the same with the runlevel args in the broker?

Otherwise looks good to me.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 22, 2016

Ok, I'll make those adjustments. I originally left the argument names alone because you can pass a single arg as a regular string, but since the length has to include the null, I think that's a good point, it'll help make it clear to callers.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 22, 2016

By the way, _GNU_SOURCE should already be defined in config.h via configure.ac.

@trws trws force-pushed the trws:argument-quoting branch 2 times, most recently from 6a847c4 to c7aac0a Dec 22, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 22, 2016

You're quite right that it is @garlick, but somehow despite that unistd.h wasn't declaring environ.

Either way, this one should be cleaned up and working. I decided the header issues were outside the scope of this PR because it was becoming a pretty deep rabbit hole, and just put runlevel.h last again so it stops breaking.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 22, 2016

Coverage Status

Coverage increased (+0.03%) to 76.358% when pulling c7aac0a on trws:argument-quoting into a1fe845 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 22, 2016

Coverage Status

Coverage decreased (-0.003%) to 76.328% when pulling c7aac0a on trws:argument-quoting into a1fe845 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 22, 2016

Thanks for those changes and sorry about the header issues. Want to squash this down to one commit again and rebase on current master? Then I'll merge after I get back online.

flux-{start,broker}: propagate argument quoting
Both flux-start and flux-broker have classically taken the user command
through an argz_stringify, causing all arguments to be flattened into a
single argument and requiring odd double quoting to make many commands
work.  This makes flux-start propagate all arguments as-is through to
the next stage.  The flux-broker case is a bit more complicated, because
of the shell default, but is currently set up to default to a shell with
no command, use `${shell} -c <arg>` when there is only one argument, and
directly invoke <arg1>...<argn> when there are multiple.  Basic result
is that commands like `flux start -s 2 flux wreckrun -n 4 sh -c
'./something_per_rank | grep stuff | ...'` do something sensible.  Old
behavior caused that to run sh -c 'sh -c ./something_per_rank ...' which
just set the arguments in the inner sh to the rest of the values, losing
them in the command.

Extra include adjustments are to appease my linter, which noted that
runlevel.h cannot be included without including various other headers
first (size_t and others undefined, etc.).  Note that this does not
completely repair runlevel.h, if it is moved from last place in
runlevel.c errors will result including environ being an undefined
symbol somehow.

@trws trws force-pushed the trws:argument-quoting branch from c7aac0a to 011e2b2 Dec 23, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 23, 2016

No problem. I'm sorry for the noise on the headers, it looked like it was just a little tweak to clean up simple stuff like size_t used without include, but there's some odd interaction between runlevel.h and at least one other that eluded me. The environ thing especially has me flummoxed, I can't think of a way that it could be undefined in the file with unistd.h included correctly, which it is. There has to be something odd going on there...

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 23, 2016

Coverage Status

Coverage decreased (-0.01%) to 76.358% when pulling 011e2b2 on trws:argument-quoting into 24973c8 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 23, 2016

Thanks!

@garlick garlick merged commit b26fbff into flux-framework:master Dec 23, 2016

4 checks passed

codecov/patch 88.13% of diff hit (target 76.08%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +12.05% compared to 24973c8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 76.358%
Details

@garlick garlick removed the review label Dec 23, 2016

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.