[pve-devel] [PATCH #1752 pve-manager 1/1] [PATCH #1752 pve-manager] Impl basic wake on LAN

[Thomas Lamprecht]

Thanks for the patch! some comments inline.

On 1/14/19 3:05 PM, Christian Ebner wrote:> Provides the basic functionality to provide a wake on LAN feature implementation to start nodes in a cluster from other nodes.

can you please break up long lines? 69 characters is a common used line length
limit for commit messages.

> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 41 +++++++++++++++++++++++++++++++++++++++++
>  PVE/NodeConfig.pm |  6 ++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index dd5471f8..f59133dc 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -168,6 +168,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'version' },
>  	    { name => 'syslog' },
>  	    { name => 'status' },
> +	    { name => 'wol' },
>  	    { name => 'subscription' },
>  	    { name => 'report' },
>  	    { name => 'tasks' },
> @@ -466,6 +467,46 @@ __PACKAGE__->register_method({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method({
> +    name => 'wol',
> +    path => 'wol',
> +    method => 'POST',
> +    permissions => {
> +	check => ['perm', '/nodes/{node}', [ 'Sys.PowerMgmt' ]],
> +    },
> +    protected => 1,
> +    description => "Boot node via 'wake on LAN'.",
> +    proxyto => 'node',

you need to remove the proxyto, else it proxies to the node you want to wake
up, which probably cannot answer the API call if not running. :)

See for 'proxy_to' in the PVE::HTTPServer::rest_handler method in this repo to
see what it really does.

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),

I'd extend this get_standard_option use to change its default description so
that it gets made clear that this is the "target node" for the WOL packet.

You can do this with:

node => get_standard_option('pve-node', {
    description => '...',
}),

> +	},
> +    },
> +    returns => { type => "null" },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $config = PVE::NodeConfig::load_config($param->{node});
> +	my $mac_addr = $config->{mac};
> +	die "No wake on LAN mac address defined for node '$param->{node}'!\n" unless ($mac_addr);

while you will find it in a lot of perl code examples in the wild we do not use
'unless', do something like:

die "No wake on LAN mac address defined for node '$param->{node}'!\n" unless ($mac_addr);
    if !defined($mac_addr);

Our Perl style guide could be helpful to get the most common guidelines/used
patterns: https://pve.proxmox.com/wiki/Perl_Style_Guide

> +
> +	my $prefix = chr(0xff) x 6;
> +	my @mac = split(':', $mac_addr);
> +	my $mac = pack('H*', join('', @mac));

I do not really like the reuse of the mac variable name, even if one is a
scalar and the other a list.

> +	my $packet = $prefix . $mac x 16;

You could also do: 

$mac_addr =~ s/://;
my $packet = chr(0xFF) x 6 . pack('H*', $mac_addr) x 16;

not sure if the single line packet assembly isn't nicer, on one hand you see
directly what happens, but it may look a bit more cryptic, not totally sure.

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

you use the Socket module without ever declaring a "use" statement. It works
because the 'use PVE::Tools' pulls it in, but we try to declare dependencies
nonetheless explicit as else refactoring may break code which was seemingly
independent of the refactoring itself.

> +	socket(my $sock, Socket::AF_INET, Socket::SOCK_DGRAM, $proto) or die "Unable to open socket for sending wake on LAN magic packet!\n";
> +	setsockopt($sock, Socket::SOL_SOCKET, Socket::SO_BROADCAST, 1) or die "Unable to set socket options for sending wake on LAN magic packet!\n";

please try to avoid really long lines, only use them if breaking it up would
make something even less readable. While <= 80 characters per line would be
ideal it isn't always a hard limit, e.g., if a single error message needs a bit
more it often is easier to read when not breaking up.

> +	send($sock, $packet, 0, $to) or die "Unable to send wake on LAN magic packet!\n";
> +	close($sock);
> +
> +	return undef;
> +    }});
>  
>  __PACKAGE__->register_method({
>      name => 'rrd', 
> diff --git a/PVE/NodeConfig.pm b/PVE/NodeConfig.pm
> index af1c782b..3cc8ba48 100644
> --- a/PVE/NodeConfig.pm
> +++ b/PVE/NodeConfig.pm
> @@ -61,6 +61,12 @@ my $confdesc = {
>  	description => 'Node description/comment.',
>  	optional => 1,
>      },
> +    mac => {

I'd prefer something like 'wol-mac' as mac is just to general. But currently
this config parser does not supports a - separator (only _) so either we settle
for wol-mac or allow '-'.

I'd to the latter, if there isn't any reason against it (@Fabian, I guess that
there wasn't?)

> +	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 = {
>