[pve-devel] [PATCH v2 #1752 pve-manager 2/3] patch #1752 Impl basic wake on LAN

[Thomas Lamprecht]

If you send a v2 squash the changes in the original patch, it looks weird
to introduce new code and then a patch later fixing it. Besides that it makes
it a bit harder to review and harder to see what happened here when looking
back at this in a few months via git.

On 1/15/19 11:54 AM, Christian Ebner wrote:
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---

it's often desired to add a (small) changelog here, telling a reviewer about the
changes you made from the previous version. Things after the --- above and the
diff a/... b/... won't make it in the commit (neither code wise or commit message
wise so it's a good place to add patch meta info, like changelogs or some thought
about the patch.

See: https://pve.proxmox.com/pipermail/pve-devel/2019-January/035146.html
for an example. If no changes happened, possible in when having multiple patches
in a series where only some needed changes it's also nice to tell the list that,
e.g., with a simple "no changes" comment here.


>  PVE/API2/Nodes.pm | 26 +++++++++++++++-----------
>  PVE/NodeConfig.pm |  2 +-
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index f59133dc..f831bc41 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);
>  
> @@ -476,11 +477,12 @@ __PACKAGE__->register_method({
>      },
>      protected => 1,
>      description => "Boot node via 'wake on LAN'.",
> -    proxyto => 'node',
>      parameters => {
>  	additionalProperties => 0,
>  	properties => {
> -	    node => get_standard_option('pve-node'),
> +	    node => get_standard_option('pve-node'), {
> +		description => 'target node for wake on LAN packet',
> +	    },
>  	},
>      },
>      returns => { type => "null" },
> @@ -488,21 +490,23 @@ __PACKAGE__->register_method({
>  	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);
> +	my $mac_addr = $config->{wol_mac};
> +	die "No wake on LAN mac address defined for node '$param->{node}'!\n"
> +	    if !defined($mac_addr);
>  
> -	my $prefix = chr(0xff) x 6;
> -	my @mac = split(':', $mac_addr);
> -	my $mac = pack('H*', join('', @mac));
> -	my $packet = $prefix . $mac x 16;
> +	$mac_addr =~ s/://;
> +	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) 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";
> -	send($sock, $packet, 0, $to) or die "Unable to send wake on LAN magic packet!\n";
> +	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";
> +	send($sock, $packet, 0, $to) or
> +	    die "Unable to send wake on LAN magic packet!\n";
>  	close($sock);
>  
>  	return undef;
> diff --git a/PVE/NodeConfig.pm b/PVE/NodeConfig.pm
> index 3cc8ba48..456886f4 100644
> --- a/PVE/NodeConfig.pm
> +++ b/PVE/NodeConfig.pm
> @@ -61,7 +61,7 @@ my $confdesc = {
>  	description => 'Node description/comment.',
>  	optional => 1,
>      },
> -    mac => {
> +    wol_mac => {
>  	type => 'string',
>  	description => 'MAC address for wake on LAN',
>  	pattern => '^([0-9a-fA-F]{2}:){5}([0-9a-fA-F]{2})$',
>