Реквест критики bash скрипта

Перепишите на питон и не морочте голову.

Специально написал скрипт для критики? ЛОЛ

Ну, во-первых все аргументы должны быть в кавычках.

Во-вторых раз уж ты эхоэш посреди скрипта, притом что пишешь кудя не попадя, то хотя бы перенаправь в ошибочный поток.

// 2maxcom: ну ты и сволочь! от тебя всего-то требовалось скрипты разместить. А ты продал всех анонимусов гуглу. Сам пройди эту его капчу!!!11

Не вникая в суть скрипта: глобальные переменные надо капсом, логгирование и эхо данные, что передаются скрипту - две совершенно разные операции (тобишь, создавай logger функцию). Вместо отдельных файлов с данными - один конфиг, с импортом переменных, тоесть никаких var=$(cat file). В чем нужность контроля/разграничения процедуры инициализации через параметр скрипта - Хз.

В общем, присоединяюсь к анонимусу. Эта песня может длиться вечно.

Перегрузят машину, после создания вашего .lock и больше скрипт не заработает. Не так пишутся блокировки. Что вы добиваетесь заходя по ssh локально по «удаленному» адресу, всё равно ваш free_port «создаётся» тупо.

переменные не экранированы в фигурные скобки, типа: ${VAR}

Лок лучше cделать через flock

1) Команды, передаваемые ssh'у, лучше засунуть в «подвал» скрипта (после exit), обособить сверху и снизу строками (я делаю что-то вроде

== include.this.shit ->

== <- include.this.shit

2) Если внутри двойных кавычек ничего не expand'иться, то и не нужно их использовать - для этих случаев есть одинарные кавычки

3) Для конструкции [ test ] в BASH есть своя реализация с гораздо более широкими возможностями: [[ test ]]

4) Очень много хардкода: одно и то же название каталога в куче разных мест, все пути прибиты гвоздями. При этом весь этот хардкод вообще хорошо бы принимать параметрами скрипта: getopts вам в руки

5) А что если выбранный вами port на самом деле уже кем-то занят? Можно конечно смотреть по fail'у ssh, но тогда сам fail разбирать нужно. Не легче ли проверять записью в /dev/tcp/host/port? Ну или lsof/netstat

6) Насчёт логирования здесь правильно заметили: у вас мало того, что тупо echo, так ещё и не в STDERR

C .lock логика мутная немного... Уж если что-то подобное нужно сделать - сохраняйте туда хотя бы pid процесса ssh -L

Кстати, а что оно делает?

ssh $core \
  -Nf \
  -L $port:localhost:$port \
  -R $port:localhost:22 \
  -o ExitOnForwardFailure=yes

-R перенаправит через другой (?) туннель весь трафик на машине $core, который шёл на tcp-порт $port, на вашу локальную машину (где запускается скрипт), порт 22. В итоге по идее если сделать коннект на локальную машину, порт $port, то этот коннект уйдёт на $core и через другой туннель вернётся снова на локальную машину, на sshd-сервер.

Ничего не понимаю, это зачем так?

function везде убрать, это бестолковый башизм чтобы не писать (), которые там и так есть.
Кавычки стоят везде где не надо и не стоят там где надо (у автора похоже вообще нет идей для чего они служат)
Вместо [ использовать [[ (т.к. #!/bin/bash)

Спасибо за респонс! Обязательно прислушаюсь. На тему вопроса про это.

ssh $core \
  -Nf \
  -L $port:localhost:$port \
  -R $port:localhost:22 \
  -o ExitOnForwardFailure=yes

ssh -p $port localhost "touch $cfile" 2> /dev/null
if [ ! -e $cfile ];
then
	echo "galnet: connection to port $port is NOT OK"
	connect
else
	echo "galnet: connection to port $port is OK"
	rm $cfile
fi

Сам скрипт надо без аргументов запускать в хроне.

переменные не экранированы в фигурные скобки, типа: ${VAR}

Фигурные скобки ничего не экранируют.

глобальные переменные надо капсом

Нет.

Так и должно быть. Скрипт должен запускаться через хрон, поэтому, если я не хочу, чтобы он работал, я создаю lock.

Так и должно быть.

Нет.

Скрипт должен запускаться через хрон, поэтому, если я не хочу, чтобы он работал,

Для этого надо редактировать crontab

я создаю lock.

Этот термин означает блокировку. То есть защиту когда уже занято. Но она делается не так.

Таким образом я могу проверить ssh подулючение. Если на $port локальной машины мы достукиваемся до своего де sshd, то соединение работает, а в противном случае мы переподключаемся.

Если ваш порт уже занят, то на него невозможно повесить редирект с ssh, если свободен, то соединения с ssh не будет и следовательно touch тоже. Так что что вы на самом деле проверяете — фиг знает.

Мне удобнее запустить одну команду, чем редактировать crontab каждый раз, когда я не хочу, чтобы скрипт работал. Да, я использую термин lock не для того, что обычно под этим подразумевают. Но это логичное название файла, который блокирует работу.

Я проверяю, не упало ли соединение. Такое происходит: 1) при потери связи 2) при ребуте машины 3) при ребуте сервера

Я проверяю, не упало ли соединение.

Да наздоровье. Непонятно только, что в результате у вас запрограммировано. Вы проверяете, изменился ли локальный файл? Так как он может создаться, если может только два состояния: либо на порту висит редирект на удаленный хост, либо ничего вообще?

Я говорю ssh клиенту подключиться и создать $cfile. Если подключение есть, то файл создался. Если нет, то не создался. Это я и проверяю.

Заметьте, что я файл после проверки удаляю.

Переменные в строках можно совмещать:
"/${FOO}lib${BAR}"
на выходе:
"/var/lib/mail"

Держите нас в курсе своих открытий. :-)

Я говорю ssh клиенту подключиться и создать $cfile.

Если вы подключаетесь на редиректнутый порт, то файл у вас создаётся на удаленной машине. Собственно для этого «ssh с командой» и предназначен.

Вам что-то не нравится?

Да, придирки не по делу. А насколько я вижу, там нет мест, где нужна полная запись со скобками.

Внимательно посмотрите на код.

ssh $core \
  -Nf \
  -L $port:localhost:$port \
  -R $port:localhost:22 \
  -o ExitOnForwardFailure=yes

У меня $port локалки редиректится на 22 порт локалки. Так что файл создается на моей машине.

Внимательно посмотрите на код

Делать мне больше нечего чтоли? Я открыл вашу ссылку, установил скроллер посредине и увидел строчку 56...

глобальные переменные надо капсом

Нет.

Ещё префикс g_ можно.

Можно, конечно. А можно именной. Но не нужно.

Это скорее не локальные переменные, а константы, они же не меняются нигде.

Я бы их сделал как-то так:

declare -r CORE_HOST='my.host.example.com' \
           CORE_USER='iam'

2018

Писать на bash что-то длиннее однострочника

Лучшие альтернативы?

Он подразумевает, что нужно писать на python - и тогда первыми 5-ю строчками будут import'ы, это лучше, чем просто писать полезный код, потому что это модно сейчас у анонимусов, а это очень важный момент.

Серьёзный недостаток, действительно. Кстати,

from a import b, c

Как бы если можно не ставить никакие быдлопитоньи пакеты, написанные левой рукой через правую ногу какими-то недотяпами типа анонимусов, а использовать уже установленные в системе проверенные временем качественные утилиты, написанные умными людьми на Си и С++, то я как бы однозначно выберу второе, если только утилитами можно ограничиться (если сама логика приложения несложная).

Питоньи пакеты уже стоят, если ты не вантузятник и тоже написаны на Си с С++. Насчёт качества утилит можно поспорить, если посмотреть их чейнджлоги, да и сама идея дёргать бинарники когда можно дёргать сразу функции из библиотек не от умных людей явно.

Не легче ли проверять записью в /dev/tcp/host/port? Ну или lsof/netstat

эталонный race condition

еще не помешало бы падать при ненулевых кодах возврата всяческих ssh. я делаю || exit $?, но я ничего длинней 10 строчек на баше не пишу

Есть set -e, который и сам будет «падать», я вообще не ленюсь и внятное сообщение об ошибке формирую вместо того, чтобы для чего-то возвращать в exit ничего не говорящий код. Код вернуть нужно, только осмысленный, но сообщение обязательно тоже должно быть.

race condition в bash-скрипте, вызываемом по расписанию с гранулярностью демона крон (не чаще, чем раз в минуту)? Да вы, батя, писатель-фантаст!

Классика. Хуже вендового и веб-говнокода только говнокод на баше.

раз в минуту - это полмиллиона раз в год. если однопроцентный шанс словить баг раз в год тебя устраивает - ну ради бога

Это скорее пользовательские настройки.

Их даже может быть вполне уместно читать из переменных среды, и тогда они действительно должны быть в верхнем регистре, и с именной приставкой:

: ${GALNET_NODE_CORE_HOST:='my.host.example.com'} \
  ${GALNET_NODE_CORE_USER:='iam'}

за 2> /dev/null - убивольнять!

case...esac в конце заменить на что-то типа if declare -F «$1» ; then «$1» else main ; fi

if test -d повторяется слишком часто.

А получится описать хотя бы один сценарий возникновения race-condition? Балабол вы, батенька, (бал)абол!

А получится описать хотя бы один сценарий возникновения race-condition? Балабол вы, батенька, (бал)абол!

я не пойму, ты то ли меня спросить пытаешься, то ли сам с собой разговориваешь. ты ведь в курсе, что я не смогу ответить, пока ты не отправишь вопрос? я че-то не уверен.

и чего ты все выходные ждал, чтобы пластинку сменить? на лор как на работу?

Хорошая отмазка. Т.е. ты не видишь вопроса? Ну ОК, мне тогда не имеет смысла продолжать метать бисер.

Т.е. ты не видишь вопроса?

я вижу то ли чрезмерное самолюбование, то ли откровенное хамство. мы, конечно, не на детском утреннике, но что-то в этот раз у меня есть настроение оставить тебя без ответа.