[pve-devel] [PATCH v3 #1752 pve-manager] implement basic wake on LAN functionality

[Thomas Lamprecht]

On 1/16/19 11:31 AM, Christian Ebner wrote:
> Provides the basic functionality to allow a wake on LAN call to start sleeping 
> nodes in a cluster from an other cluster member.

works well, nice! I've some style and little improvements comments still.

> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> Changes to v3:
>     * 'wol' -> 'wakeonlan'
>     * 'or' -> '||'
>     * Changed the permission back to /nodes/{node} as discussed offline
> 
>  PVE/API2/Nodes.pm | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  PVE/NodeConfig.pm |  6 ++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index dd5471f8..144dff81 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -48,6 +48,7 @@ use Digest::MD5;
>  use Digest::SHA;
>  use PVE::API2::Disks;
>  use JSON;
> +use Socket;
>  
>  use base qw(PVE::RESTHandler);
>  
> @@ -168,6 +169,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'version' },
>  	    { name => 'syslog' },
>  	    { name => 'status' },
> +	    { name => 'wakeonlan' },
>  	    { name => 'subscription' },
>  	    { name => 'report' },
>  	    { name => 'tasks' },
> @@ -466,6 +468,50 @@ __PACKAGE__->register_method({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method({
> +    name => 'wakeonlan',
> +    path => 'wakeonlan',
> +    method => 'POST',
> +    permissions => {
> +	check => ['perm', '/nodes/{node}', [ 'Sys.PowerMgmt' ]],
> +    },
> +    protected => 1,
> +    description => "Boot node via 'wake on LAN'.",

nitpick: Maybe something like:
Try to wake a node via a 'wake on LAN' network packet.

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node', {
> +		description => 'target node for wake on LAN packet',
> +	    }),
> +	},
> +    },
> +    returns => { type => "null" },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $config = PVE::NodeConfig::load_config($param->{node});
> +	my $mac_addr = $config->{wakeonlan};
> +	if (!defined($mac_addr)) {
> +	    die "No wake on LAN MAC address defined for '$param->{node}'!\n";
> +	}
> +
> +	$mac_addr =~ s/://g;
> +	my $packet = chr(0xff) x 6 . pack('H*', $mac_addr) x 16;


> +
> +	my $proto = getprotobyname('udp');
> +	my $addr = gethostbyname('255.255.255.255');
> +	my $port = getservbyname('discard', 'udp');
> +	my $to = Socket::pack_sockaddr_in($port, $addr);
> +	socket(my $sock, Socket::AF_INET, Socket::SOCK_DGRAM, $proto)

Instead of $proto you could directly use: Socket::IPPROTO_UDP here and remove $proto
all together.

> +	    || die "Unable to open socket for sending wake on LAN packet!\n";

here it isn't important for what the socket would have been used, so I'd
omit that, as an caller sees that the exception came from the wakeonlan call
and knows already where to look for problems.

Further, perl has some error variables[0] which can tell an user why the
operation failed, there are ones for various levels, e.g. $@ for getting
perl level errors, e.g., if you died inside an eval { }; block you could
check those. Then there's $!, which equals to the errno if perl calls into
the C runtime, like it does here for, e.g., socket, setsockopt and send.

So if you use something like:

die "Unable to open socket: $!\n";

the user sees the reason for the failure.

[0]: https://perldoc.perl.org/perlvar.html#Error-Variables

> +	setsockopt($sock, Socket::SOL_SOCKET, Socket::SO_BROADCAST, 1)
> +	    || die "Unable to set socket option for sending wake on LAN packet!\n";

nit: for readability maybe add a newline here?

> +	send($sock, $packet, 0, $to)
> +	    || die "Unable to send wake on LAN packet!\n";
> +	close($sock);
> +
> +	return undef;
> +    }});
>  
>  __PACKAGE__->register_method({
>      name => 'rrd', 
> diff --git a/PVE/NodeConfig.pm b/PVE/NodeConfig.pm
> index 8ab88130..f82f4ace 100644
> --- a/PVE/NodeConfig.pm
> +++ b/PVE/NodeConfig.pm
> @@ -61,6 +61,12 @@ my $confdesc = {
>  	description => 'Node description/comment.',
>  	optional => 1,
>      },
> +    wakeonlan => {
> +	type => 'string',
> +	description => 'MAC address for wake on LAN',
> +	pattern => '^([0-9a-fA-F]{2}:){5}([0-9a-fA-F]{2})$',
> +	optional => 1,
> +    },
>  };
>  
>  my $acmedesc = {
>