[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #267: GUACAMOLE-513: Implement Wake-on-LAN support in libguac

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #267:
URL: https://github.com/apache/guacamole-server/pull/267#discussion_r436412025



##########
File path: src/libguac/guacamole/wol-constants.h
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_WOL_CONSTANTS_H
+#define GUAC_WOL_CONSTANTS_H
+
+/**
+ * Header file that provides constants and defaults related to libguac
+ * Wake-on-LAN support.
+ * 
+ * @file wol-constants.h
+ */
+
+/**
+ * The value for the local IPv4 broadcast address.
+ */
+#define GUAC_WOL_LOCAL_IPV4_BROADCAST "255.255.255.255"
+
+/**
+ * The number of seconds to wait after sending the Wake-on-LAN packet
+ * for the destination host to start responding.
+ */
+#define GUAC_WOL_BOOT_WAIT_TIME 60

Review comment:
       I'm still not sure about this.
   
   The other constants make total sense; `GUAC_WOL_LOCAL_IPV4_BROADCAST` is exactly that, as are `GUAC_WOL_PACKET_SIZE` and `GUAC_WOL_PORT`. Each is a standard value which any WoL implementation should reasonably provide as a reusable constant.
   
   `GUAC_WOL_BOOT_WAIT_TIME` isn't a standard value nor a standard part of WoL. My understanding from reading the code thus far is that this is just a constant which we are using internally to avoid having to define such a constant within each protocol. I agree that we should aim to be DRY and that defining such a constant somewhere makes sense ... but I don't think the public API of libguac is the place for it. I think it's outside the scope of concerns for the core, public library of guacamole-server.
   
   There is a place where we have been placing internally-reusable code that isn't part of the public API (`src/common/`). If it goes anywhere, I think that would be the place for it, even it it's just something like `src/common/common/defaults.h`. Such a header might even serve as a place to receive other default values that have otherwise been duplicated across each protocol implementation but _could_ be common.
   
   If you think this constant does belong in the public libguac API, I think I can be swayed, but I'd like to read through the reasoning. As it stands, I currently can't see it as something within scope.

##########
File path: src/libguac/wol.c
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+
+#include "guacamole/wol.h"
+
+/**
+ * Generate the magic Wake-on-LAN (WoL) packet for the specified MAC address
+ * and place it in the character array.
+ * 
+ * @param packet
+ *     The character array that will contain the generated packet.
+ * 
+ * @param mac_address
+ *     The unsigned int representation of the MAC address to place in the packet.
+ */
+static void __guac_wol_create_magic_packet(unsigned char packet[],
+        unsigned int mac_address[]) {
+    
+    int i;
+    unsigned char mac[6];
+    
+    /* Concurrently fill the first part of the packet with 0xFF, and copy the
+     MAC address from the int array to the char array. */
+    for (i = 0; i < 6; i++) {
+        packet[i] = 0xFF;
+        mac[i] = mac_address[i];
+    }
+    
+    /* Copy the MAC address contents into the char array that is storing
+     the rest of the packet. */
+    for (i = 1; i <= 16; i++) {
+        memcpy(&packet[i * 6], &mac, 6 * sizeof(unsigned char));
+    }
+    
+}
+
+/**
+ * Send the magic Wake-on-LAN (WoL) packet to the specified broadcast address,
+ * returning the number of bytes sent, or zero if any error occurred and nothing
+ * was sent.
+ * 
+ * @param broadcast_addr
+ *     The broadcast address to which to send the magic WoL packet.
+ * 
+ * @param packet
+ *     The magic WoL packet to send.
+ * 
+ * @return 
+ *     The number of bytes sent, or zero if nothing could be sent.
+ */
+static ssize_t __guac_wol_send_packet(const char* broadcast_addr,
+        unsigned char packet[]) {
+    
+    struct sockaddr_in wol_dest;
+    int wol_socket;
+    
+    /* Determine the IP version, starting with IPv4. */
+    wol_dest.sin_port = htons(GUAC_WOL_PORT);
+    wol_dest.sin_family = AF_INET;
+    if (inet_pton(AF_INET, broadcast_addr, &(wol_dest.sin_addr)) == 0) {
+        wol_dest.sin_family = AF_INET6;
+        if (inet_pton(AF_INET6, broadcast_addr, &(wol_dest.sin_addr)) == 0)
+            return 0;
+    }
+    
+    /* Set up socket for IPv4 broadcast. */
+    if (wol_dest.sin_family == AF_INET) {
+        int wol_bcast = 1;
+        wol_socket = socket(AF_INET, SOCK_DGRAM, 0);
+
+        /* Attempt to set broadcast; exit with error if this fails. */
+        if (setsockopt(wol_socket, SOL_SOCKET, SO_BROADCAST, &wol_bcast,
+                sizeof(wol_bcast)) < 0)
+            return 0;
+    }
+    
+    /* Set up socket for IPv6 multicast. */
+    else {
+        wol_socket = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP);
+        
+        /* Stick to a single hop for now. */
+        int hops = 1;
+        
+        if (setsockopt(wol_socket, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops,
+                sizeof(hops)))
+            return 0;
+    }
+    
+    /* Send the packet and return number of bytes sent. */
+    return sendto(wol_socket, packet, GUAC_WOL_PACKET_SIZE, 0,
+            (struct sockaddr*) &wol_dest, sizeof(wol_dest));

Review comment:
       Whether successful or not, does `wol_socket` need to be closed before it goes out of scope to avoid leaking a descriptor?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org