Merge lp:~develop7/synapse-project/ssh-plugin-tweaks into lp:synapse-project

[Andrei Dziahel]

In synapse 0.2.8 after updating ssh_config (like adding new hosts) you have to restart synapse. After applying this patch, ssh_config becomes monitored by glib and is reparsed automatically on updating.

Code is definitely not production-ready, and there could be some caveats, so I'd appreciate to be reviewed by someone experienced.

[Andrei Dziahel (develop7)]

well, it works for me, but there's some things I'm worried about

Like following:

At L51 diagnostic message is printed on detecting changes in file. And here's what is really printed on saving .ssh/config:

% ./src/ui/synapse
[16:08:48.986272 Info] Starting up...
...skip...
[16:09:10.456628 Info] [SshPlugin] ssh_config is changed, reparsing
[16:09:10.456982 Info] [SshPlugin] ssh_config is changed, reparsing
[16:09:10.457302 Info] [SshPlugin] ssh_config is changed, reparsing
[16:09:10.457462 Info] [SshPlugin] ssh_config is changed, reparsing

Reparsing is done 4 times every single save. And I don't know, why. Any ideas?

[Alberto Aldegheri (albyrock87)]

This is strange, I'm not an expert on monitors, but I suggest another way to do this.
In example you could simply reparse the file every 5 minutes (if it exists).

Also, are you checking that ssh_config exists before creating the monitor?

[Andrei Dziahel (develop7)]

> This is strange, I'm not an expert on monitors, but I suggest another way to do this.
> In example you could simply reparse the file every 5 minutes (if it exists).
And then I have to wait 5 minutes (worst scenario) while synapse will hook up new ssh hosts/ditch old ones? Looks no good to me, sorry.
Anyway then, I'll delay actual reparsing to, say, 500ms.
> Also, are you checking that ssh_config exists before creating the monitor?
No. I'll fix that, thanks.

[Michal Hruby (mhr3)]

There are some hints when a monitor fires, I think you should be looking for CHANGES_DONE, but you better check that.

[lp:~develop7/synapse-project/ssh-plugin-tweaks]

478. By Andrei Dziahel on 2011-09-22

tuned monitoring subscription

[Andrei Dziahel (develop7)]

@mhr3, thanks for pointing out. I've reduced reparsing count to 2 instead of 4 (see r478)
Still considering delay reparsing to, say, 200 ms.

[lp:~develop7/synapse-project/ssh-plugin-tweaks]

479. By Andrei Dziahel on 2011-09-23

fixed double reparsing of ssh config file; implemented handling of config monitoring error

[Andrei Dziahel (develop7)]

Ok, so I've just added monitoring error handling (like Alberto suggested) and learned that FileMonitorEvent.CHANGES_DONE_HINT should be used alone (in my case).

[lp:~develop7/synapse-project/ssh-plugin-tweaks]

480. By Andrei Dziahel on 2011-09-26

merged with upstream

[Andrei Dziahel (develop7)]

So, um, I suppose this branch is ready for merging into upstream/trunk/whatever you'd like to merge it.

[Alberto Aldegheri (albyrock87)]

Yes, I will merge it soon!

review: Approve

=== modified file 'src/plugins/ssh-plugin.vala'
--- src/plugins/ssh-plugin.vala	2011-08-04 16:37:34 +0000
+++ src/plugins/ssh-plugin.vala	2011-09-26 09:46:23 +0000
@@ -27,6 +27,9 @@
   {
     public  bool      enabled { get; set; default = true; }
     private ArrayList<SshHost> hosts;
+    
+    protected File config_file;
+    protected FileMonitor monitor;
 
     static construct
     {
@@ -40,7 +43,21 @@
 
     public void activate ()
     {
+      this.config_file = File.new_for_path (Environment.get_home_dir () + "/.ssh/config");
+
       parse_ssh_config.begin ();
+      
+      try 
+      {
+        this.monitor = config_file.monitor_file(FileMonitorFlags.NONE);
+        this.monitor.changed.connect((m, f, of, type) => { 
+          handle_ssh_config_update(m, f, of, type); 
+        });
+      }
+      catch (IOError e)
+      {
+        Utils.Logger.warning(this, "Failed to start monitoring changes of ssh client config file");
+      }
     }
 
     public void deactivate () {}
@@ -61,13 +78,11 @@
 
     private async void parse_ssh_config ()
     {
-      var file = File.new_for_path (Environment.get_home_dir () + "/.ssh/config");
-
       hosts.clear ();
 
       try
       {
-        var dis = new DataInputStream (file.read ());
+        var dis = new DataInputStream (config_file.read ());
 
         // TODO: match key boundary
         Regex host_key_re = new Regex ("(host\\s)", RegexCompileFlags.OPTIMIZE | RegexCompileFlags.CASELESS);
@@ -102,7 +117,16 @@
       }
       catch (Error e)
       {
-        Utils.Logger.warning (this, "%s: %s", file.get_path (), e.message);
+        Utils.Logger.warning (this, "%s: %s", config_file.get_path (), e.message);
+      }
+    }
+    
+    public void handle_ssh_config_update(FileMonitor monitor, File file, File? other_file, FileMonitorEvent event_type)
+    {
+      if (event_type == FileMonitorEvent.CHANGES_DONE_HINT)
+      {
+        Utils.Logger.log(this, "ssh_config is changed, reparsing");
+        parse_ssh_config.begin ();
       }
     }