Skip to content

Conversation

@adrianlasota
Copy link
Contributor

@adrianlasota adrianlasota commented Sep 2, 2025

This pull request introduces a new remote shell (RShell) connection mechanism for executing commands on remote systems via an HTTP-based protocol. The main changes include the addition of a robust RShellConnection class, a Flask-based RShell server, and a sample RShell client for UEFI shell environments. These components work together to allow remote command execution and result retrieval over HTTP, supporting both Linux and UEFI environments.

Key changes:

RShell Connection Implementation

  • Added the RShellConnection class in mfd_connect/rshell.py, which manages the lifecycle of RShell servers and clients, handles command execution via HTTP requests, and provides logging and error handling for unsupported features.
  • Integrated support for starting a local RShell server process and health-checking its status before establishing connections.

RShell Server

  • Introduced mfd_connect/rshell_server.py, a Flask-based HTTP server that manages command queues per client, handles command dispatch and result collection, and provides health check endpoints.
  • Implemented endpoints for executing commands, posting results/exceptions, and tracking client connections and command status.

RShell Client

  • Added mfd_connect/rshell_client.py, a sample HTTP client application designed for UEFI shell environments, which polls the RShell server for commands, executes them, and posts results or exceptions back to the server.
  • Included a custom sleep implementation to support environments where standard Python sleep is unavailable (e.g., UEFI shell).

Example Usage

  • Provided examples/rshell_example.py demonstrating how to instantiate and use the new RShellConnection to execute remote commands and manage connections.

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@adrianlasota adrianlasota requested a review from Copilot September 3, 2025 07:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a remote shell (RShell) capability that enables HTTP-based command execution between a Python client and Flask server, with specific support for UEFI shell environments.

Key changes:

  • Added RShellConnection class for managing remote command execution via HTTP
  • Implemented Flask-based server for handling command queues and client connections
  • Created UEFI-compatible HTTP client with specialized handling for UEFI shell limitations

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.

File Description
mfd_connect/rshell_server.py Flask REST server managing command queues, health checks, and result collection
mfd_connect/rshell_client.py HTTP client designed for UEFI shell with polling and command execution capabilities
mfd_connect/rshell.py RShellConnection class providing remote command execution interface
examples/rshell_example.py Usage example demonstrating RShellConnection functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@adrianlasota adrianlasota requested a review from Copilot November 12, 2025 13:53
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

1 similar comment
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

Signed-off-by: Lasota, Adrian <adrian.lasota@intel.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 5 out of 7 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


if __name__ == "__main__":
print("Starting Flask REST server...")
app.run(host="0.0.0.0", port=80)
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running Flask server on privileged port 80 without authentication or authorization checks exposes all endpoints to any network client. Consider using a non-privileged port (e.g., 8080) or implementing authentication middleware to verify client requests.

Suggested change
app.run(host="0.0.0.0", port=80)
app.run(host="0.0.0.0", port=8080)

Copilot uses AI. Check for mistakes.
"""
print("Getting output for command ID:", command_id)
print(f"Waiting for output {timeout} seconds")
timeout = timeout + 5 # add time for client loop waiting
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number '5' lacks explanation. Consider documenting why 5 seconds are added to the timeout or defining it as a named constant (e.g., CLIENT_LOOP_BUFFER = 5).

Copilot uses AI. Check for mistakes.
os_name = os.name


def _sleep(interval): # noqa: ANN001, ANN202
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The busy-wait implementation of _sleep will consume 100% CPU during sleep intervals. This is inefficient even in UEFI environments. Consider using a less aggressive polling interval or documenting this performance trade-off.

Copilot uses AI. Check for mistakes.
def __init__(
self,
ip: str | IPv4Address | IPv6Address,
server_ip: str | IPv4Address | IPv6Address | None = "127.0.0.1",
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value '127.0.0.1' has type str but parameter accepts None. The logic at line 56 checks if server_ip which treats empty string as falsy. Consider using None as default and explicitly setting to '127.0.0.1' when None, or adjust the conditional to if server_ip is None.

Copilot uses AI. Check for mistakes.
Comment on lines +4 to +5
logging.basicConfig(level=logging.DEBUG)
from mfd_connect.rshell import RShellConnection
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import statement should appear before code execution. Move the import of RShellConnection to line 4, before the logging.basicConfig() call.

Suggested change
logging.basicConfig(level=logging.DEBUG)
from mfd_connect.rshell import RShellConnection
from mfd_connect.rshell import RShellConnection
logging.basicConfig(level=logging.DEBUG)

Copilot uses AI. Check for mistakes.
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

5 similar comments
@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

@mfd-intel-bot
Copy link

We don't publish DEVs .whl.
To build .whl, run 'pip install git+https://github.com/intel/mfd-connect@MFD-7178'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants