Skip to content

Conversation

bjne
Copy link

@bjne bjne commented Jan 2, 2025

Add a send_eof function similar to:

https://www.php.net/manual/en/function.ssh2-send-eof.php

@terrafrost
Copy link
Member

In-so-far as the coding standards are concerned... the code changes aren't compliant with the PSR-2 standards. Here is the relevant one:

  • Opening braces for methods MUST go on the next line, and closing braces MUST go on the next line after the body.

The comments need tweaking as well. They follow the DocBlock standards so instead of this:

    /** Send EOF on a channel
     *
     * Sends an EOF to the stream; this is typically used to close standard
     * input, while keeping output and error alive.
     *
     * @param int|null $channel Channel id returned by
     * self::getInteractiveChannelId() @return void
     */

You should have this:

    /**
     * Send EOF on a channel
     *
     * Sends an EOF to the stream; this is typically used to close standard
     * input, while keeping output and error alive.
     *
     * @param int|null $channel Channel id returned by self::getInteractiveChannelId()
     * @return void
     */

I mean, I guess indenting a multi line annotation would be sufficient as well, but having it all on one line works, too. Additionally, each annotation should be on it's own line.

Finally... altho phpseclib uses a mix of camelCase and under_line's, for the most part, phpseclib uses camelCase. So if you could change the function name from send_eof to sendEOF that'd be great.

@terrafrost
Copy link
Member

Functionally, calling this new method could result in some weird before if a channel is later closed. Like right now NET_SSH2_MSG_CHANNEL_EOF would probably sent twice, once when this function is called and once when the channel closure actually happens. Some additional checks prob ought to be put in place to make sure that doesn't happen.

My first thought is that maybe this could be achieved by hijacking $this->channel_status[$channel]. Right now when that's set to NET_SSH2_MSG_CHANNEL_EOF that essentially seems to mean that NET_SSH2_MSG_CHANNEL_CLOSE has been sent but the response packet hasn't yet been received. And if $this->channel_status[$channel] is set to NET_SSH2_MSG_CHANNEL_EOF that means that the channel really is closed. But maybe we could just do unset($this->channel_status[$channel]) instead for that and use NET_SSH2_MSG_CHANNEL_EOF to denote that EOF has been sent and NET_SSH2_MSG_CHANNEL_CLOSE to denote that NET_SSH2_MSG_CHANNEL_CLOSE has been sent and we're still waiting for a response.

All of that stuff is prob a bit much to expect for a first time contributor, however. I'd say: go ahead and make the code changes I requested and then I can pull that off into my own branch and do the rest.

terrafrost added a commit to terrafrost/phpseclib that referenced this pull request Jan 16, 2025
terrafrost added a commit to terrafrost/phpseclib that referenced this pull request Jan 16, 2025
@terrafrost terrafrost merged commit 87e8e42 into phpseclib:3.0 Jan 16, 2025
41 of 42 checks passed
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.

2 participants