Vos avis pour améliorer mon script

**xekkos** · 14/09/2016, 09h02

Bonjour,

J'ai testé et corrigé quelque bug sur mon script ce matin. Je viens vous le présenter afin d'avoir des critiques pour améliorer ma manière d'écrire des scripts shell.

J'ai écris un petit scripte pour mettre en place les paramétre de configuration de mon Bus Pirate (carte électronique,voir ici)

Voici le code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
#!/bin/bash
 
echo "Bienvenu dans l'installation du programme buspirate"
 
if [ $EUID -ne 0 ] ; then
	echo "Il vous faut les droits administrateurs!"
	exit
fi
 
if [ -x screen ] ; then
	echo "Screen n'est pas installé dans votre système. Voulez-vous l'installer? [y/n]"
	read TEST
	if [ $TEST  == y ] ; then
		apt-get install screen
	else
		exit
	fi
fi
 
if [ -f /usr/bin/buspirate ] ; then
	echo "Le programme /usr/bin/buspirate est déjà installé. Voulez-vous le supprimer et le réinstaller? [y/n]"
	read TEST
	if [ $TEST == y ] ; then
		rm -fr /usr/bin/buspirate
		echo "Le programme est supprimé"
	else
		exit
	fi
fi
 
if [ -f /etc/udev/rules.d/98-buspirate.rules ] ; then
	echo "Le fichier /etc/udev/rules.d/98-buspirate.rules est déjà présent. Voulez-vous le supprimer? [y/n]"
	read TEST
	if [ $TEST == y ] ; then
		rm -fr /etc/udev/rules.d/98-buspirate.rules
		echo "Le fichier est supprimé"
	else
		exit
	fi
fi
 
echo "Brancher maintenant votre Bus Pirate. Si vous l'avez déjà fait, débrancher le puis rebrancher le et taper une touche!"
read
 
serialNumber=$(dmesg | tail -n15 | grep SerialNumber: | awk ' { print $NF }')
 
idVendor=$(dmesg | tail -n15 | grep idVendor | awk -F, ' { print $2 }' | awk -F= ' { print $2 }' )
 
idProduct=$(dmesg | tail -n15 | grep idProduct | awk -F= ' { print $NF }' )
 
echo 'SUBSYSTEM=="tty" ATTRS{idVendor}=="'$idVendor'" ATTRS{idProduct}=="'$idProduct'" ATTRS{serial}=="'$serialNumber'" GROUP="users" MODE="0660" SYMLINK+="buspirate"' > /etc/udev/rules.d/98-buspirate.rules
 
echo "Enregistrement udev créer"
 
cat > /usr/bin/buspirate << EOF
#!/bin/bash
sudo screen /dev/buspirate 115200 8N1
EOF
 
chmod +x /usr/bin/buspirate
 
echo "Scripte de démarrage crée"
echo "Pour utiliser votre programme, vous devez debranché et rebrancher votre Bus Pirate"

Merci pour vos critiques
Salutation

**disedorgue** · 14/09/2016, 11h06

Bonjour,

Quelques remarques:
-On protège les variables en les encadrant avec des guillemets ("$VARIABLE"):
Ici, dans ton test if [ $TEST == y ] tu aura une erreur de syntaxe si la personne appuie juste sur entrée lors de la commande read.
Et d'ailleurs même le y doit être encadré puisque c'est une chaine de caractère.
Donc, en plus propre: if [ "$TEST" == "y" ] (et en plus portable un seul =: if [ "$TEST" = "y" ])

-Ton test qui vérifie que screen est installé n'est pas bon, il faut mettre le chemin complet et non juste le nom de la commande et si ok, alors la commande est installée et non l'inverse...
Donc, il faut savoir le chemin de la commande...
Comme tu es en bash, j'aurai tendance à utiliser type (ou plus généralement la commande which):
Donc remplacer

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if [ -x screen ] ; then

par

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
type -t screen >/dev/null
if [ $? -ne 0 ] ; then

Après en conseil plus général, on essaie de mettre en variable en début de fichier les noms de fichier en dur afin de n'avoir à ne modifier celui-ci qu'a un seul endroit.

**BufferBob** · 14/09/2016, 11h26

salut,

au minimum dans ton script tu proposes de réinstaller buspirate, mais là tu ne fais que le supprimer, et à coups de rm, est-ce que le binaire ne fait pas partie d'un package ? (apt-get install --reinstall ?)

aussi pour lire tes trois variables je te propose (à vérifier/adapter) :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
read SerialNumber idVendor idProduct < <(dmesg | awk -F'[ \t=,]+' '
   /SerialNumber:/ { sn=$6 }
   /idVendor/ { idv=$10 }
   /idProduct/ {idp=$12}
   END { printf("%s %s %s\n", sn, idv, idp) }
')

**becket** · 14/09/2016, 11h50

Envoyé par xekkos

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
 
 
cat > /usr/bin/buspirate << EOF
#!/bin/bash
sudo screen /dev/buspirate 115200 8N1
EOF
 
chmod +x /usr/bin/buspirate

En fait, il crée son exécutable de toute pièce. Donc pas de soucis

**BufferBob** · 14/09/2016, 16h22

Envoyé par becket

En fait, il crée son exécutable de toute pièce. Donc pas de soucis

exact, je l'ai vu en plus... bref, my bad ^^

**xekkos** · 14/09/2016, 16h30

Merci pour ces bonnes critique et j'en prends note!

Pour que tu comprennes bien BufferBob. Je n'installe rien avec apt! Si tu veux j'ai une carte électronique avec un FTDI 232 pour communiquer avec un PIC. Il suffit juste de se connecter avec un terminal sur la carte. Je fais ce petit scripte tous simple juste pour faire un enregistrement udev et copier un autre petit script dans /usr/bin pour m'éviter de taper une commande plus longue! Hey oui je suis fainéant!! Rien de bien compliqué mais ça m'a déjà poser quelque problème et interrogation!

Merci pour ce condenser de remplissage des variables, j'aime! La commande marche nickel. Je crois voire que awk est vraiment le couteau suisse de traitement des chaînes de caractères? Il faut que je l'étudie plus!

En regardant vite fait le man de bash, je vois que j'ai pas besoin de fermer les guillemets pour entre une variable. C'est mieux comme cela ou c'est kifkif?
ancien code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

echo 'SUBSYSTEM=="tty" ATTRS{idVendor}=="'$idVendor'" ATTRS{idProduct}=="'$idProduct'" ATTRS{serial}=="'$serialNumber'" GROUP="users" MODE="0660" SYMLINK+="buspirate"' > /etc/udev/rules.d/98-buspirate.rules

Nouveau code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

echo 'SUBSYSTEM=="tty" ATTRS{idVendor}=="$idVendor" ATTRS{idProduct}=="$idProduct" ATTRS{serial}=="$serialNumber" GROUP="users" MODE="0660" SYMLINK+="buspirate"' > /etc/udev/rules.d/98-buspirate.rules

Merci disedorgue, c'est vrais que je n'avais pas penser mettre plus de variables enfin surtout les noms des fichiers en début! plus simple et plus logique. Tu me dit que je dois protéger mes variables avec des guillemets, je ne suis pas sur de bien comprendre mais en gros c'est pour qu'elle soit considéré comme des chaînes de caractères et non autre chose donc cela évite je ne sais pas quoi comme erreur? Je suis un peu juste?

Pour le test si screen est installé, j'ai prit bêtement une commande sur le net! Et vu que je l'ai déjà installé vu que ca ne criait pas j'ai pas penser à tester si ça marche correctement. En analysant ta proposition je constate que je n'ai pas de man sur type, c'est normal? Et pourquoi tu rediriges la sortie dans null? On ne peut pas laisser flottant?

Désoler pour toutes ces questions et encore merci à ceux qui prennent le temps de me répondre et aussi à ceux qui mon lut!
Salutation

**disedorgue** · 14/09/2016, 17h05

Dans le cas d'une chaine entre quote ('), tout les éléments de la chaine sont considéré comme des caractères normaux, donc pas remplacement de variable par la valeur de celle-ci.

Si tu n'encadres pas ta variable par des guillemets lorsque tu testes sa valeur et que celle-ci est vide, alors la commande de test considère qu'il lui manque un champs. Par contre lorsqu'elle est encadré, la commande voit bien le champs même si la variable est vide.
Et ici, on ne parle que de test, si ta variable est par exemple un nom de fichier avec des espaces, les commandes usuelle sur les fichiers verront autant de fichier qu'il y a de mot séparé par un espace au lieu de ne voir qu'un seul fichier.
Il existe une règle implicite qui est vraie dans 99,9% des cas: On encadre ses variables par des guillemets et si on ne le fait pas on est capable de dire pourquoi.

La commande type est une builtin bash, pour voir comment elle fonctionne, soit man bash soit help type depuis un shell bash.
Et je redirige sa sortie vers null car ce qui m'interresse, ce n'est pas ce qu'il affiche mais s'il sort en erreur ou pas.

**xekkos** · 14/09/2016, 17h20

Ok je m'en rappellerais du quote, je pense. En effet, c'est con de rajouter des erreurs! j'avais mal vérifier mon changement. Et maintenant, je comprends bien pourquoi je dois encadrer des variables. Je vais bien noter cette règle dans ma tête.
Merci pour cette réponse! Je finis de corriger mon script et améliorer la partie awk. Je reviendrais poster mon résultat

**xekkos** · 15/09/2016, 10h12

Bonjour,

J'ai profité pour de lire un peu de doc sur awk! Il me semble efficace dans beaucoup de cas mais pas forcément à appréhender au début! Il faudra que je fasse d'autre script avec pour m'y habituer. Pour finir la solution de BufferBob ne fonctionne pas!

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
read SerialNumber idVendor idProduct < <(dmesg | awk -F'[ \t=,]+' '
   /SerialNumber:/ { sn=$6 }
   /idVendor/ { idv=$10 }
   /idProduct/ {idp=$12}
   END { printf("%s %s %s\n", sn, idv, idp) }
')

J'ai pas tous compris pour bien annalyser cette commande. Déjà la redirection, pourquoi un espace entre les "<"? Après je pense que les parenthèses sont la comme en arithmétique afin d'exécuter le code en premier?!?
Dans l'argument -F de awk. Entre "[" et "]", je vois que c'est pour sélectionner plusieurs séparateurs mais pourquoi avoir mis un "+" à la fin?

J'ai essayé plein d'adaptation mais je n'arrive à rien.

Alors voici mon nouveau code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
#!/bin/bash
 
SCRIPTBUSPIRATE=/usr/bin/buspirate
ENREGISTREMENTUDEV=/etc/udev/rules.d/98-buspirate.rules
 
echo "Bienvenu dans l'installation du programme buspirate"
 
if [ "$EUID" -ne 0 ] ; then
	echo "Il vous faut les droits administrateurs!"
	exit
fi
 
type -t screen > /dev/null
if [ $? -ne 0 ] ; then
	echo "Screen n'est pas installé dans votre système. Voulez-vous l'installer? [y/n]"
	read TEST
	if [ "$TEST"  == "y" ] ; then
		apt-get install screen
	else
		exit
	fi
fi
 
if [ -f $SCRIPTBUSPIRATE ] ; then
	echo "Le programme $SCRIPTBUSPIRATE est déjà installé. Voulez-vous le supprimer et le réinstaller? [y/n]"
	read TEST
	if [ "$TEST" == "y" ] ; then
		rm -fr $SCRIPTBUSPIRATE
		echo "Le programme est supprimé"
	else
		exit
	fi
fi
 
if [ -f $ENREGISTREMENTUDEV ] ; then
	echo "Le fichier $ENREGISTREMENTUDEV est déjà présent. Voulez-vous le supprimer? [y/n]"
	read TEST
	if [ "$TEST" == "y" ] ; then
		rm -fr $SCRIPTBUSPIRATE
		echo "Le fichier est supprimé"
	else
		exit
	fi
fi
 
echo "Brancher maintenant votre Bus Pirate. Si vous l'avez déjà fait, débrancher le puis rebrancher le et taper une touche!"
read
 
serialNumber=$(dmesg | tail -n15 | grep SerialNumber: | awk ' { print $NF }')
idVendor=$(dmesg | tail -n15 | grep idVendor | awk -F, ' { print $2 }' | awk -F= ' { print $2 }' )
idProduct=$(dmesg | tail -n15 | grep idProduct | awk -F= ' { print $NF }' )
 
echo 'SUBSYSTEM=="tty" ATTRS{idVendor}=="'$idVendor'" ATTRS{idProduct}=="'$idProduct'" ATTRS{serial}=="'$serialNumber'" GROUP="users" MODE="0660" SYMLINK+="buspirate"' > $ENREGISTREMENTUDEV
 
echo "Enregistrement udev créer"
 
cat > $SCRIPTBUSPIRATE << EOF
#!/bin/bash
sudo screen /dev/buspirate 115200 8N1
EOF
 
chmod +x $SCRIPTBUSPIRATE
 
echo "Scripte de démarrage crée"
echo "Pour utiliser votre programme, vous devez debranché et rebrancher votre Bus Pirate"

Pour la partie détection des droits d'administrateurs, je la fais d'une bonne manière? J'ai voulu essayer de mettre su à la place exit en me disant qu'il allait me loguer comme root mais non! Une autre solution?

Et le choix d'utiliser /usr/bin pour enregistrer mon petit script est judicieux? Ou il y a un dossier mieux défini pour cela?

Merci et salutation

**BufferBob** · 15/09/2016, 10h43

Envoyé par xekkos

Pour finir la solution de BufferBob ne fonctionne pas!

ah ! non ! c'est un peu court jeune homme ! on pouvait dire... oh ! Dieu ! ...bien des choses en somme...
...en commençant par décrire ce qui ne fonctionne pas par exemple (et d'où la parenthèse plus haut "à vérifier/adapter"

)

J'ai pas tous compris pour bien annalyser cette commande. Déjà la redirection, pourquoi un espace entre les "<"? Après je pense que les parenthèses sont la comme en arithmétique afin d'exécuter le code en premier?!?

<(...) en bash c'est une substitution de processus, en gros ça évalue ce qu'il y a à l'intérieur des parenthèses et ça ressort comme un fichier qu'on lit, hors tu as probablement l'habitude du classique commande < fichier, ben là c'est pareil finalement, commande < <(commande), je te dirais bien que c'est équivalent à un pipe mais ce serait probablement mentir, puisque read a b c < <(machin); echo "$a $b $c" fonctionne comme attendu tandis que machin | read a b c; echo "$a $b $c" ne permettra pas d'accéder aux variables comme attendu (elles ne sont alors disponibles que dans le scope d'un subshell genre machin | { read a b c; echo "$a $b $c"; })

Dans l'argument -F de awk. Entre "[" et "]", je vois que c'est pour sélectionner plusieurs séparateurs mais pourquoi avoir mis un "+" à la fin?

c'est ni plus ni moins une regex, donc effectivement les crochets délimitent un ensemble de caractères potentiels, comme des espaces ou des tabulations, et le + à la fin signifie "1 ou plus", si par exemple en début de fichier on a un champ suivi d'une série d'espaces suivie d'un autre champ, on demande à awk de zapper tous les espaces et pas juste un seul

Pour la partie détection des droits d'administrateurs, je la fais d'une bonne manière? J'ai voulu essayer de mettre su à la place exit en me disant qu'il allait me loguer comme root mais non! Une autre solution?

tester l'euid ça me semble pertinent, spawn un login root beaucoup moins

Et le choix d'utiliser /usr/bin pour enregistrer mon petit script est judicieux? Ou il y a un dossier mieux défini pour cela?

je pense que je mettrais plutôt dans ~~/usr/local/bin ou~~ /usr/local/sbin
~~https://en.wikipedia.org/wiki/Filesy...archy_Standard~~
http://man7.org/linux/man-pages/man7/hier.7.html

**xekkos** · 15/09/2016, 11h46

Oui court car je suis parfois fainéant! C'est juste que dans un poste plus haut, j'avais dit qu'elle fonctionnaient nickel alors j'ai voulu me corriger mais j'ai bien compris qu'il fallait la prendre avec des pincettes! Même si elle ne fonctionnait pas elle m'a permit de comprendre l'enchaînement des actions.

Je commence à mieux comprendre les redirections de flux mais il faut que je creuse plus la différence entre un "{" et une "(". Regex encore un mot que je ne connais pas et qu'il faudra que je creuse! Je vais bien creuser ces temps, c'est pas toujours facile d'apprendre seul! Heureusement que vous êtes la merci encore!

Je reviendrais poster mes prochaines créations.
Salutation

**jack-ft** · 23/09/2016, 21h14

Envoyé par xekkos

Alors voici mon nouveau code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
#!/bin/bash
 
SCRIPTBUSPIRATE=/usr/bin/buspirate
ENREGISTREMENTUDEV=/etc/udev/rules.d/98-buspirate.rules
 
echo "Bienvenu dans l'installation du programme buspirate"
 
if [ "$EUID" -ne 0 ] ; then
	echo "Il vous faut les droits administrateurs!"
	exit
fi
 
type -t screen > /dev/null
if [ $? -ne 0 ] ; then
	echo "Screen n'est pas installé dans votre système. Voulez-vous l'installer? [y/n]"
	read TEST
	if [ "$TEST"  == "y" ] ; then
		apt-get install screen
	else
		exit
	fi
fi
 
if [ -f $SCRIPTBUSPIRATE ] ; then
	echo "Le programme $SCRIPTBUSPIRATE est déjà installé. Voulez-vous le supprimer et le réinstaller? [y/n]"
	read TEST
	if [ "$TEST" == "y" ] ; then
		rm -fr $SCRIPTBUSPIRATE
		echo "Le programme est supprimé"
	else
		exit
	fi
fi
 
if [ -f $ENREGISTREMENTUDEV ] ; then
	echo "Le fichier $ENREGISTREMENTUDEV est déjà présent. Voulez-vous le supprimer? [y/n]"
	read TEST
	if [ "$TEST" == "y" ] ; then
		rm -fr $SCRIPTBUSPIRATE
		echo "Le fichier est supprimé"
	else
		exit
	fi
fi
 
echo "Brancher maintenant votre Bus Pirate. Si vous l'avez déjà fait, débrancher le puis rebrancher le et taper une touche!"
read
 
serialNumber=$(dmesg | tail -n15 | grep SerialNumber: | awk ' { print $NF }')
idVendor=$(dmesg | tail -n15 | grep idVendor | awk -F, ' { print $2 }' | awk -F= ' { print $2 }' )
idProduct=$(dmesg | tail -n15 | grep idProduct | awk -F= ' { print $NF }' )
 
echo 'SUBSYSTEM=="tty" ATTRS{idVendor}=="'$idVendor'" ATTRS{idProduct}=="'$idProduct'" ATTRS{serial}=="'$serialNumber'" GROUP="users" MODE="0660" SYMLINK+="buspirate"' > $ENREGISTREMENTUDEV
 
echo "Enregistrement udev créer"
 
cat > $SCRIPTBUSPIRATE << EOF
#!/bin/bash
sudo screen /dev/buspirate 115200 8N1
EOF
 
chmod +x $SCRIPTBUSPIRATE
 
echo "Scripte de démarrage crée"
echo "Pour utiliser votre programme, vous devez debranché et rebrancher votre Bus Pirate"

[ORTHOGRAPHE]
Une petite amélioration qui ne va pas changer grand chose au fonctionnement du programme:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
echo "Bienvenue dans l'installation du programme buspirate"
	echo "Il vous faut les droits administrateur!" 1>&2
echo "Branchez maintenant votre Bus Pirate. Si vous l'avez déjà fait, débranchez-le puis rebranchez-le et tapez une touche!"
echo "Enregistrement udev créé"
echo "Script de démarrage créé"
echo "Pour utiliser votre programme, vous devez debrancher et rebrancher votre Bus Pirate"

[/ORTHOGRAPHE]

Vos avis pour améliorer mon script

Shell et commandes GNU

Discussions similaires

Partager

Partager