openhab / openhab-linuxpkg

Repo for Linux packages
Eclipse Public License 2.0
18 stars 33 forks source link

openhab-cli: tail vs. multitail, function override #99

Closed ThomDietrich closed 6 years ago

ThomDietrich commented 6 years ago

https://github.com/openhab/openhab-linuxpkg/blob/d001188d66cc10ff711acd417f02c85c3ee3d2de/resources/usr/bin/openhab-cli#L122-L124

@BClark09 openHABian now includes a multitail color scheme: https://github.com/openhab/openhabian/issues/196

How do you feel about multitail vs tail? The two panes and colors of multitail are clearly nice but I sometimes miss the option to scroll back up in tail (in my console). Which do you favor? (If you want to see the difference, execute openhablog, openhablog2 and openhablog3 on a fresh openHABian system.)

Going one step further, I wonder if it would be beneficial to be able to tune the behavior of the stiff distribution script openhab-cli on an openHABian system or potentially other systems. I'd fancy the option to modify how and what openhab-cli presents when called with the showlogs argument. How do you feel about the following proposal: IF certain alias exists THEN execute alias ELSE execute default routine. We could add such a behavior to all subroutines (even though I'd be interested in the log one).

What do you think? Did I miss anything? A potential security risk maybe?

BClark09 commented 6 years ago

The purpose of openhab-cli was to provide a direct command line link to the appropriate shortcut in openHAB, for the start/stop/backup/restore commands this was obvious but I was unsure of what to do for log viewing as there are so many ways of doing this. (openhablog2 and openhablog3 being much better than a standard tail IMO but not guarenteed to work with the majority of .deb and .rpm based systems).

Your suggestion is an excellent way of getting round the no reliance on external scripts rule! I can't see why it would be a problem, but there might be a potential security risk if the alias was somehow replaced by a 3rd party (I'm not well-read in these things). It might be better to use an environment variable:

if [ -z "${OPENHAB_LOG_VIEWER}" ]; then
  tail -f "${OPENHAB_LOGDIR:?}"/*.log
else
  echo "executing custom log viewer: ${OPENHAB_LOG_VIEWER}"
  exec $OPENHAB_LOG_VIEWER
fi

What do you think? Would you be able to use that? What would be the best name for the env var?

ThomDietrich commented 6 years ago

Hey Ben, I thought about the issue and had the chance to discuss it with a colleague just now. We both agree that a environment variable is not really better than an alias. Also if a 3rd party wants to harm the system (not sure why they should take the detour) they can do that either way. On top we realized, that aliases/envvars depend on the user and the environment the openhab-cli script is called from. All in all not ideal.

Here is my (our) new proposal: Let's define a script file somewhere under /var/lib/openhab (owned by root) similar to below. The file will be sourced in openhab-cli and conditionally called as discussed above. Advantages: secure as it belongs to root, execution-independent, flexible beyond one command.

# openhab-cli-overide.sh
# For modified behavior

# function showlogs_override () { }

# function console_override () { }

As for the integration, I'd turn it around:

if declare -F showlogs_override &>/dev/null; then
  showlogs_override
else
  tail -f "${OPENHAB_LOGDIR:?}"/*.log
fi

I don't think we need the echo. For the end user there shouldn't be a difference.

Wdyt?

BClark09 commented 6 years ago

Sounds good! but (and you're probably going to hate me for it 😇 ) how do we do that POSIXly?

From https://unix.stackexchange.com/questions/332005/test-for-functions-existence-that-can-work-on-both-bash-and-zsh

if type 'showlogs_override' 2>/dev/null | grep -q 'function'
then
   showlogs_override
else
  tail -f "${OPENHAB_LOGDIR:?}"/*.log
fi

Would you like this before 2.2.0? Making a root folder inside userdata might be tricky, there are several times where this folder might be swept for openhab user ownership.

ThomDietrich commented 6 years ago

Hate is a strong word 😄 looks good and works for me. POSIX is a noble goal, my scripts are all bash 4 compatible, which is a good compromise in my eyes.

Yes 2.2 would be great! In fact the issue is here to bring this into 2.2 so I am not tempted to hack the base script. 😅

About the location and root: You are right. Imho the file should be located somewhere it is not automatically overwritten on update and where it naturally has root ownership. Sadly that rules out /var/lib/openhab2, /usr/share/openhab2 and /etc/openhab2... correct!? Do you have a good idea? From the Linux FHS point of view I'd say it has to go somewhere below /usr. How about /usr/share/openhab-cli_overide.sh? If you have a better (and founded) idea I have no issue with that, let's move forward. How can I help?

BClark09 commented 6 years ago

I will try! To prevent writing directly to /usr/share/ shall we create a /etc/openhab2-cli folder? A configuration folder specifically for the cli doesn't seem too far-fetched right?

EDIT: Reading the FHS, /etc/ would be more appropriate than /var/lib/ because the latter is data that edited by the program itself. /usr/ should be read only. Have I interpreted that correctly?

BClark09 commented 6 years ago

I made a draft PR, and tested it on Debian, Fedora and CentOS. Please let me know what you think, or what your folder preference is.