# Tag Info

15

That looks crazy. But some constructive comments: Did you check for ssh-copy-id script, that is usually shipped with openssh? It solves for you the first part of your problem in standard way. The second problem is in my eyes non-existent. Do you really need your authorized_keys in all accounts? Isn't it enough for one user that can do sudo? The third is ...

11

I'll proceed top to bottom. mail_error Which would you rather read: x.body = e.message or mail.body = exception.message? Don't try to save a few characters on naming — your fellow programmers will like you better. In particular, x has a connotation of being a floating point number, which this isn't. scanning_all_servers and server_status Net::SSH#start ...

10

target = ARGV[0].to_s user = ARGV[1].to_s wordlist_file=ARGV[2].to_s Aren't those Strings already? network = 0 (1..5).each do if icmp.ping network += 1 end end would be more idiomatic as: network = 5.times.count do icmp.ping end #count will count how many times block returned true, exactly what you want. Ruby has "perlish" globals ...

9

To answer your questions: Yes. Your code could be boiled down to: #!/bin/python import sys import os os.system("sshpass -p VerySecrectPassword ssh pi@172.16.0.141" + " " .join(sys.argv[1:])) Note that I also put the imports on two lines, as recommended by PEP8, Python's official style-guide. But I would actually re-expand it to: #!/bin/python import ...

8

In modern systems, there's ssh-copy-id for this. But yeah it's not everywhere. In systems that don't have it, I have a similar script like yours, but to mimic the "standard", I call it ssh-copy-id.sh. Name it as you like, but I don't think "loadkey" really sums up the action of appending your public key to the authorized list file on a remote server. So I'd ...

8

This answer is going to talk about your style, and best practices. Some of it may be subjective. I'll go through the code line by line. use strict; use warnings; Good job! Modern Perl programs should always start with strict and warnings. If you want more detailed information on what's wrong, you can add use diagnostics, but remember to remove it before ...

7

Wrapping your methods in a class definition isn't the same thing as making your project OOP. These methods don't care about their receiver, and forcing the programmer to instantiate the PrintJobs class before calling them doesn't accomplish anything useful here. Now, for special-purpose scripts like this, not being OO isn't the worst thing in the world - in ...

7

One of the core elements your code is missing is object-orientation. Take advantage of Ruby's powerful object model. That alone will vastly improve what you have. What we're going to do is progressively breakdown everything into chunks of logic and encapsulate these chunks of logic in objects and methods. Before that, let's go through each line and give ...

6

I see a few things that may help you improve your code. Pass by const reference where practical In every case that a QString ip is passed, the code could instead use a const QString &ip avoiding pointless multiple copies and also allowing the addition of a const IP address. Consider large files Reading the entire file into memory using QTextStream::...

5

The conditions to create keys The condition on the directory seems a bit excessive here: if [ ! -d "$HOME/.ssh" ] && [ ! -f "$HOME/.ssh/id_rsa" ] && [ ! -f "$HOME/.ssh/id_rsa.pub" ]; then echo -e "Private / Public keys not generated" echo -e "Generating..." ssh-keygen -b 4096 fi My concern is that if the key files are missing ... 5 As suggested in the comments, I would strongly suggest looking at a proper configuration management tool such as Ansible or Puppet (both free) instead of rolling your own. That will probably save you a lot of time, once you’ve done the initial ramp-up. Here are some comments on the code presented: There aren’t any comments or documentation. It’s hard to ... 5 All your imports should be below eachother like this: import sys import paramiko import getpass I think using classes is a bit too much for this, but you should create a few functions to diverse functionality A Main A function for the shell commands A function for connecting This should be enough for now, you can always add more. Secondly when using ... 4 Your main questions When I pass the IP address, should the class just store it internally and then work with it, or is passing in between member functions better? You mean, you should pass data around as function parameters, or store inside fields. In your example, storing in fields doesn't really make sense. Currently you have this: The read function ... 4 Avoid echo -ne Although you intend to use this script specifically on Mac OS X, as a general rule of thumb, it's good to avoid the various flags of echo, because they are not portable. In some of your use cases the flags are completely unnecessary, for example instead of this: echo -ne "V.$VER\n\n" You could write like this to get the same effect: echo ...

4

Bug / useless line In the block of code: id.strip puts "Cancelling job: #{id}" #sudo cancel #{id} execute.exec!("sudo cancel #{id}") id.strip generates a value to be immediately discarded, essentially a no-op. In Ruby immutability is the default: look at the following IRB session for clarification: irb(main):001:0> x= " foo" => " foo" ...

4

I would start by making your ip_list easier to understand. You could write a class for this, but using a collectins.namedtuple is a lot easier here: from collections import namedtuple Client = namedtuple("Client", "ip username passwd root_passwd") clients = [Client("192.168.11.44", "root", "****", "****"), Client("192.168.11.8", "root", "****", ...

4

Looks like Fabric is what you need. Fabric is a pretty simple deploy tool (comparing to Ansible/Puppet/etc), that lets you execute simple shell commands over SSH (Paramiko inside). Features you will get from it: You can specify SSH key path using command line: fab command -i /path/to/key.pem or parse it from command line: import sys from fabric.api ...

4

Running ssh in a loop is not efficient. Since the script is not interactive, you could pass the entire script to a remote Bash process on stdin, so that loop will run entirely on the remote server, locally: ssh remotehost bash << "EOF" mydirs=(/var/www/files /var/www/photos /var/www/info) for d in ${mydirs[@]}; do test -d$d res=$? test$res -...

3

Constants in UPPERCASE @username = 'user' @password = nil @host = 'server_to_run_command_on' should be constants, as well as: LOGFILE = "path/to/log.txt" Marking a variable as constant in Ruby just needs upper-casing its name. Return value simplification res = check.exec!(cmd) data = res write_data(data) Is unnecessarily long just write: ...

3

Overall, the code is not complicated and can be understood. I would though focus on improving variable names and modularity: remember that the code is much more often read than written - try to name your variables in a meaningful way: for line in lines should be for host in hosts for i in range(1, 3) can be for customer_index in range(1, 3) cmds may be ...

3

Looks good to me. Kudos for the "... or die" idiom. More informative output would be "unable to run df on $s" or "unable to run svmon on$s". You don't have to do tail or grep on the far end, as a line of perl could do that. But it is a perfectly clean and sensible approach, which I advocate keeping. If you wind up with many such monitoring commands, you ...

3

Succinct: #!/bin/bash -e ssh remotehost mkdir -p /var/www/{files,photos,info} Failure to create any directory will give non-zero exit for ssh, which causes the outer script to exit with error, because of the -e switch. mkdir will print suitable error messages if it encounters errors. If you prefer the error message to appear on standard output (like ...

3

I think this is interesting code. I admit I haven't dug much into Ruby's Net::SSH library, so I bet you've learned some things about it in writing this code. It sounds to me like your biggest concern now is about professionalism, stability, and dependability which is why you want feedback before offering to use this code in a production environment. If so, I ...

2

It's a pity to enter all those server names manually, twice: once for the choice number listing, and one more time for the ssh / telnet command selection. A better way would have been to create a file with the list of the command-servername pairs, like this: ssh ESP_R7609S_MTSO ssh ESP_R7609_RIGUERO telnet ESP_R7609_SUBASTA ssh ESP_R7606_BOLONIA ssh ...

2

First of all, why do you repeat read -n4 COMMND in every block? Just put it at the top of the loop. Typically, chat applications are implemented either as "client-server" -- one machine is the "server" and listens for requests from clients then distributes them out, or as "peer-to-peer" -- each chatter sends messages to the other chatter(s), who are ...

2

I don't see much reason to worry about this method's length. At least not right now - if you add more event handling, yeah, it might become a little cumbersome. But for what it does now, there's no pressing need to do anything. I would recommend more whitespace, though. There's no reason to compress,every,line,that,has,commas,in,it. Just makes it harder to ...

2

According to your update, mkdir may be ok. But a more official way is to use flock. You'd better to lock the file in the shell script. Locking using Java can't make any another process accessing the file. Lock the file using flock in shell.

2

In the absence of different strings because of symlinks, hardlinks, multiple IPs for the same machine and hostnames instead of raw IPs, then yes, the synchronized combined with intern is thread-safe; no idea about performance, which is probably not an issue anyway. If you only run all of this from a single program then it's probably okay in general, ...

2

Some comments: def initialize(host, user=Etc.getlogin, *args). Ugly signature. I'd write def initialize(host, options = {}) I had never seen that multiline way of writing a conditional. It does not look too good... Use File::join instead of joining paths by hand. The arguments in the documentation are wrong. args << Etc.getpwnam(Etc.getlogin).dir + '/....

2

Some notes: Your documentation doesn't match... well, it doesn't match anything in your code! Wrong documentation is the only thing worse than no documentation. A ternary should not span multiple lines. Thankfully, a regular if..else is also an expression in Ruby. @keys = if args.empty? Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa' else args end ...

Only top voted, non community-wiki answers of a minimum length are eligible