You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/12/28 17:53:48 UTC
[GitHub] [guacamole-server] necouchman opened a new pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
necouchman opened a new pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320
----------------------------------------------------------------
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320#discussion_r549836939
##########
File path: src/libguac/guacamole/wol-constants.h
##########
@@ -39,13 +39,5 @@
*/
#define GUAC_WOL_PACKET_SIZE 102
-/**
- * The port number that the magic packet should contain as the destination. In
- * reality this doesn't matter all that much, since the packet is not usually
- * processed by a full IP stack, but defining one is considered a standard
- * practice.
- */
-#define GUAC_WOL_PORT 9
-
Review comment:
It gets moved over to the `defaults.h` file as a default value, since we're now allowing it to be specified. UDP port 9 is officially the "Discard Protocol", and has been unofficially adopted for Wake-on-LAN. So, it technically isn't the "Wake-on-LAN" port, just the one that gets used most times.
I'm happy to back this change out and just leave it as is, using this value as the default, I was just recalling the discussion we had about defaults when originally implementing this support and trying to put it in the proper place.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper merged pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
Posted by GitBox <gi...@apache.org>.
mike-jumper merged pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320
----------------------------------------------------------------
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320#discussion_r550337413
##########
File path: src/libguac/guacamole/wol-constants.h
##########
@@ -39,13 +39,5 @@
*/
#define GUAC_WOL_PACKET_SIZE 102
-/**
- * The port number that the magic packet should contain as the destination. In
- * reality this doesn't matter all that much, since the packet is not usually
- * processed by a full IP stack, but defining one is considered a standard
- * practice.
- */
-#define GUAC_WOL_PORT 9
-
Review comment:
Okay, sounds good - I've moved it back and confirmed that everything builds correctly.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320#discussion_r549837028
##########
File path: src/libguac/guacamole/wol.h
##########
@@ -42,11 +42,15 @@
* @param broadcast_addr
* The broadcast address to which to send the magic Wake-on-LAN packet.
*
+ * @param broadcast_port
+ * The UDP port to use when sending the WoL packet.
+ *
* @return
* Zero if the packet is successfully sent to the destination; non-zero
* if the packet cannot be sent.
*/
-int guac_wol_wake(const char* mac_addr, const char* broadcast_addr);
+int guac_wol_wake(const char* mac_addr, const char* broadcast_addr,
+ const unsigned short broadcast_port);
Review comment:
Yeah, that's a good point. I'll rename it.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320#discussion_r550331903
##########
File path: src/libguac/guacamole/wol-constants.h
##########
@@ -39,13 +39,5 @@
*/
#define GUAC_WOL_PACKET_SIZE 102
-/**
- * The port number that the magic packet should contain as the destination. In
- * reality this doesn't matter all that much, since the packet is not usually
- * processed by a full IP stack, but defining one is considered a standard
- * practice.
- */
-#define GUAC_WOL_PORT 9
-
Review comment:
So long as the port is a standard, I think this is still the correct place. If this were not a standard port, and we just chose that number because we like it as a default for our own protocol plugins, then the internal `defaults.h` would make more sense.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #320: GUACAMOLE-1245: Add support for specifying Wake-on-LAN port.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #320:
URL: https://github.com/apache/guacamole-server/pull/320#discussion_r549827673
##########
File path: src/libguac/guacamole/wol-constants.h
##########
@@ -39,13 +39,5 @@
*/
#define GUAC_WOL_PACKET_SIZE 102
-/**
- * The port number that the magic packet should contain as the destination. In
- * reality this doesn't matter all that much, since the packet is not usually
- * processed by a full IP stack, but defining one is considered a standard
- * practice.
- */
-#define GUAC_WOL_PORT 9
-
Review comment:
Why remove the port constant from libguac?
##########
File path: src/libguac/guacamole/wol.h
##########
@@ -42,11 +42,15 @@
* @param broadcast_addr
* The broadcast address to which to send the magic Wake-on-LAN packet.
*
+ * @param broadcast_port
+ * The UDP port to use when sending the WoL packet.
+ *
* @return
* Zero if the packet is successfully sent to the destination; non-zero
* if the packet cannot be sent.
*/
-int guac_wol_wake(const char* mac_addr, const char* broadcast_addr);
+int guac_wol_wake(const char* mac_addr, const char* broadcast_addr,
+ const unsigned short broadcast_port);
Review comment:
While the address for the WoL packet is expected to be the broadcast address, I don't believe there is such a thing as a "broadcast port".
##########
File path: src/protocols/telnet/settings.c
##########
@@ -63,6 +63,7 @@ const char* GUAC_TELNET_CLIENT_ARGS[] = {
"wol-send-packet",
"wol-mac-addr",
"wol-broadcast-addr",
+ "wol-broadcast-port",
Review comment:
As with the `broadcast_port` parameter, I suspect that `wol-broadcast-port` may be technically incorrect, and that something like `wol-port` would be preferable.
----------------------------------------------------------------
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