Bug 341284 - old-style options not working anymore in xfreerdp 1.2.0
Summary: old-style options not working anymore in xfreerdp 1.2.0
Status: RESOLVED FIXED
Alias: None
Product: krdc
Classification: Applications
Component: RDP (show other bugs)
Version: 4.13.2
Platform: Other Linux
: NOR major
Target Milestone: ---
Assignee: Urs Wolfer
URL:
Keywords:
: 341228 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-11-26 09:25 UTC by Moritz Bunkus
Modified: 2015-02-25 18:24 UTC (History)
14 users (show)

See Also:
Latest Commit:
Version Fixed In: 14.12.1


Attachments
xfreerdp 1.2 patch (4.11 KB, patch)
2014-12-08 10:37 UTC, Albert Mikaelyan
Details
xfreerdp 1.2 patch 2 (3.86 KB, patch)
2014-12-08 10:51 UTC, Albert Mikaelyan
Details
attachment-10946-0.html (1.26 KB, text/html)
2014-12-08 19:14 UTC, Tony Murray
Details
xfreerdp 1.2 patch 3 (6.35 KB, patch)
2014-12-09 15:39 UTC, Albert Mikaelyan
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Moritz Bunkus 2014-11-26 09:25:10 UTC
On Arch Linux xfreerdp 1.2.0 Beta has just been introduced. This version doesn't work with krdc anymore. Using a wrapper script instead of the xfreerdp binary it can be seen that xfreerdp always states that the usage is wrong. For example, using kdebugdialog to enable krdc's RDP-related messages shows me the following command line options that I'm running xfreerdp with:

[0 mbunkus@chai-latte ~] /usr/bin/xfreerdp -g 1800x1000 -k 0x00000409 -u mosu -p NoThisIsNotMyActualPassword -D -X 104857884 -a 24 -x l --ignore-certificate --plugin cliprdr -t 3389 ketani

FreeRDP - A Free Remote Desktop Protocol Implementation
See www.freerdp.com for more information

Usage: /usr/bin/xfreerdp [file] [options] [/v:<server>[:port]]
…

I don't know if xfreerdp has actually removed support for the old-style options, but please, PLEASE switch to the new-style options. There are several issues in the old-style option parsers, e.g. the segfaults mentioned in https://bugs.kde.org/show_bug.cgi?id=341228 and https://github.com/FreeRDP/FreeRDP/issues/2172

Here's the original bug report from Arch Linux about xfreerdp not grokking the arguments anymore:

https://bugs.archlinux.org/task/42890

Reproducible: Always

Steps to Reproduce:
1. Upgrade to the latest xfreerdp 1.2.0 beta/git
2. Try to connect to any server via RDP with krdc, options don't seem to matter
3. Watch it not work.
4. Get the command line options used by krdc via kdebugdialog
5. Execute xfreerdp with those options directly and observe the friendly usage message

Actual Results:  
Well… only a blue window meaning xfreerdp did not start up again.
Comment 1 Philippe Cloutier 2014-12-03 05:30:55 UTC
Urs Wolfer stated in ticket 331990 that FreeRDP 1.1 is not yet supported. I also lost the ability to use my main connection after upgrading from 1.0 to Debian's 1.1.0~git20140921.1.440916e+dfsg1-1.1. One of my connections still worked though. After comparing their settings, I figured out that setting Sound to "Disable Sound" worked around the issue.

Debian testing's FreeRDP is now 1.1.0~git20140921.1.440916e+dfsg1-2. The next stable version, which should release in 2015, should therefore feature FreeRDP "1.1". This would mean this issue would affect numerous users.
Comment 2 Urs Wolfer 2014-12-04 13:25:13 UTC
*** Bug 341228 has been marked as a duplicate of this bug. ***
Comment 3 Urs Wolfer 2014-12-04 13:35:16 UTC
I'm a bit confused that some distributions seem to deliver beta releases of Freerdp. Freerdp 1.1 is not supported yet in KRDC.

There exists a patch which re-adds support for Freerdp 1.1, but with this patch, 1.0 is not supported anymore: https://git.reviewboard.kde.org/r/115059/
We need a patch which supports both 1.0 and 1.1.

Until this is done, distributions which deliver beta releases of Freerpd need to patch KRDC, just like Gentoo does this: https://bugs.gentoo.org/show_bug.cgi?id=511010
Please open bug reports for that in your distributions bug tracker and post the bug report link here.

@Mihai DONȚU: In #331990 you have posted a python script which just removes one parameter if I got it right. Is this the only parameter which seems to cause issues with Freerdp 1.1?
Comment 4 Moritz Bunkus 2014-12-04 13:55:14 UTC
There are already bug reports in Arch Linux (which is what I'm using) about this issue. I've also filed a bug at FreeRDP itself about the segfault in the legacy command line handling (like I've written above), but their reaction (or lack of one) seems to speak volumes about how much they value backwards compatibility.

Of course you could also argue that krdc should not work around bugs in FreeRDP, but on the other hand FreeRDP could argue that krdc shouldn't rely on the old command-line interface indefinitely. So maybe krdc will have to bite the bullet and support both depending on the FreeRDP version installed.
Comment 5 Mihai DONȚU 2014-12-04 14:07:39 UTC
(In reply to Urs Wolfer from comment #3)
> @Mihai DONȚU: In #331990 you have posted a python script which just removes
> one parameter if I got it right. Is this the only parameter which seems to
> cause issues with Freerdp 1.1?

Yes, that appears to be the only parameter. There were some others (-E for example), but I got rid of them by simply recreating the connection (I'm a long time user of krdc and things appear to have crept in).
Comment 6 Urs Wolfer 2014-12-04 14:16:55 UTC
@Moritz Bunkus: Thanks a lot for your feedback.

I'm adding links here just for the reference:
Archlinux bug report: https://bugs.archlinux.org/task/42890
Freerdp segfault bug report: https://github.com/FreeRDP/FreeRDP/issues/2172

I fully agree with you that KRDC should switch over to the new interface (or even to the native library interface instead of calling a xfreerdp binary - but that is another discussion). For the moment it is just not that simple since we would need to support both 1.0 and 1.1 cmd line interface.

IMHO distributions which deliver beta releases of Freerdp need to apply the patch which also Gentoo applies for some months already. This is the only prompt fix for the moment. 

Fixing it in KRDC would at least need waiting until the next KDE Application release - in case that somebody finds the time to implement 1.1 and 1.0 support (which is most probably not the case in the moment).

Fixing it in Freerdp would need somebody who is interested in maintaining the old interface - I am not sure if Freerdp developers are since they have not responded to your report.

@Mihai DONȚU: Is the parameter which you are removing causing the segfault as described above? Or are you using an older version of Freerdp?
Comment 7 Mihai DONȚU 2014-12-04 14:27:15 UTC
(In reply to Urs Wolfer from comment #6)
> @Mihai DONȚU: Is the parameter which you are removing causing the segfault
> as described above? Or are you using an older version of Freerdp?

No. My version (1.2.0_beta1_pre20141115) just displays its options and exits.
Comment 8 Philippe Cloutier 2014-12-04 20:48:53 UTC
This issue exposes bug 339490.

Do not remove unusable connections which worked with FreeRDP 1.0. Re-creating will not be possible. Instead, edit your connections (the only adjustment needed may be to disable sound). If it is too late, you may create a connection by editing ~/.kde/share/apps/krdc/bookmarks.xml. The following works:
  <bookmark href="rdp://pcloutier@207.134.144.150:3611">
   <title>rdp://pcloutier@207.134.144.150:3611</title>
   <info>
    <metadata owner="http://freedesktop.org">
     <bookmark:icon name="krdc"/>
    </metadata>
    <metadata owner="http://www.kde.org">
     <time_added>1389069529</time_added>
     <time_visited>1417717526</time_visited>
     <visit_count>3</visit_count>
    </metadata>
   </info>
  </bookmark>

The href attribute needs to be adapted to your target.


FreeRDP 1.0 is known to be very buggy. I experienced important bugs with it myself. I suppose Debian upgraded attempting to help that, although on this machine, the snapshot is even buggier.

I reported this downstream in ticket #772066.

When debugging is on, console shows:
krdc(11334)/krdc (RDP backend) RdpView::start: Is LDAP login: false "pcloutier"
krdc(11334)/krdc (RDP backend) RdpView::start: Starting xfreerdp with arguments: ("-g", "1920x1080", "-k", "0x00001009", "-u", "pcloutier", "-p", "hiddenpassword", "-D", "-X", "92274721", "-a", "24", "--plugin", "rdpsnd", "--plugin", "rdpdr", "--data", "disk:media:/media", "--", "-x", "b", "--rfx", "--ignore-certificate", "--plugin", "cliprdr", "-t", "3611", "207.134.144.150")
krdc(11334) MainWindow::statusChanged: -2
krdc(11334) MainWindow::statusChanged: -1
krdc(11334) MainWindow::statusChanged: 0
krdc(11334)/krdc (RDP backend) RdpView::receivedStandardError: receivedStandardError: "WARNING: Using deprecated command-line interface!
"
krdc(11334)/krdc (RDP backend) RdpView::processError: processError: 1
Comment 9 Albert Mikaelyan 2014-12-08 10:37:03 UTC
Created attachment 89866 [details]
xfreerdp 1.2 patch

Hey guys!

I'm not a C++ developer, but I used lots of manuals and common sense, and created the following patch.
Every option of krdc works with it. Please review it.
Comment 10 Albert Mikaelyan 2014-12-08 10:51:34 UTC
Created attachment 89867 [details]
xfreerdp 1.2 patch 2

A bit cleaner ^
Comment 11 Tony Murray 2014-12-08 13:05:09 UTC
Please have a look at this patch: https://git.reviewboard.kde.org/r/115059/

What's really missing is detection between different versions of xfreerdp.  It also hasn't been fully tested against 1.2 if there is a difference from 1.1.
Comment 12 Urs Wolfer 2014-12-08 19:00:09 UTC
@Albert Mikaelyan: Thanks for you patch. As Tony already pointed out, most of this changes are already included in the patch done by him.

Albert, can you please merge your changes into Tony's and try it with 1.2?

For the required version detection, you can probably start a xfreerdp process and parse the output of "xfreerdp --version". If you get that done, I will merge this patch.
Comment 13 Tony Murray 2014-12-08 19:14:03 UTC
Created attachment 89871 [details]
attachment-10946-0.html

I reviewed his patch and it doesn't seem to be any different, when it come
to the switches. There are several other fixes in my patch. I'm going to
see if I can find some time to add a fallback to the old options with my
patch.

On Mon, Dec 8, 2014, 1:00 PM Urs Wolfer <uwolfer@kde.org> wrote:

> https://bugs.kde.org/show_bug.cgi?id=341284
>
> --- Comment #12 from Urs Wolfer <uwolfer@kde.org> ---
> @Albert Mikaelyan: Thanks for you patch. As Tony already pointed out, most
> of
> this changes are already included in the patch done by him.
>
> Albert, can you please merge your changes into Tony's and try it with 1.2?
>
> For the required version detection, you can probably start a xfreerdp
> process
> and parse the output of "xfreerdp --version". If you get that done, I will
> merge this patch.
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
>
Comment 14 Albert Mikaelyan 2014-12-09 15:39:40 UTC
Created attachment 89881 [details]
xfreerdp 1.2 patch 3

I have merged Tony's review request with my patch and updated it a bit.
There are some differences actually:
1. use +clipboard instead of /clipboard as specified by freerdp docs.
2. fixed audio-mode - it would not pass any number otherwise.
3. '/u:' option now accepts domain as per doc:
/u:[<domain>&#93;<user> or <user>[@<domain>] Username
So no need to check anything or set '/d:'
4. more of a question: there's a '/size' option available, which accepts the old style:
    arguments << "-g" << QString::number(width) + 'x' + QString::number(height);
so I just changed to this:
    arguments << "/size:" + QString::number(width) + 'x' + QString::number(height);
any reason to set 2 arguments '/w:' and '/h:' instead?

I am working with this patch, and connecting to different systems with different options. So far everything seems fine and works as it should.
Tony, please consider implementing my changes to the review request.
Comment 15 Michel Houde 2014-12-16 15:05:24 UTC
Hi everyone,  can someone tell me how to install this patch please ? I'm fairly new to this kib=nd of stuff.

Thanks in advance.
Comment 16 Albert Mikaelyan 2014-12-16 16:21:48 UTC
(In reply to Michel Houde from comment #15)
> Hi everyone,  can someone tell me how to install this patch please ? I'm
> fairly new to this kib=nd of stuff.
> 
> Thanks in advance.


If you're on Arch Linux, you can use the following PKGBUILD to test the patch (download as zip):
https://github.com/Tahvok/krdc-xfreerdp-1.2
Please, do not spam with irrelevant content here. You can further contact me directly if you want.
Comment 17 Michel Houde 2014-12-16 17:53:25 UTC
Thank you very much!  Will try it and report back.
Comment 18 bprange 2014-12-19 16:13:31 UTC
(In reply to Michel Houde from comment #17)
> Thank you very much!  Will try it and report back.

Any news on this patch?  Would love to get KRDC back up and running.

Thanks!
Comment 19 Rex Dieter 2014-12-19 16:55:07 UTC
How does the patches in this bug compare with what's posted in reviewboard @
https://git.reviewboard.kde.org/r/115059/
Comment 20 Rex Dieter 2014-12-19 16:56:09 UTC
Sorry, comment #14 answers my question nicely already.
Comment 21 Tony Murray 2014-12-29 18:13:49 UTC
I have updated the reviewboard patch with a version detection and Albert's changes.  

(Albert 4. is more a matter of preference I think it is cleaner to have the size as two arguments)

Please comment on this patch either here or the reviewboard and I'll look at committing it for the next release.
Comment 22 Urs Wolfer 2014-12-29 20:11:02 UTC
It would be great if somebody could test against 1.0. and 1.1 (or 1.2) ASAP. Code looks good to me. Please post any feedback here.
Comment 23 Albert Mikaelyan 2015-01-01 17:00:07 UTC
Tests I made:
1.2 - works as it should

1.1: Worked great as well.
$ xfreerdp --version
This is FreeRDP version 1.1.0-beta1 (git 1.1.0-beta+2013071101) 


But in 1.0, I received an error message "RDP Arguments" after which clicking 'OK' all continues fine, and I'm connected to the host.
I looked and the new patch appears to add this line on row 256:
        KMessageBox::error(0, "Starting xfreerdp with arguments:\n"+arguments.join(" "),
                           i18n("RDP Arguments"));

$ xfreerdp --version
This is FreeRDP version 1.0.2

It appears to be set for debugging and it should be just removed, Tony, please confirm.
Comment 24 Tony Murray 2015-01-02 17:07:35 UTC
Git commit 8436cc622c01d00773e47f03ab9121e1163b6369 by Tony Murray.
Committed on 02/01/2015 at 15:36.
Pushed by murrant into branch 'master'.

Support for FreeRDP 1.0.2 and newer. Including 1.1 and 1.2.

FEATURE:
FIXED-IN: 14.12.1
REVIEW: 115059

M  +184  -81   rdp/rdpview.cpp

http://commits.kde.org/krdc/8436cc622c01d00773e47f03ab9121e1163b6369
Comment 25 Tony Murray 2015-01-02 17:08:44 UTC
Git commit a9e514b29b5611e01c2c024157d36eaf416e0c5a by Tony Murray.
Committed on 02/01/2015 at 15:36.
Pushed by murrant into branch 'Applications/14.12'.

Support for FreeRDP 1.0.2 and newer. Including 1.1 and 1.2.

FEATURE:
FIXED-IN: 14.12.1
REVIEW: 115059

M  +184  -81   rdp/rdpview.cpp

http://commits.kde.org/krdc/a9e514b29b5611e01c2c024157d36eaf416e0c5a
Comment 26 Urs Wolfer 2015-01-03 16:32:06 UTC
Git commit d792c64fae0c63691cbaa7d603b759e15c8508ec by Urs Wolfer.
Committed on 03/01/2015 at 16:30.
Pushed by uwolfer into branch '14.12'.

properly handle connection errors with xfreerdp > 1.0

- check all output lines instead of stopping at first empty line (1.2 messages contain empty lines)
- check against new message spelling (tested against 1.2)

M  +4    -7    rdp/rdpview.cpp

http://commits.kde.org/krdc/d792c64fae0c63691cbaa7d603b759e15c8508ec