X2Go Bug report logs - #1469
X2Go Client connection issue (Socket error)

version graph

Package: x2goclient; Maintainer for x2goclient is X2Go Developers <x2go-dev@lists.x2go.org>; Source for x2goclient is src:x2goclient.

Reported by: Melroy van den Berg <melroy89@protonmail.com>

Date: Mon, 18 May 2020 18:55:02 UTC

Severity: normal

Tags: pending

Found in version 4.1.2.2

Fixed in version 4.1.2.3

Reply or subscribe to this bug.

Toggle useless messages

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Mon, 18 May 2020 18:55:02 GMT) (full text, mbox, link).


Acknowledgement sent to Melroy van den Berg <melroy89@protonmail.com>:
New Bug report received and forwarded. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Mon, 18 May 2020 18:55:02 GMT) (full text, mbox, link).


Message #5 received at submit@bugs.x2go.org (full text, mbox, reply):

From: Melroy van den Berg <melroy89@protonmail.com>
To: "submit@bugs.x2go.org" <submit@bugs.x2go.org>
Subject: X2Go Client connection issue (Socket error)
Date: Mon, 18 May 2020 18:54:06 +0000
[Message part 1 (text/plain, inline)]
Package: x2goclient
Version: 4.1.2.2

I often get an error message when I try to connect to my Debian buster docker image, running x2goserver and XFCE, via the X2Go Client on my host machine.
It will give the following error message (when using the --debug flag on the x2goclient):

x2go-DEBUG-../src/sshmasterconnection.cpp:2071> "ssh_channel_open_session failed": "Socket error: No such file or directory"
x2go-DEBUG-../src/sshprocess.cpp:478> I/O error: "ssh_channel_open_session failed."" - Socket error: No such file or directory" (0).

I created a detailed video, I posted it on YouTube, please take a look at the video:
https://youtu.be/LAlLtBNTIUo

Logging details, reproducibility and more is explained in the video itself. You'll notice that I can sometimes connect to the XFCE session and sometimes I can't. Although connecting via the ssh cli command is always working without any issues. Meaning there is really some kind of bug in the X2Go Client. I'm afraid.

Again, I really hope somebody could help me in finding the root-cause and fixing the issue.
Since I really love X2Go. And I would like to use this tool in production.
I'm also a software developer so maybe I can help debugging the problem.

Similar (same) bug is reported in the past (from 2019): https://lists.x2go.org/pipermail/x2go-user/2019-March/005512.html
All the details regarding this Docker image and setup can be found in this git repo: https://gitlab.melroy.org/melroy/xfcevdi

See attachments for additional debug logs of the client itself. One when successful and one during the error.

Thanks in advance!

Kind regards,
Melroy van den Berg

Ps. You can contact me by mail of course. But I can understand that debugging this issue would be preferable be done via VoIP for example.
[Message part 2 (text/html, inline)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Mon, 18 May 2020 21:05:02 GMT) (full text, mbox, link).


Acknowledgement sent to Melroy van den Berg <melroy89@protonmail.com>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Mon, 18 May 2020 21:05:02 GMT) (full text, mbox, link).


Message #10 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Melroy van den Berg <melroy89@protonmail.com>
To: "1469@bugs.x2go.org" <1469@bugs.x2go.org>
Subject: Two additional remarks
Date: Mon, 18 May 2020 21:02:38 +0000
[Message part 1 (text/plain, inline)]
In this additional mail I would like to add two things:
1) Additional attachments for the logs (see the attachments in the mail)
2) Trying to reproduce the same issue in the other client (pyhoca),
I was NOT able to reproduce the issue in pyhoca. Everything worked as expected, meaning it's definitely an issue in the x2goclient v4.1.2.2.

Regarding the pyhoca test, I created a second follow-up video which shows that this works: https://www.youtube.com/watch?v=meOZ3YEfkyM

I can confirm even after stopping and starting pyhoca multiple times, it *always* works via pyhoca. Yet again not via x2goclient. That still gives often the error: Socket error: No such file or directory" (0). See my first mail.

Kind regards,
Melroy van den Berg
[Message part 2 (text/html, inline)]
[error.log (text/x-log, attachment)]
[successful.log (text/x-log, attachment)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Tue, 19 May 2020 02:00:03 GMT) (full text, mbox, link).


Acknowledgement sent to Melroy van den Berg <melroy89@protonmail.com>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Tue, 19 May 2020 02:00:03 GMT) (full text, mbox, link).


Message #15 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Melroy van den Berg <melroy89@protonmail.com>
To: "1469@bugs.x2go.org" <1469@bugs.x2go.org>
Subject: Patch!
Date: Tue, 19 May 2020 01:59:16 +0000
[Message part 1 (text/plain, inline)]
Hi,

I did some refactoring in the sshmasterconnection.cpp file. I think it can definitely use some clean-up and further splitting functions and even into multiple files eventually.
I cloned the master branch of code.x2go.org/x2goclient repo.

The changes I applied are in the attachment of this mail (a git patch file). I think closing previous sessions and connection solved my strange SSH connection is resulting into Socket error: No such file or directory".
Although X2Go client is calling [libssh calls](https://api.libssh.org/master/index.html) (libssh version currently in use runtime on my host is: 0.7.0).
For some reason this vague error message popped-up by libssh, properly a result of previous session being created and not correctly closed. And/or not closed in the right order (/missing API calls), like a good example:

ssh_channel_send_eof(channel);
ssh_channel_close(channel);
ssh_channel_free(channel);

And I created yet again a new YT video showing my result:
https://youtu.be/vmASLJq0CKM
(including some brief explanation what I did so far, I also ran an formatted on the code, because it was very very hard to read with all the wrong indents)

I also made another video showing yet another bug I found regarding the SSH tunneling (during some heavy testing), the only solution was to restart the whole docker image:
https://youtu.be/xEpIyo84fWc (I think this issue is unrelated, but maybe wise to take serious as well)

Again see attachment for the patch (branched from master).

Regards,
Melroy van den Berg
[Message part 2 (text/html, inline)]
[melroy_socket_error.patch (text/x-patch, attachment)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Tue, 19 May 2020 19:50:11 GMT) (full text, mbox, link).


Acknowledgement sent to Mike Gabriel <mike.gabriel@das-netzwerkteam.de>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Tue, 19 May 2020 19:50:13 GMT) (full text, mbox, link).


Message #20 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
To: Melroy van den Berg <melroy89@protonmail.com>, 1469@bugs.x2go.org
Subject: Re: [X2Go-Dev] Bug#1469: Patch!
Date: Tue, 19 May 2020 19:48:33 +0000
[Message part 1 (text/plain, inline)]
Hi Melroy,

On  Di 19 Mai 2020 03:59:16 CEST, Melroy van den Berg wrote:

> Hi,
>
> I did some refactoring in the sshmasterconnection.cpp file. I think  
> it can definitely use some clean-up and further splitting functions  
> and even into multiple files eventually.
> I cloned the master branch of code.x2go.org/x2goclient repo.
>
> The changes I applied are in the attachment of this mail (a git  
> patch file). I think closing previous sessions and connection solved  
> my strange SSH connection is resulting into Socket error: No such  
> file or directory".
> Although X2Go client is calling [libssh  
> calls](https://api.libssh.org/master/index.html) (libssh version  
> currently in use runtime on my host is: 0.7.0).
> For some reason this vague error message popped-up by libssh,  
> properly a result of previous session being created and not  
> correctly closed. And/or not closed in the right order (/missing API  
> calls), like a good example:
>
> ssh_channel_send_eof(channel);
> ssh_channel_close(channel);
> ssh_channel_free(channel);
>
> And I created yet again a new YT video showing my result:
> https://youtu.be/vmASLJq0CKM
> (including some brief explanation what I did so far, I also ran an  
> formatted on the code, because it was very very hard to read with  
> all the wrong indents)
>
> I also made another video showing yet another bug I found regarding  
> the SSH tunneling (during some heavy testing), the only solution was  
> to restart the whole docker image:
> https://youtu.be/xEpIyo84fWc (I think this issue is unrelated, but  
> maybe wise to take serious as well)
>
> Again see attachment for the patch (branched from master).
>
> Regards,
> Melroy van den Berg

Thanks for working on X2Go Client. With your patch, there is a problem  
though. It is barely unreviewable, because you have so many changes  
(also white space changes) in one patch file.

Please clone the X2Go Client Git repo, add atomic changes with good  
commit messages. Keep white-space changes and technical changes fully  
separate (ideally: technical changes first, white-space changes last).

Then export your patches using 'git format-patch  
<last-official-commit>..HEAD' and send us the set of patches instead  
this one bulk commit/patch.

Thanks for your efforts!!!

Mike
-- 

DAS-NETZWERKTEAM
c\o Technik- und Ökologiezentrum Eckernförde
Mike Gabriel, Marienthaler Str. 17, 24340 Eckernförde
mobile: +49 (1520) 1976 148
landline: +49 (4351) 850 8940

GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22  0782 9AF4 6B30 2577 1B31
mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

[Message part 2 (application/pgp-signature, inline)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Tue, 19 May 2020 20:05:44 GMT) (full text, mbox, link).


Acknowledgement sent to Melroy van den Berg <melroy89@protonmail.com>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Tue, 19 May 2020 20:05:46 GMT) (full text, mbox, link).


Message #25 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Melroy van den Berg <melroy89@protonmail.com>
To: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>, "1469@bugs.x2go.org" <1469@bugs.x2go.org>
Subject: Re: [X2Go-Dev] Bug#1469: Patch!
Date: Tue, 19 May 2020 20:00:40 +0000
Hi,

Well.. it was not unnecessary. The code was actually unreadable, wrong indentations and inconsistent throughout the whole file.
I needed to execute this auto-formatter, before I could even begin to understand the code.

In fact, I urge you to execute this on the whole archive. And keep the code quality high via a CI pipeline or something.

I can understand it makes it hard to read. I also fixed other some issues like this double error message during login and all the debug statements.
What would be your approach? I think it's still wise to auto-format the code in consistent matter.

Regards,
Melroy van den Berg

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Op dinsdag, mei 19, 2020 9:48 PM, Mike Gabriel <mike.gabriel@das-netzwerkteam.de> schreef:

> Hi Melroy,
>
> On Di 19 Mai 2020 03:59:16 CEST, Melroy van den Berg wrote:
>
> > Hi,
> > I did some refactoring in the sshmasterconnection.cpp file. I think > it can definitely use some clean-up and further splitting functions > and even into multiple files eventually.
> > I cloned the master branch of code.x2go.org/x2goclient repo.
> > The changes I applied are in the attachment of this mail (a git > patch file). I think closing previous sessions and connection solved > my strange SSH connection is resulting into Socket error: No such > file or directory".
> > Although X2Go client is calling libssh > calls (libssh version > currently in use runtime on my host is: 0.7.0).
> > For some reason this vague error message popped-up by libssh, > properly a result of previous session being created and not > correctly closed. And/or not closed in the right order (/missing API > calls), like a good example:
> > ssh_channel_send_eof(channel);
> > ssh_channel_close(channel);
> > ssh_channel_free(channel);
> > And I created yet again a new YT video showing my result:
> > https://youtu.be/vmASLJq0CKM
> > (including some brief explanation what I did so far, I also ran an > formatted on the code, because it was very very hard to read with > all the wrong indents)
> > I also made another video showing yet another bug I found regarding > the SSH tunneling (during some heavy testing), the only solution was > to restart the whole docker image:
> > https://youtu.be/xEpIyo84fWc (I think this issue is unrelated, but > maybe wise to take serious as well)
> > Again see attachment for the patch (branched from master).
> > Regards,
> > Melroy van den Berg
>
> Thanks for working on X2Go Client. With your patch, there is a problem though. It is barely unreviewable, because you have so many changes (also white space changes) in one patch file.
>
> Please clone the X2Go Client Git repo, add atomic changes with good commit messages. Keep white-space changes and technical changes fully separate (ideally: technical changes first, white-space changes last).
>
> Then export your patches using 'git format-patch <last-official-commit>..HEAD' and send us the set of patches instead this one bulk commit/patch.
>
> Thanks for your efforts!!!
>
> Mike
>
> -----------------------------------
>
> DAS-NETZWERKTEAM
> c\o Technik- und Ökologiezentrum Eckernförde
> Mike Gabriel, Marienthaler Str. 17, 24340 Eckernförde
> mobile: +49 (1520) 1976 148
> landline: +49 (4351) 850 8940
>
> GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31
> mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de



Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Tue, 19 May 2020 20:40:06 GMT) (full text, mbox, link).


Acknowledgement sent to uli42@gmx.de:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Tue, 19 May 2020 20:40:09 GMT) (full text, mbox, link).


Message #30 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Ulrich Sibiller <ulrich.sibiller@gmail.com>
To: Melroy van den Berg <melroy89@protonmail.com>, 1469@bugs.x2go.org
Cc: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Subject: Re: [X2Go-Dev] Bug#1469: Bug#1469: Patch!
Date: Tue, 19 May 2020 22:37:08 +0200
What about taking the original code, run your auto-formatter (what
tool are you using?), make a patch. Then copy your patched version
over the auto-formatted version and create one or more other patches,
depending on your changes. Using git add -p this is very easy to
accomplish.

Uli

On Tue, May 19, 2020 at 10:08 PM Melroy van den Berg
<melroy89@protonmail.com> wrote:
>
> Hi,
>
> Well.. it was not unnecessary. The code was actually unreadable, wrong indentations and inconsistent throughout the whole file.
> I needed to execute this auto-formatter, before I could even begin to understand the code.
>
> In fact, I urge you to execute this on the whole archive. And keep the code quality high via a CI pipeline or something.
>
> I can understand it makes it hard to read. I also fixed other some issues like this double error message during login and all the debug statements.
> What would be your approach? I think it's still wise to auto-format the code in consistent matter.
>
> Regards,
> Melroy van den Berg
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> Op dinsdag, mei 19, 2020 9:48 PM, Mike Gabriel <mike.gabriel@das-netzwerkteam.de> schreef:
>
> > Hi Melroy,
> >
> > On Di 19 Mai 2020 03:59:16 CEST, Melroy van den Berg wrote:
> >
> > > Hi,
> > > I did some refactoring in the sshmasterconnection.cpp file. I think > it can definitely use some clean-up and further splitting functions > and even into multiple files eventually.
> > > I cloned the master branch of code.x2go.org/x2goclient repo.
> > > The changes I applied are in the attachment of this mail (a git > patch file). I think closing previous sessions and connection solved > my strange SSH connection is resulting into Socket error: No such > file or directory".
> > > Although X2Go client is calling libssh > calls (libssh version > currently in use runtime on my host is: 0.7.0).
> > > For some reason this vague error message popped-up by libssh, > properly a result of previous session being created and not > correctly closed. And/or not closed in the right order (/missing API > calls), like a good example:
> > > ssh_channel_send_eof(channel);
> > > ssh_channel_close(channel);
> > > ssh_channel_free(channel);
> > > And I created yet again a new YT video showing my result:
> > > https://youtu.be/vmASLJq0CKM
> > > (including some brief explanation what I did so far, I also ran an > formatted on the code, because it was very very hard to read with > all the wrong indents)
> > > I also made another video showing yet another bug I found regarding > the SSH tunneling (during some heavy testing), the only solution was > to restart the whole docker image:
> > > https://youtu.be/xEpIyo84fWc (I think this issue is unrelated, but > maybe wise to take serious as well)
> > > Again see attachment for the patch (branched from master).
> > > Regards,
> > > Melroy van den Berg
> >
> > Thanks for working on X2Go Client. With your patch, there is a problem though. It is barely unreviewable, because you have so many changes (also white space changes) in one patch file.
> >
> > Please clone the X2Go Client Git repo, add atomic changes with good commit messages. Keep white-space changes and technical changes fully separate (ideally: technical changes first, white-space changes last).
> >
> > Then export your patches using 'git format-patch <last-official-commit>..HEAD' and send us the set of patches instead this one bulk commit/patch.
> >
> > Thanks for your efforts!!!
> >
> > Mike
> >
> > -----------------------------------
> >
> > DAS-NETZWERKTEAM
> > c\o Technik- und Ökologiezentrum Eckernförde
> > Mike Gabriel, Marienthaler Str. 17, 24340 Eckernförde
> > mobile: +49 (1520) 1976 148
> > landline: +49 (4351) 850 8940
> >
> > GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31
> > mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
> _______________________________________________
> x2go-dev mailing list
> x2go-dev@lists.x2go.org
> https://lists.x2go.org/listinfo/x2go-dev


Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Tue, 19 May 2020 21:30:05 GMT) (full text, mbox, link).


Acknowledgement sent to Melroy van den Berg <melroy89@protonmail.com>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Tue, 19 May 2020 21:30:06 GMT) (full text, mbox, link).


Message #35 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Melroy van den Berg <melroy89@protonmail.com>
To: "1469@bugs.x2go.org" <1469@bugs.x2go.org>, Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Subject: But wait; there is more... patches!
Date: Tue, 19 May 2020 21:24:09 +0000
[Message part 1 (text/plain, inline)]
Hi Mike & Uli,

As Mike suggested, I redone all changes (plus even some more bonus items).
Since I was in a separate git branch I could run the format-patch easily via:git format-patch master. Thanks for the suggestion.

@Uli I'm using the auto-formatter for C++ built-in in VS Code.

I think the end-result with all the separate patches makes more sense now, sorry I didn't do it right away. I think once I have a wiki account, I will add all these best practices your x2go wiki. Instead of Attachment: <your-patch.diff>: https://wiki.x2go.org/doku.php/wiki:bugs?s[]=git&s[]=patch#bug_submission_with_a_patch.

See the attachments for all the patches in this mail (11 in total). Thanks in advance.

Best regards,
Melroy van den Berg
[Message part 2 (text/html, inline)]
[0002-endl-for-every-debug-line.patch (text/x-patch, attachment)]
[0003-send-eof-before-close.patch (text/x-patch, attachment)]
[0004-return-false-in-all-bad-cases.patch (text/x-patch, attachment)]
[0006-Remove-deprecated-call-and-use-new-libssh-API-call.-.patch (text/x-patch, attachment)]
[0005-Refactor-and-fix-channel-session-closes.patch (text/x-patch, attachment)]
[0007-Fix-when-not-password-auth.-Otherwise-you-get-double.patch (text/x-patch, attachment)]
[0010-Mark-as-unused-explicitly.patch (text/x-patch, attachment)]
[0011-improve-gitignore-why-not.patch (text/x-patch, attachment)]
[0008-Correct-retval-check.-Plus-some-minor-formatting.patch (text/x-patch, attachment)]
[0009-Another-missing-endl.patch (text/x-patch, attachment)]
[0001-First-auto-format-the-code-for-readability-and-human.patch (text/x-patch, attachment)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Tue, 19 May 2020 21:45:06 GMT) (full text, mbox, link).


Acknowledgement sent to Mike Gabriel <mike.gabriel@das-netzwerkteam.de>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Tue, 19 May 2020 21:45:09 GMT) (full text, mbox, link).


Message #40 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
To: Melroy van den Berg <melroy89@protonmail.com>
Cc: 1469@bugs.x2go.org
Subject: Re: But wait; there is more... patches!
Date: Tue, 19 May 2020 21:40:07 +0000
[Message part 1 (text/plain, inline)]
Hi Melroy,

On  Di 19 Mai 2020 23:24:09 CEST, Melroy van den Berg wrote:

> Hi Mike & Uli,
>
> As Mike suggested, I redone all changes (plus even some more bonus items).
> Since I was in a separate git branch I could run the format-patch  
> easily via:git format-patch master. Thanks for the suggestion.
>
> @Uli I'm using the auto-formatter for C++ built-in in VS Code.
>
> I think the end-result with all the separate patches makes more  
> sense now, sorry I didn't do it right away. I think once I have a  
> wiki account, I will add all these best practices your x2go wiki.  
> Instead of Attachment: <your-patch.diff>:  
> https://wiki.x2go.org/doku.php/wiki:bugs?s[]=git&s[]=patch#bug_submission_with_a_patch.
>
> See the attachments for all the patches in this mail (11 in total).  
> Thanks in advance.
>
> Best regards,
> Melroy van den Berg

Awesome. I will go over those patches asap and give feedback. Thanks  
for providing the changeset in the way I requested. I will also  
provide wiki access the coming days (ideally tomorrow).

If you don't hear back from me until next Monday/Tuesday, please re-ping me.

Mike
-- 

DAS-NETZWERKTEAM
c\o Technik- und Ökologiezentrum Eckernförde
Mike Gabriel, Marienthaler Str. 17, 24340 Eckernförde
mobile: +49 (1520) 1976 148
landline: +49 (4351) 850 8940

GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22  0782 9AF4 6B30 2577 1B31
mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

[Message part 2 (application/pgp-signature, inline)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Wed, 20 May 2020 07:45:08 GMT) (full text, mbox, link).


Acknowledgement sent to Mihai Moldovan <ionic@ionic.de>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Wed, 20 May 2020 07:45:09 GMT) (full text, mbox, link).


Message #45 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Mihai Moldovan <ionic@ionic.de>
To: 1469-submitter@bugs.x2go.org
Cc: control@bugs.x2go.org, 1469@bugs.x2go.org
Subject: X2Go issue (in src:x2goclient) has been marked as pending for release
Date: Wed, 20 May 2020 09:41:11 +0200 (CEST)
tag #1469 pending
fixed #1469 4.1.2.3
thanks

Hi!

X2Go issue #1469 (src:x2goclient) reported by you has been
fixed in X2Go Git. You can see the changelog below, and you can
check the diff of the fix at:

    https://code.x2go.org/gitweb?p=x2goclient.git;a=commitdiff;h=56d82b912b3b92508fe21035e075dc879691f5dc

The issue will most likely be fixed in src:x2goclient (4.1.2.3).

light+love
X2Go Git Admin (on behalf of the sender of this mail)

---
commit 56d82b912b3b92508fe21035e075dc879691f5dc
Author: Melroy van den Berg <melroy@melroy.org>
Date:   Wed May 20 09:38:47 2020 +0200

    .gitignore: ignore Visual Studio temporary files. Fixes: #1469.

diff --git a/debian/changelog b/debian/changelog
index 32a5550..21abcc1 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -26,6 +26,7 @@ x2goclient (4.1.2.3-0x2go1) UNRELEASED; urgency=medium
       error messages.
     - src/sshmasterconnection: correct retval check.
     - src/sshmasterconnection: explicitly mark function parameters as unused.
+    - .gitignore: ignore Visual Studio temporary files. Fixes: #1469.
 
  -- X2Go Release Manager <git-admin@x2go.org>  Thu, 13 Feb 2020 12:31:20 +0100
 


Added tag(s) pending. Request was from Mihai Moldovan <ionic@ionic.de> to control@bugs.x2go.org. (Wed, 20 May 2020 07:45:09 GMT) (full text, mbox, link).


Marked as fixed in versions 4.1.2.3. Request was from Mihai Moldovan <ionic@ionic.de> to control@bugs.x2go.org. (Wed, 20 May 2020 07:45:09 GMT) (full text, mbox, link).


Message sent on to Melroy van den Berg <melroy89@protonmail.com>:
Bug#1469. (Wed, 20 May 2020 07:45:10 GMT) (full text, mbox, link).


Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Wed, 20 May 2020 07:55:06 GMT) (full text, mbox, link).


Acknowledgement sent to Mihai Moldovan <ionic@ionic.de>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Wed, 20 May 2020 07:55:08 GMT) (full text, mbox, link).


Message #57 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Mihai Moldovan <ionic@ionic.de>
To: Melroy van den Berg <melroy89@protonmail.com>, 1469@bugs.x2go.org, Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Subject: Re: [X2Go-Dev] Bug#1469: But wait; there is more... patches!
Date: Wed, 20 May 2020 09:52:46 +0200
[Message part 1 (text/plain, inline)]
* On 5/19/20 11:24 PM, Melroy van den Berg wrote:
> As Mike suggested, I redone all changes (plus even some more bonus items).
> Since I was in a separate git branch I could run the format-patch easily via:git
> format-patch master. Thanks for the suggestion.
> 
> @Uli I'm using the auto-formatter for C++ built-in in VS Code.
> 
> I think the end-result with all the separate patches makes more sense now, sorry
> I didn't do it right away. I think once I have a wiki account, I will add all
> these best practices your x2go wiki. Instead of Attachment: <your-patch.diff>:
> https://wiki.x2go.org/doku.php/wiki:bugs?s[]=git&s[]=patch#bug_submission_with_a_patch.
> 
> See the attachments for all the patches in this mail (11 in total). Thanks in
> advance.

Alex's coding style is... special, I guess, but I usually don't change it unless
I have to change a line anyway and mine can also be considered odd at times.


0001-First-auto-format-the-code-for-readability-and-human.patch: won't apply,
it's just adding unnecessary noise and making blaming more difficult.


0002-endl-for-every-debug-line.patch: won't apply. Using (std::)endl is actually
a bug, because qDebug (which is used by x2goDebug internally) already appends a
newline character by default, so another endl call would just give us double
newlines and make the log output worse. I'm actually tempted to remove the other
endls instead, but there's no real benefit in doing so immediately, so I'll just
keep them until a change to the lines has to be made anyway (or it fits
semantically).


0003-send-eof-before-close.patch: good catch, thanks, applied. Strictly
speaking, though, this should not cause any issues, as ssh_channel_close () will
(according to its documentation) already send an EOF. The worst thing that
SHOULD happen is that the additional ssh_channel_send_eof () call is unnecessary
and will cause an internal error in libssh because it might want to write to an
already EOF'd and closed channel. Then again, this error should be benign, since
we'll kill the channel right away anyway.


0004-return-false-in-all-bad-cases.patch: applied, thanks.


0005-Refactor-and-fix-channel-session-closes.patch: highly amended, but mostly
ultimately applied. Did not apply unnecessary code style or endl changes, but
picked most stuff. Also, I originally made the new function static because I
didn't see any benefit in having it part of the API for other places to call it.
Turned out this was a bad idea, so I reverted to registering it within the class
and make it private, like you did. Thanks.


0006-Remove-deprecated-call-and-use-new-libssh-API-call.-.patch: won't apply.
This code path is for old libssh versions only (pre-0.8) and still works there,
so I don't see any reason to change it. Newer versions should already use the
non-deprecated function, so everything's fine.


0007-Fix-when-not-password-auth.-Otherwise-you-get-double.patch: mostly applied,
slightly amended. Not changing return (...) vs. return ..., the actual change
looks good though.


0008-Correct-retval-check.-Plus-some-minor-formatting.patch: functionally
applied, minus formatting.


0009-Another-missing-endl.patch: not applied, same reasoning as for 0001.


0010-Mark-as-unused-explicitly.patch: can't hurt, I guess, so applied, but
slightly amended.


0011-improve-gitignore-why-not.patch: mostly applied, without the .gitignore
file entry deletion which is intentional.



Mihai

[signature.asc (application/pgp-signature, attachment)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Wed, 20 May 2020 15:10:16 GMT) (full text, mbox, link).


Acknowledgement sent to Melroy van den Berg <melroy89@protonmail.com>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Wed, 20 May 2020 15:10:18 GMT) (full text, mbox, link).


Message #62 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Melroy van den Berg <melroy89@protonmail.com>
To: "1469@bugs.x2go.org" <1469@bugs.x2go.org>
Subject: Re: Re: [X2Go-Dev] Bug#1469: But wait; there is more... patches!
Date: Wed, 20 May 2020 15:04:17 +0000
[Message part 1 (text/plain, inline)]
Hi Mike,

Thanks for your reply. Are are my thoughts.

> Using (std::)endl is actually a bug

Ah, thanks for the heads-up! I think in that case we should in fact remove all the endl instead of adding it :).

> Strictly speaking, though, this should not cause any issues

Why was I getting socket error then? I really hope everything got merged correctly now. Including my changes in the separate function.

Since all the changes together made my socket error from libssh disappear now.

> This code path is for old libssh versions only (pre-0.8) and still works there,
> so I don't see any reason to change it.

I got deprecation warnings on this function/line while compiling. Why not merge it and remove build warnings?

The change in within the #else, maybe if you think it should be in the #if you can apply the change in that section (but I think in my case the else part got compiled).

Nevertheless, it's a deprecation that needs to be solved anyway.

> Not changing return (...) vs. return ..., the actual change
> looks good though.

Why not changing the return(). This is wrong code, it's just a boolean! Bad practice, please don't it. Don't be afraid to change it.

A generic remark: I think X2Go is missing a good pipeline with testcases and other quality checks. Which also hopefully increases your *trust in the code* and enables refactoring as well.

I'm not afraid to refactor the code and clean-up the formatting, splitting functions and even into multiple files. If this all improve readability, debuggability and test-ability long-term.

That is why I raise a request to create a decent pipeline to allow the necessary changes in further improve code maturity and the needed changes to do so.

And maybe even a better diff tool to perform refactoring changes during review.

Any ideas or suggestions? I'm running a GitLab instance myself for example; which enables DevOps and CI/CD within all my projects.

Thanks once more!

Kind regards,
Melroy van den Berg
[Message part 2 (text/html, inline)]

Information forwarded to x2go-dev@lists.x2go.org, X2Go Developers <x2go-dev@lists.x2go.org>:
Bug#1469; Package x2goclient. (Mon, 25 May 2020 05:15:01 GMT) (full text, mbox, link).


Acknowledgement sent to Mihai Moldovan <ionic@ionic.de>:
Extra info received and forwarded to list. Copy sent to X2Go Developers <x2go-dev@lists.x2go.org>. (Mon, 25 May 2020 05:15:02 GMT) (full text, mbox, link).


Message #67 received at 1469@bugs.x2go.org (full text, mbox, reply):

From: Mihai Moldovan <ionic@ionic.de>
To: Melroy van den Berg <melroy89@protonmail.com>, 1469@bugs.x2go.org
Subject: Re: [X2Go-Dev] Bug#1469: But wait; there is more... patches!
Date: Mon, 25 May 2020 07:13:43 +0200
[Message part 1 (text/plain, inline)]
* On 5/20/20 5:04 PM, Melroy van den Berg wrote:
>> Using (std::)endl is actually a bug
> 
> Ah, thanks for the heads-up! I think in that case we should in fact remove all the endl instead of adding it :).

Yeah, probably. It creates additional newlines in the debug output which isn't
really intended. It might be a nice way to group messages to some degree, but I
figure I added most of the endls myself when I didn't know better, which was a
mistake in the past.


>> Strictly speaking, though, this should not cause any issues
> 
> Why was I getting socket error then? I really hope everything got merged correctly now. Including my changes in the separate function.
> Since all the changes together made my socket error from libssh disappear now.

Of course, I merged in most functional changes unless explicitly noted. The
issues were probably caused by the other locations, not this one specifically.
I pulled it anyway, because that's probably the more correct behavior in any case.


> I got deprecation warnings on this function/line while compiling. Why not merge it and remove build warnings?
> The change in within the #else, maybe if you think it should be in the #if you can apply the change in that section (but I think in my case the else part got compiled).
> Nevertheless, it's a deprecation that needs to be solved anyway.

It doesn't need to be solved because it *is* already solved when compiling
against newer libssh versions. The actual bug is that it seems to have used the
#else branch in your compile runs and I'm curious why. What OS have you been
using to build it and, specifically, what version of libssh-{4,dev} was installed?

Case in point, I certainly don't get a warning when building against libssh
0.9.3, which indicates that the the correct branch without the non-deprecated
function is used.

If your system has libssh >= 0.8.0 and it's taking the wrong branch, I'd like to
know what's wrong.


>> Not changing return (...) vs. return ..., the actual change
>> looks good though.
> 
> Why not changing the return(). This is wrong code, it's just a boolean! Bad practice, please don't it. Don't be afraid to change it.

That's just not true.

Up until C++14, "return (foo)" and "return foo" were functionally absolutely
equivalent and just a matter of taste/style. I like the former way better,
because it encapsulates the return value optically.

Since C++14, you're half right that it's wrong, because both statements MIGHT
behave differently. This, however, is only the case for return values that
explicitly use decltype (auto).

We're not using any C++14 (or even C++11) features anyway and probably won't do
so for a longer time, because we also target legacy systems without such
support. In the end, it is personal preference here.


> A generic remark: I think X2Go is missing a good pipeline with testcases and
> other quality checks.

Yes, it does, and that's arguably bad. We've been discussing this every single
year, but that's a mammoth task.


> Which also hopefully increases your *trust in the code*
> and enables refactoring as well.
> I'm not afraid to refactor the code and clean-up the formatting, splitting
> functions and even into multiple files. If this all improve readability,
> debuggability and test-ability long-term.

I always avoid changing stuff for no good reason. While proper version control
systems like git take out some of the complexity that is involved by, e.g.,
formatting updates, it still comes at a price that cannot be automatically
handled by the tools.

I like to git blame a lot to see how portions of code changed, but each
non-functional change adds another layer of indirection to the process, which
can make it very frustrating.

Most software projects I came across handle it the same way and will tell you to
please split out non-functional changes or to just remove them completely.


> That is why I raise a request to create a decent pipeline to allow the necessary
> changes in further improve code maturity and the needed changes to do so.
> And maybe even a better diff tool to perform refactoring changes during review.
> 
> Any ideas or suggestions? I'm running a GitLab instance myself for example;
> which enables DevOps and CI/CD within all my projects.
> Thanks once more!

We do have a public Jenkins instance for CI at https://jenkins.x2go.org .

Testing is really a badly missing thing, but we try to test things in a manual
manner especially before a release if time permits, but naturally that's not a
substitute for, e.g., unit tests.

Completely automatic testing of GUI stuff is extremely difficult, though.

Maybe someday, but I won't and cannot promise it. I'm already stumped with so
much other stuff that regularly breaks or needs to be updated/improved and is
arguably more important (because the best testing infrastructure won't help you
if all the other things don't work).



Mihai

[signature.asc (application/pgp-signature, attachment)]

Send a report that this bug log contains spam.


X2Go Developers <owner@bugs.x2go.org>. Last modified: Wed Jul 8 02:18:10 2020; Machine Name: ymir.das-netzwerkteam.de

X2Go Bug tracking system

Debbugs is free software and licensed under the terms of the GNU Public License version 2. The current version can be obtained from https://bugs.debian.org/debbugs-source/.

Copyright © 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson, 2005-2017 Don Armstrong, and many other contributors.