Skip to content

Support for direct-streamlocal@openssh.com UNIX socket connection#216

Closed
gjalves wants to merge 4 commits intolibssh2:masterfrom
gjalves:master
Closed

Support for direct-streamlocal@openssh.com UNIX socket connection#216
gjalves wants to merge 4 commits intolibssh2:masterfrom
gjalves:master

Conversation

@gjalves
Copy link
Copy Markdown

@gjalves gjalves commented Oct 16, 2017

This patch allow to use direct-streamlocal service from OpenSSH 6.7, that allows UNIX socket connections.

@stale
Copy link
Copy Markdown

stale bot commented Mar 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 21, 2019
@stale stale bot closed this Apr 11, 2019
@willco007 willco007 reopened this Nov 4, 2019
@stale stale bot removed the stale label Nov 4, 2019
@Red-M
Copy link
Copy Markdown

Red-M commented Nov 10, 2019

This looks like the travis build failed due to some internal issue in the test suite?

@gjalves
Copy link
Copy Markdown
Author

gjalves commented Nov 10, 2019

I think travis is mumbling about cosmetic aspects, like line lengths and misplaced spaces. I'll review this patch to overcome those errors.

@Red-M
Copy link
Copy Markdown

Red-M commented Nov 10, 2019

Yeah, I can see that now, its failing on the command perl src/checksrc.pl.

@Red-M
Copy link
Copy Markdown

Red-M commented Nov 25, 2019

Is this allowed to be merged now?

Comment thread src/channel.c
return LIBSSH2_ERROR_BAD_USE;

if(!channel->session)
return LIBSSH2_ERROR_SOCKET_RECV;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain the reason for this check? As far as I understand, there should never be a channel with a NULL session.

Also, the choice of error code here seems odd. Why return this error code?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This check is unrelated with this patch and was wrongly pushed. I've stomped with channel->session == NULL and put this check to came along without crash, but I do not know why this error happened. I got the same impression that is an impossible situation, but I do not bothered to go deeper.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, should we delete this check if it isn't related to this PR, or is this something we need?

@stale
Copy link
Copy Markdown

stale bot commented May 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 4, 2020
@willco007 willco007 removed the stale label May 4, 2020
@willco007
Copy link
Copy Markdown
Member

Could you please add documentation for this new API when you get a chance? Thanks!

@stale
Copy link
Copy Markdown

stale bot commented Sep 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 12, 2020
@stale stale bot removed the stale label Sep 13, 2020
@Red-M
Copy link
Copy Markdown

Red-M commented Dec 20, 2020

@willco007 Sorry to be a pest, but this looks like its done now and can be merged?

@doorsdown
Copy link
Copy Markdown
Contributor

Agreed this looks ready for merging.

Comment thread src/channel.c
@willco007
Copy link
Copy Markdown
Member

The libssh2_channel_direct_streamlocal_ex.3 man page needs to also be added to the CMakeLists.txt file. But it looks good after that is resolved. Thanks!

@stale
Copy link
Copy Markdown

stale bot commented Jun 16, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 16, 2021
@willco007
Copy link
Copy Markdown
Member

I'd like to land this, but we're still waiting for final update to the CMakeLists.

@stale
Copy link
Copy Markdown

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@willco007 willco007 removed the stale label May 24, 2022
@lmtr0
Copy link
Copy Markdown

lmtr0 commented Jan 3, 2023

Hello there good day!

I'd like to land this, but we're still waiting for final update to the CMakeLists.

is the rewrite ready?

@vszakats
Copy link
Copy Markdown
Member

vszakats commented Apr 5, 2023

This would need a rebase and docs/Makefile.am update due to conflicts, plus adding the man page also to docs/CMakeLists.txt. With these and clearing the question with the unrelated if() condition and the minor alignment nit, this is good for merge.

Comment thread include/libssh2.h
Comment on lines +721 to +722
const char *socket_path,
const char *shost, int sport);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd be nice to align these.

Suggested change
const char *socket_path,
const char *shost, int sport);
const char *socket_path,
const char *shost, int sport);

vszakats added a commit to vszakats/libssh2 that referenced this pull request Apr 10, 2023
vszakats added a commit that referenced this pull request Apr 10, 2023
This patch allow to use direct-streamlocal service from OpenSSH 6.7,
that allows UNIX socket connections.

Mods:
- delete unrelated condition:
  Ref: #216 (comment)
- rebase on master, whitespace updates.

Patch-by: @gjalves Gustavo Junior Alves

Closes #216
Closes #632
Closes #945
agreppin pushed a commit to agreppin/libssh2 that referenced this pull request Jul 14, 2024
…bssh2#945)

This patch allow to use direct-streamlocal service from OpenSSH 6.7,
that allows UNIX socket connections.

Mods:
- delete unrelated condition:
  Ref: libssh2#216 (comment)
- rebase on master, whitespace updates.

Patch-by: @gjalves Gustavo Junior Alves

Closes libssh2#216
Closes libssh2#632
Closes libssh2#945
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants