Fix missing parameter to call to get_wol_mac_addresses for WOL featur… #403

kylemhall · 2025-08-27T10:53:31Z

Description by Korbit AI

What change is being made?

Update socket creation logic in the wakeonlan function to correctly pass context when fetching MAC addresses and update GitHub Actions artifacts version in the workflow configuration.

Why are these changes being made?

The previous implementation failed to include necessary context when calling get_wol_mac_addresses, leading to potential failures in fetching correct MAC addresses needed for WOL functionality. The GitHub Actions artifacts versioning was updated to ensure compatibility and leverage improvements from newer releases.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

korbit-ai

I've completed my review and didn't find any issues... but I did find this horse.

            .''
  ._.-.___.' (`\
 //(        ( `'
'/ )\ ).__. )
' <' `\ ._/'\
   `   \     \

Files scanned

File Path	Reviewed
lib/Libki/Clients.pm	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

codereviewbot-ai · 2025-08-27T10:54:02Z

lib/Libki/Clients.pm

+    my @mac_addresses = get_wol_mac_addresses($c);

    foreach my $mac_address (@mac_addresses) {
        my $socket = new IO::Socket::INET( Proto => 'udp' )


Creating a new socket for each MAC address inside the loop can lead to performance issues, especially if the number of MAC addresses is large. Consider creating a single socket outside the loop and reusing it for all MAC addresses to improve resource utilization and performance.

Recommended Change:

my $socket = new IO::Socket::INET( Proto => 'udp' ) or $c->log()->fatal("ERROR in Socket Creation : $!\n"); foreach my $mac_address (@mac_addresses) { # Use the socket to send packets }

kylemhall · 2025-08-27T10:57:17Z

/korbit-generate-pr-description

korbit-ai · 2025-08-27T10:57:20Z

@kylemhall I am looking at your pull request. The description will be updated shortly. In the meantime, please do not edit the description until I have finished writing mine.

codereviewbot-ai · 2025-08-27T10:57:22Z

lib/Libki/Clients.pm

+        or $c->log()->fatal("ERROR in Socket Creation : $!\n");

-        if ($socket) {
+    if ($socket) {
+        foreach my $mac_address (@mac_addresses) {
            $c->log()->debug("Sending magic packet to $mac_address at $host:$port");
            $mac_address =~ s/://g;
-            my $packet = pack('C6H*', 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, $mac_address x 16);
+            my $packet = pack( 'C6H*', 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, $mac_address x 16 );

-            setsockopt($socket, SOL_SOCKET, SO_BROADCAST, 1);
-            $success = 0 unless send($socket, $packet, 0, $sockaddr);
+            setsockopt( $socket, SOL_SOCKET, SO_BROADCAST, 1 );
+            $success = 0 unless send( $socket, $packet, 0, $sockaddr );
            $socket->close;
-        } else {
-            $success = 0;
        }
    }


The error handling for socket creation is not stopping the execution after a fatal error, which could lead to further issues if the socket is not successfully created. The code logs a fatal error but continues to execute the subsequent code inside the if ($socket) block.

Recommended Change:
Wrap the code that depends on the socket inside an else block to ensure it only executes if the socket is successfully created:

my $socket = new IO::Socket::INET( Proto => 'udp' ) or $c->log()->fatal("ERROR in Socket Creation : $!"); if ($socket) { # existing code for using the socket } else { return 0; # or handle the error appropriately }

This is terrible advice, ignoring.

codereviewbot-ai · 2025-08-27T10:57:22Z

lib/Libki/Clients.pm

+            setsockopt( $socket, SOL_SOCKET, SO_BROADCAST, 1 );
+            $success = 0 unless send( $socket, $packet, 0, $sockaddr );


The broadcast option for the socket is set inside the loop for each MAC address, which is inefficient and redundant. This option only needs to be set once after the socket is created.

Recommended Change:
Move the setsockopt call to just after the socket creation, outside the loop, to improve efficiency and clarity:

my $socket = new IO::Socket::INET( Proto => 'udp' ); setsockopt( $socket, SOL_SOCKET, SO_BROADCAST, 1 ); foreach my $mac_address (@mac_addresses) { # send packets }

…#402

…#402 Creating a new socket for each MAC address inside the loop can lead to performance issues, especially if the number of MAC addresses is large. Consider creating a single socket outside the loop and reusing it for all MAC addresses to improve resource utilization and performance.

codereviewbot-ai · 2025-08-27T11:21:57Z

lib/Libki/Clients.pm

-            setsockopt($socket, SOL_SOCKET, SO_BROADCAST, 1);
-            $success = 0 unless send($socket, $packet, 0, $sockaddr);
+            $success = 0 unless send( $socket, $packet, 0, $sockaddr );
            $socket->close;


The socket is being closed inside the loop after sending a packet to each MAC address. This approach is inefficient as it requires reopening the socket for each MAC address, which can significantly degrade performance when dealing with multiple MAC addresses.

Recommended Change:
Move the socket closure outside of the loop so that the socket is opened once, used for sending all packets, and then closed:

foreach my $mac_address (@mac_addresses) { # send packets } $socket->close;

codereviewbot-ai · 2025-08-27T11:21:57Z

lib/Libki/Clients.pm

+            $success = 0 unless send( $socket, $packet, 0, $sockaddr );
            $socket->close;


The function uses a success flag to indicate if all packets were sent successfully, but it does not provide any mechanism to identify which packet failed or why. This lack of detailed error reporting can make debugging and operational monitoring difficult.

Recommended Change:
Enhance error handling by logging detailed error messages for each failed packet transmission attempt. This can help in identifying the specific MAC address and the reason for the failure, improving maintainability and support:

unless (send( $socket, $packet, 0, $sockaddr )) { $c->log()->error("Failed to send magic packet to $mac_address: $!"); $success = 0; }

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Misleading Fatal Error Handling ▹ view	✅ Fix detected

Files scanned

File Path	Reviewed
lib/Libki/Clients.pm	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

lib/Libki/Clients.pm

+    my $socket = new IO::Socket::INET( Proto => 'udp' )
+        or $c->log()->fatal("ERROR in Socket Creation : $!\n");


codereviewbot-ai · 2025-08-27T11:31:27Z

lib/Libki/Clients.pm

+    my $socket = new IO::Socket::INET( Proto => 'udp' )
+        or $c->log()->fatal("ERROR in Socket Creation : $!\n");

-    foreach my $mac_address (@mac_addresses) {
-        my $socket = new IO::Socket::INET( Proto => 'udp' )
-            or $c->log()->fatal("ERROR in Socket Creation : $!\n");
+    my $socket = new IO::Socket::INET( Proto => 'udp' )


The socket is created twice consecutively (lines 22 and 25) which is likely a mistake. This could lead to resource leaks or unexpected behavior as the first socket created is immediately overwritten by the second one. This redundancy should be removed to ensure efficient resource usage and to avoid confusion.

Recommended Change:
Remove the duplicate socket creation to ensure only one socket is created and used:

my $socket = new IO::Socket::INET( Proto => 'udp' ) or do { $c->log()->error("Socket Creation failed: $!"); return 0; };

codereviewbot-ai · 2025-08-27T11:31:27Z

lib/Libki/Clients.pm

+        or $c->log()->fatal("ERROR in Socket Creation : $!\n");

-    foreach my $mac_address (@mac_addresses) {
-        my $socket = new IO::Socket::INET( Proto => 'udp' )
-            or $c->log()->fatal("ERROR in Socket Creation : $!\n");
+    my $socket = new IO::Socket::INET( Proto => 'udp' )
+        or do {
+        $c->log()->error("Socket Creation failed: $!\n");
+        $success = 0;


The error handling for socket creation logs an error but does not stop the execution, which could lead to further issues if the socket is not successfully created. The use of fatal in line 23 and error in line 27 should be consistent and should prevent further execution to avoid operating on an undefined socket.

Recommended Change:
Ensure that the execution stops after logging the error by returning immediately if the socket cannot be created:

my $socket = new IO::Socket::INET( Proto => 'udp' ) or do { $c->log()->error("Socket Creation failed: $!"); return 0; };

codereviewbot-ai · 2025-08-27T11:32:25Z

lib/Libki/Clients.pm

+        or do {
+        $c->log()->error("Socket Creation failed: $!\n");
+        $success = 0;
+        };


The error handling after attempting to create a socket (lines 23-26) logs an error but does not stop the execution, which could lead to further issues if the socket is not successfully created. This inconsistency in error handling can lead to operations being performed on an undefined socket, potentially causing runtime errors.

Recommended Change:
Ensure that the execution stops after logging the error by returning immediately if the socket cannot be created:

my $socket = new IO::Socket::INET( Proto => 'udp' ) or do { $c->log()->error("Socket Creation failed: $!"); return 0; };

codereviewbot-ai · 2025-08-27T11:32:25Z

lib/Libki/Clients.pm

-        my $socket = new IO::Socket::INET( Proto => 'udp' )
-            or $c->log()->fatal("ERROR in Socket Creation : $!\n");
+    if ( $success && $socket ) {
+        setsockopt( $socket, SOL_SOCKET, SO_BROADCAST, 1 );


The broadcast option for the socket is set inside the loop for each MAC address (line 29), which is inefficient and redundant. This option only needs to be set once after the socket is created. Setting this option multiple times is not necessary and can degrade performance.

Recommended Change:
Move the setsockopt call to just after the socket creation, outside the loop, to improve efficiency and clarity:

my $socket = new IO::Socket::INET( Proto => 'udp' ); setsockopt( $socket, SOL_SOCKET, SO_BROADCAST, 1 ); foreach my $mac_address (@mac_addresses) { # send packets }

korbit-ai bot reviewed Aug 27, 2025

View reviewed changes

codereviewbot-ai bot reviewed Aug 27, 2025

View reviewed changes

kylemhall added 4 commits August 27, 2025 06:57

GitHub actions fail for pull requests #397

42411f5

Fix missing parameter to call to get_wol_mac_addresses for WOL feature …

2d5c2b1

…#402

For WOL feature, move call to setsockopt out of the look #402

7d6e68c

kylemhall force-pushed the issue_402 branch from 6876061 to 7d6e68c Compare August 27, 2025 11:21

codereviewbot-ai bot reviewed Aug 27, 2025

View reviewed changes

korbit-ai bot reviewed Aug 27, 2025

View reviewed changes

lib/Libki/Clients.pm Outdated

Comment on lines 23 to 24

my $socket = new IO::Socket::INET( Proto => 'udp' )

or $c->log()->fatal("ERROR in Socket Creation : $!\n");

This comment was marked as resolved.

Sign in to view

codereviewbot-ai bot reviewed Aug 27, 2025

View reviewed changes

For WOL feature, improve error handling #402

8e5f495

kylemhall force-pushed the issue_402 branch from e2f3501 to 8e5f495 Compare August 27, 2025 11:32

codereviewbot-ai bot reviewed Aug 27, 2025

View reviewed changes

kylemhall merged commit a98ada9 into master Aug 27, 2025
5 checks passed

kylemhall deleted the issue_402 branch August 27, 2025 11:58

		setsockopt( $socket, SOL_SOCKET, SO_BROADCAST, 1 );
		$success = 0 unless send( $socket, $packet, 0, $sockaddr );

		$success = 0 unless send( $socket, $packet, 0, $sockaddr );
		$socket->close;

		my $socket = new IO::Socket::INET( Proto => 'udp' )
		or $c->log()->fatal("ERROR in Socket Creation : $!\n");

Fix missing parameter to call to get_wol_mac_addresses for WOL featur… #403

Fix missing parameter to call to get_wol_mac_addresses for WOL featur… #403

Uh oh!

Conversation

kylemhall commented Aug 27, 2025 • edited by korbit-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description by Korbit AI

What change is being made?

Why are these changes being made?

Uh oh!

korbit-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

kylemhall commented Aug 27, 2025

Uh oh!

korbit-ai bot commented Aug 27, 2025

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

kylemhall Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

This comment was marked as resolved.

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kylemhall commented Aug 27, 2025 •

edited by korbit-ai bot

Loading

korbit-ai bot left a comment •

edited

Loading