[Exec] Properly split command & pipe support #6819

cpiber · 2020-01-12T11:16:14Z

Implemented @@ manual split (@see https://www.openhab.org/docs/configuration/actions.html#exec-actions)
Pass to shell if not manually split (detect shell via https://stackoverflow.com/a/31547504/7508309, permission by author)
Fixes [exec] Cannot use quoted spaces in commands #6729
Added benefit: Pass to shell allows for pipes

Signed-off-by: Constantin Piber cp.piber@gmail.com

@see

- Implemented @@ manual split (@see https://www.openhab.org/docs/configuration/actions.html#exec-actions) - Pass to shell if not manually split (detect shell via https://stackoverflow.com/a/31547504/7508309, permission by author) Fixes #6729 Added benefit: Pass to shell allows for pipes Signed-off-by: Constantin Piber <cp.piber@gmail.com>

TravisBuddy · 2020-01-12T11:20:52Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

cpiber · 2020-01-12T11:49:45Z

Similar to #6815, but allows for pipes and doesn't change output buffer functionality

9037568

One major concern with this is it will break anyone who is already passing bash or cmd as the command to be executed. Might still be worth doing, but we may need additional input (@J-N-K or @kaikreuzer ?).

Some fixes will be needed, in any event.

...penhab.binding.exec/src/main/java/org/openhab/binding/exec/internal/handler/ExecHandler.java

@9037568

Changes as suggested by @9037568 Signed-off-by: Constantin Piber <cp.piber@gmail.com>

cpiber

@9037568 See comments and new commit

TravisBuddy · 2020-01-12T15:04:29Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

TravisBuddy · 2020-01-12T15:09:11Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Rossko57 · 2020-01-13T17:59:18Z

Comment only; please don't forget us Windows users. (I have no idea if any this has an impact there.)

cpiber · 2020-01-13T18:05:44Z

Comment only; please don't forget us Windows users. (I have no idea if any this has an impact there.)

Sorry, new to this.

Tested on Linux with: echo "test space" | grep " ", output: test space (correct)
Tested on Windows: echo "1" | echo "2", output: 2 (correct, need a command to test spaces as well, should work though)
So, on all OSes, the only change should be that quotes are now respected (and pipes enabled), so no breaking changes.

9037568

Addressed latest changes with commentary.

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

TravisBuddy · 2020-01-16T06:26:16Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

9037568

Since I haven't seen feedback from TPTB, I guess I'll have to ask you to fix the breakage issue, too.
In order to not break existing setups, each of the conditions starting at line 198 needs to check if the received command already starts with one of the shell commands, and if so, not prepend the shell command to the received command.

Please also eliminate the remaining occurrences of commandLine.toString().

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

TravisBuddy · 2020-01-21T08:03:38Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

9037568

I think this should be the last set of changes needed...

...penhab.binding.exec/src/main/java/org/openhab/binding/exec/internal/handler/ExecHandler.java

only invoke shell if the first WORD is a shell-program, not just the beginning of the string (ex. shout shouldn't match) Signed-off-by: Constantin Piber <cp.piber@gmail.com>

TravisBuddy · 2020-01-22T06:41:34Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

9037568

LGTM. Thanks for this work, @cpiber !

J-N-K · 2020-01-23T17:08:23Z

...penhab.binding.exec/src/main/java/org/openhab/binding/exec/internal/handler/ExecHandler.java

+                } else {
+                    // Invoke shell with 'c' option and pass string
+                    logger.debug("Passing to shell for parsing command.");
+                    if (getOperatingSystemType() == OS.WINDOWS) {


I have a question here: Why don't you get the operating system type once and then use the local variable in a switch-statement. Are your sure that everything other than OS.WINDOWS and OS.UNKNOWN is a supported *nix? And please add the unsupported OS-name to the warn-message logger.warn("OS '{}' not supported, please split commands manually!", getOperatingSystemName());).

Edit: I found it below. But the if-else is not very intuitive here. Please use a switch-statement.

J-N-K · 2020-01-23T17:12:44Z

...penhab.binding.exec/src/main/java/org/openhab/binding/exec/internal/handler/ExecHandler.java


                Process proc = null;
                try {
-                    proc = rt.exec(commandLine.toString());
+                    proc = rt.exec(cmdArray);
                } catch (Exception e) {


Is it necessary to catch Exception here? According to the docs there is a limited number of exceptions, I think we can omit the NPE if we make sure that none of the commands is null (which should be done anyway).

J-N-K · 2020-01-23T17:13:35Z

...penhab.binding.exec/src/main/java/org/openhab/binding/exec/internal/handler/ExecHandler.java

@@ -193,7 +245,7 @@ public void run() {
                    isr.close();
                } catch (IOException e) {
                    logger.error("An exception occurred while reading the stdout when executing '{}' : '{}'",


And this should be warn, also below. error is reserved for the framework of occasions where the stability of the system may be affected.

J-N-K · 2020-01-23T17:15:31Z

...penhab.binding.exec/src/main/java/org/openhab/binding/exec/internal/handler/ExecHandler.java

+        return System.getProperty("os.name");
+    }
+
+}


Please run mvn spotless:apply after the code issues are done.

What exactly does that do?

Okay so it runs some more tests, I get
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:1.24.3:apply (default-cli) on project org.openhab.binding.feed.tests: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:1.24.3:apply failed: Overlapping text edits -> [Help 1]
What do I do? Can't find any info on that.
Also doesn't seem to by because of my changes (I didn't touch feed binding, at least I don't think)

Reset the changes in other bindings and use mvn spotless:apply -pl bundles\org.openhab.binding.exec to only re-format the exec binding.

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

TravisBuddy · 2020-01-25T17:32:25Z

Travis tests were successful

Hey @cpiber,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com> Signed-off-by: leluna <hengrui.jiang@googlemail.com>

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com> Signed-off-by: Hans-Reiner Hoffmann <hans-reiner.hoffmann@gmx.de>

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

elordude · 2020-06-23T18:56:41Z

I used to have a exec thing to track CPU/MOBO temperatures, it worked fine for more than a year until the recent updates.

I have tired multiple combinations for the command (below) and none of the work, i have updated the misc/exec.whitelist every time, the error I get is shown below.
/bin/cat /sys/class/hwmon/hwmon2/temp3_input
cat /sys/class/hwmon/hwmon2/temp3_input
sudo /bin/cat /sys/class/hwmon/hwmon2/temp3_input

Thing exec:command:core1temp "System" @ "Exec" [ command="/bin/cat /sys/class/hwmon/hwmon2/temp3_input", transform="JS(milli.js)", interval=10, autorun=true ]

2020-06-23 11:55:47.099 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception:
java.lang.NullPointerException: null
at org.openhab.transform.javascript.internal.JavaScriptEngineManager.getScript(JavaScriptEngineManager.java:68) ~[?:?]
at org.openhab.transform.javascript.internal.JavaScriptTransformationService.transform(JavaScriptTransformationService.java:74) ~[?:?]
at org.openhab.binding.exec.internal.handler.ExecHandler.transformResponse(ExecHandler.java:314) ~[?:?]
at org.openhab.binding.exec.internal.handler.ExecHandler.execute(ExecHandler.java:293) ~[?:?]
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) ~[?:1.8.0_252]
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308) ~[?:1.8.0_252]
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180) ~[?:1.8.0_252]
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294) ~[?:1.8.0_252]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_252]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_252]
at java.lang.Thread.run(Thread.java:748) [?:1.8.0_252]

Rossko57 · 2020-06-23T21:37:53Z

Not much point opening a months old pull request, just because it mentions exec.

You appear to have run into the javascript issue described here -
#7880

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com> Signed-off-by: Daan Meijer <daan@studioseptember.nl>

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

cpiber requested a review from kgoderis as a code owner January 12, 2020 11:16

9037568 mentioned this pull request Jan 12, 2020

[Exec] Add the ability to specify an array of arguments via "@@" #6815

Closed

9037568 requested changes Jan 12, 2020

View reviewed changes

Incorporate suggested changes

4827bc8

Changes as suggested by @9037568 Signed-off-by: Constantin Piber <cp.piber@gmail.com>

cpiber commented Jan 12, 2020

View reviewed changes

Remove remaining new Object[] {...}

51498e1

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

Hilbrand added the enhancement An enhancement or new feature for an existing add-on label Jan 13, 2020

Rossko57 mentioned this pull request Jan 13, 2020

[exec] Escapes single quotes not passing #6269

Closed

9037568 requested changes Jan 16, 2020

View reviewed changes

9037568 added the awaiting feedback Awaiting feedback from the pull request author label Jan 16, 2020

cpiber added 2 commits January 16, 2020 07:17

Fixed logging levels error to warn per standard

518f068

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

Removed redundant .toString() and log

d2767d7

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

9037568 requested changes Jan 18, 2020

View reviewed changes

Commands starting with any shell split by spaces

4b9a043

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

9037568 removed the awaiting feedback Awaiting feedback from the pull request author label Jan 21, 2020

9037568 requested changes Jan 22, 2020

View reviewed changes

9037568 added the awaiting feedback Awaiting feedback from the pull request author label Jan 22, 2020

Remove redundant checks, better shell checks

44cc868

only invoke shell if the first WORD is a shell-program, not just the beginning of the string (ex. shout shouldn't match) Signed-off-by: Constantin Piber <cp.piber@gmail.com>

9037568 approved these changes Jan 22, 2020

View reviewed changes

9037568 removed the awaiting feedback Awaiting feedback from the pull request author label Jan 22, 2020

J-N-K requested changes Jan 23, 2020

View reviewed changes

Cosmetic changes, cleanup, spotless

f1dad13

Signed-off-by: Constantin Piber <cp.piber@gmail.com>

J-N-K approved these changes Jan 25, 2020

View reviewed changes

J-N-K merged commit fecfacb into openhab:2.5.x Jan 25, 2020

andibraeu pushed a commit to andibraeu/openhab-addons that referenced this pull request Feb 17, 2020

[Exec] Properly split command & pipe support (openhab#6819)

b8c9ddd

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

9037568 mentioned this pull request Feb 19, 2020

[exec] Fix missing break in switch #7034

Closed

kaikreuzer added this to the 2.5.2 milestone Feb 19, 2020

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request May 29, 2020

[Exec] Properly split command & pipe support (openhab#6819)

e13f224

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[Exec] Properly split command & pipe support (openhab#6819)

4449ca6

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[Exec] Properly split command & pipe support (openhab#6819)

9c3fb5b

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[Exec] Properly split command & pipe support (openhab#6819)

9bb948c

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[Exec] Properly split command & pipe support (openhab#6819)

61eb25e

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020

[Exec] Properly split command & pipe support (openhab#6819)

86fac2e

* [Exec] Properly split command Signed-off-by: Constantin Piber <cp.piber@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Exec] Properly split command & pipe support #6819

[Exec] Properly split command & pipe support #6819

cpiber commented Jan 12, 2020

TravisBuddy commented Jan 12, 2020

cpiber commented Jan 12, 2020

9037568 left a comment

cpiber left a comment

TravisBuddy commented Jan 12, 2020

TravisBuddy commented Jan 12, 2020

Rossko57 commented Jan 13, 2020

cpiber commented Jan 13, 2020

9037568 left a comment

TravisBuddy commented Jan 16, 2020

9037568 left a comment

TravisBuddy commented Jan 21, 2020

9037568 left a comment

TravisBuddy commented Jan 22, 2020

9037568 left a comment

J-N-K Jan 23, 2020

J-N-K Jan 23, 2020

J-N-K Jan 23, 2020

J-N-K Jan 23, 2020

cpiber Jan 23, 2020

cpiber Jan 23, 2020 •

edited

Loading

cpiber Jan 25, 2020

J-N-K Jan 25, 2020

TravisBuddy commented Jan 25, 2020

elordude commented Jun 23, 2020

Rossko57 commented Jun 23, 2020

[Exec] Properly split command & pipe support #6819

[Exec] Properly split command & pipe support #6819

Conversation

cpiber commented Jan 12, 2020

TravisBuddy commented Jan 12, 2020

Travis tests were successful

cpiber commented Jan 12, 2020

9037568 left a comment

Choose a reason for hiding this comment

cpiber left a comment

Choose a reason for hiding this comment

TravisBuddy commented Jan 12, 2020

Travis tests were successful

TravisBuddy commented Jan 12, 2020

Travis tests were successful

Rossko57 commented Jan 13, 2020

cpiber commented Jan 13, 2020

9037568 left a comment

Choose a reason for hiding this comment

TravisBuddy commented Jan 16, 2020

Travis tests were successful

9037568 left a comment

Choose a reason for hiding this comment

TravisBuddy commented Jan 21, 2020

Travis tests were successful

9037568 left a comment

Choose a reason for hiding this comment

TravisBuddy commented Jan 22, 2020

Travis tests were successful

9037568 left a comment

Choose a reason for hiding this comment

J-N-K Jan 23, 2020

Choose a reason for hiding this comment

J-N-K Jan 23, 2020

Choose a reason for hiding this comment

J-N-K Jan 23, 2020

Choose a reason for hiding this comment

J-N-K Jan 23, 2020

Choose a reason for hiding this comment

cpiber Jan 23, 2020

Choose a reason for hiding this comment

cpiber Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

cpiber Jan 25, 2020

Choose a reason for hiding this comment

J-N-K Jan 25, 2020

Choose a reason for hiding this comment

TravisBuddy commented Jan 25, 2020

Travis tests were successful

elordude commented Jun 23, 2020

Rossko57 commented Jun 23, 2020

cpiber Jan 23, 2020 •

edited

Loading