Vos avis pour améliorer mon script

Version imprimable

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:

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

14/09/2016, 11h06
disedorgue
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:

if [ -x screen ] ; then

par
Code:

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.

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:

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) } ')

Citation:
Envoyé par xekkos
Code:

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

14/09/2016, 16h22
BufferBob

Citation:

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 ^^
14/09/2016, 16h30
xekkos

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:

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:

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
14/09/2016, 17h05
disedorgue

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.
14/09/2016, 17h20
xekkos

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

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:

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:

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

15/09/2016, 10h43
BufferBob

Citation:

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" ;))

Citation:

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"; })

Citation:

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

Citation:

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

Citation:

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
15/09/2016, 11h46
xekkos

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

Citation:

Envoyé par xekkos

Alors voici mon nouveau code:

Code:

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:

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]