Accept a cfg as an argument as an alt way to start the viewer

[Kypert]

The user can specify a tigervnc configuration file as an argument to the
viewer. Previously the viewer assumed this to be a server, but now we
will first check if there is any file matching the given argument. If
so, try to load the content of that file, like we normally do.

Fixes issue #38.

[CendioOssman]

Thanks. This looks nice. Just two things:

1. Ignoring errors from fopen() is probably confusing. It's better if the user gets a proper permission denied error or such in those cases. What we should do is check if the file exists using stat().
2. I'm wondering if there is a possible security issue here if we prefer files over hostnames. E.g. someone manages to sneak in a file called myserver.example.com in the working directory of vncviewer. Should we perhaps require a path separator in the filename? I.e. you would have to specify ./myconfig.tigervnc or an absolute path.

[Kypert]

Thank you for the review! Sorry for the late response.

1. Reason for ignoring the errors is that I wanted to keep the implementation simple. I have started on an implementation that uses stat(), which is easy for the Linux target, but since I do not have the mingw environment setup, I am not sure if I need to take special consideration for that target (will work on installing the cross-compiler correctly). Also breaking it out to a separate helper function.

2. I can see your point, but for the Windows users, I am not sure the filename would always come as an absolute or path-separated relative path (will investigate, unless you know for sure?). I suppose it is fine to add the constraint for the console users but I am thinking about the file associated .tigervnc case:

• double-click
• dragged into vncviewer

[CendioOssman]

1. Reason for ignoring the errors is that I wanted to keep the implementation simple. I have started on an implementation that uses stat(), which is easy for the Linux target, but since I do not have the mingw environment setup, I am not sure if I need to take special consideration for that target (will work on installing the cross-compiler correctly). Also breaking it out to a separate helper function.

I think mingw makes sure that stat() behaves sanely even when compiling for Windows.

2. I can see your point, but for the Windows users, I am not sure the filename would always come as an absolute or path-separated relative path (will investigate, unless you know for sure?). I suppose it is fine to add the constraint for the console users but I am thinking about the file associated .tigervnc case:

I'm fairly sure you always get an absolute path in those cases. A relative path is too risky in those cases.

[Kypert]

I'm fairly sure you always get an absolute path in those cases.

Seems like you are indeed right, from my own manual testing.

Working on setting up the mingw environment, unfortunately long since I last used it so I need to read up a bit, sorry for the delays!

[Kypert]

@CendioOssman: Managed to setup mingw, attempted to address the issues you pointed out, please see the amended commit.

[CendioOssman]

You can have normal files that start with . (and maybe even a hostname?).
I suggest you simply look for a path separator (\ on Windows and / on Unix). No hostname can contain those. So you can do that check first and avoid the stat() call completely.

[Kypert]

Thank you for the comment! I like the comment about / and \, but stat() also serves the purpose to make sure the file exists and that the user has the access, like you proposed in previous attempt. Need to establish that knowledge before calling loadViewerParameters(). I can return back to fopen() and make some special handling for the permission denied case, if you like?

[CendioOssman]

What extra error handling do we actually need? Just call loadViewerParameters() and handle any exception. It will tell you why the file couldn't be opened.

[Kypert]

Originally I wanted to dodge any failures in fopen() in the event that the given argument in fact was a host. However, with your latest comment about checking for the path separator, we can distinguish if we have a file or hostname directly (or at least supposed to be). I understand now and will change accordingly, thank you for the persistently good code review comments! Please bear with me :)

[Kypert]

@CendioOssman, please let me know if any other issues remain in the latest amendment.

[CendioOssman]

The C++ code looks good now. :)

But the Java code should be changed to match so we don't have different behaviours.

(And we don't need the amendments in the commit message. Your original comment works fine. :) )

[Kypert]

Thanks again for your patience! I have aligned the two clients (added checking for empty server name in the C++ version too) and removed the amendment comments in the commit message. I am still learning best practices when working in GitHub, so I appreciate all the pointers you can give me :)

@CendioOssman, notice anything else that should be done here?

[gitcnd]

Was there some reason why this was not done as a switch?
Having a parameter which means 2 different things depending on presence of files seems less-than-standard

[CendioOssman]

A switch often makes it more difficult to integrate with "Open with" features in other programs.

[CendioOssman]

Thank you for your hard work. Looks good now.