[Monotone-devel] Patches and questions to various automate commands

[Thomas Keller]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Hi all!

Some questions on several automate commands:

automate genkey:
- ----------------

Apparently this command outputs status infos on stderr, which is totally
unexpected when used over stdio. IMHO only stdio itself should directly
output stuff over stderr - if commands need error output, they need to
ensure this gets encoded into stdio.
Any objections against applying the attached genkey.patch? The only
thing I'm wondering here is if I should push the interface_version here
- - while the status output is certainly not part of the documented format
it outputs, it was just there since 4.1 ...

automate keys:
- --------------

A small little glich I encountered just recently: A running mtn
instances won't notice if a key has been dropped from the keystore
because it reads the local keystore only once (maybe_read_key_dir() in
key_store.cc). Now, I _think_ the whole key_store caching thing is a bad
idea at all because there is at least one other command I know (automate
cert) which now runs over stdio and needs an up-to-date key list as
well. However, I have no idea if its really a good idea to completly
remove the keystore cache as I did in keystore.patch.

automate db_get:
- ----------------

No patch yet, just wondering here if it would be a good idea to change
this command in the way `ls vars` works? Problem I'm facing here is that
one f.e. cannot guess the name(s) of the known servers to get their
fingerprints. I'd likely change the output format of db_get to some new
basic_io-based format and output everything just like `ls vars`. Would
the name "db_get" be anymore good then for this or shall I rename or
even introduce a new command? I'd vote for

db_get DOMAIN KEY       => get_variables [DOMAIN]
db_set DOMAIN KEY VALUE => set_variable DOMAIN KEY VALUE

since this would correlate nicely to all the other getters and setters
we have so far in the interface. Maybe it would be even better to name
them get_db_variables and set_db_variable to be more precise. What do
you think?

Thomas.

- --
only dead fish swim with the stream: http://thomaskeller.biz/blog
Für Freiheit und gegen staatliche Überwachungsmaßnahmen:
http://leipzig.vorratsdatenspeicherung.de
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHXTkxaf7NlBYNEJIRAlC9AJ0UAezyEWeOXI1Rt1aZqW1Zv0lOhQCfbhvK
yQgE/j301VlejLiVuEHMmjk=
=gU46
-----END PGP SIGNATURE-----

#
# old_revision [4c0c4c4111dec5c850e3db5c984c889d1b4d4d95]
#
# patch "automate.cc"
#  from [70e539a80dd102f396e776f1a06760da5606b165]
#    to [fce0bfb5651e4b12b02d8efc1050ab0ccffe1981]
#
============================================================
--- automate.cc 70e539a80dd102f396e776f1a06760da5606b165
+++ automate.cc fce0bfb5651e4b12b02d8efc1050ab0ccffe1981
@@ -1544,10 +1544,7 @@ CMD_AUTOMATE(genkey, N_("KEYID PASSPHRAS
   N(!exists, F("key '%s' already exists") % ident);

   keypair kp;
-  P(F("generating key-pair '%s'") % ident);
   generate_key_pair(kp, passphrase);
-  P(F("storing key-pair '%s' in %s/")
-    % ident % app.keys.get_key_dir());
   app.keys.put_key_pair(ident, kp);

   basic_io::printer prt;

#
# old_revision [4c0c4c4111dec5c850e3db5c984c889d1b4d4d95]
#
# patch "key_store.cc"
#  from [0644bde00d3fd85c7a34eea4cb99f12a4779f40d]
#    to [66774adf51e196fd11f574e664bfd9612c0e55de]
# 
# patch "key_store.hh"
#  from [9417e9f8b3fc1b2f78424ae80a7a629f23b90ff7]
#    to [74aeee95a60ee2cad80456dde2e1cc822d57e378]
#
============================================================
--- key_store.cc        0644bde00d3fd85c7a34eea4cb99f12a4779f40d
+++ key_store.cc        66774adf51e196fd11f574e664bfd9612c0e55de
@@ -74,6 +74,14 @@ key_store::read_key_dir()
 void
 key_store::read_key_dir()
 {
+  // stop recursion from various places (f.e. keyreader::consume_key_pair)
+  if (reading_keys) return;
+  reading_keys = true;
+
+  // clear the in-memory caches
+  keys.clear();
+  hashes.clear();
+
   vector<path_component> key_files, dirs;
   if (directory_exists(key_dir))
     {
@@ -92,21 +100,14 @@ key_store::read_key_dir()
       istringstream is(dat());
       read_packets(is, kr, app);
     }
-}
 
-void
-key_store::maybe_read_key_dir()
-{
-  if (have_read)
-    return;
-  have_read = true;
-  read_key_dir();
+  reading_keys = false;
 }
 
 void
 key_store::ensure_in_database(rsa_keypair_id const & ident)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   map<rsa_keypair_id, keypair>::iterator i = keys.find(ident);
 
   // if this object does not have the key, the database had better.
@@ -115,7 +116,7 @@ key_store::ensure_in_database(rsa_keypai
       I(app.db.public_key_exists(ident));
       return;
     }
-  
+
   if (app.db.put_key(ident, i->second.pub))
     L(FL("loaded public key '%s' into db") % ident);
 }
@@ -134,7 +135,7 @@ key_store::get_key_ids(globish const & p
 key_store::get_key_ids(globish const & pattern,
                        vector<rsa_keypair_id> & priv)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   priv.clear();
   for (map<rsa_keypair_id, keypair>::const_iterator
          i = keys.begin(); i != keys.end(); ++i)
@@ -145,7 +146,7 @@ key_store::get_key_ids(vector<rsa_keypai
 void
 key_store::get_key_ids(vector<rsa_keypair_id> & priv)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   priv.clear();
   for (map<rsa_keypair_id, keypair>::const_iterator
          i = keys.begin(); i != keys.end(); ++i)
@@ -155,7 +156,7 @@ key_store::key_pair_exists(rsa_keypair_i
 bool
 key_store::key_pair_exists(rsa_keypair_id const & ident)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   return keys.find(ident) != keys.end();
 }
 
@@ -163,7 +164,7 @@ key_store::get_key_pair(rsa_keypair_id c
 key_store::get_key_pair(rsa_keypair_id const & ident,
                         keypair & kp)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   map<rsa_keypair_id, keypair>::const_iterator i = keys.find(ident);
   I(i != keys.end());
   kp = i->second;
@@ -179,7 +180,7 @@ key_store::get_key_file(rsa_keypair_id c
   for (unsigned int i = 0; i < leaf.size(); ++i)
     if (leaf.at(i) == '+')
       leaf.at(i) = '_';
-  
+
   file = key_dir / path_component(leaf);
 }
 
@@ -214,7 +215,7 @@ key_store::put_key_pair_memory(rsa_keypa
 key_store::put_key_pair_memory(rsa_keypair_id const & ident,
                                keypair const & kp)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   L(FL("putting key pair '%s'") % ident);
   pair<map<rsa_keypair_id, keypair>::iterator, bool> res;
   res = keys.insert(make_pair(ident, kp));
@@ -238,7 +239,7 @@ key_store::delete_key(rsa_keypair_id con
 void
 key_store::delete_key(rsa_keypair_id const & ident)
 {
-  maybe_read_key_dir();
+  read_key_dir();
   map<rsa_keypair_id, keypair>::iterator i = keys.find(ident);
   if (i != keys.end())
     {
============================================================
--- key_store.hh        9417e9f8b3fc1b2f78424ae80a7a629f23b90ff7
+++ key_store.hh        74aeee95a60ee2cad80456dde2e1cc822d57e378
@@ -13,7 +13,7 @@ private:
 {
 private:
   system_path key_dir;
-  bool have_read;
+  bool reading_keys;
   app_state & app;
   std::map<rsa_keypair_id, keypair> keys;
   std::map<hexenc<id>, rsa_keypair_id> hashes;
@@ -21,7 +21,6 @@ private:
   void get_key_file(rsa_keypair_id const & ident, system_path & file);
   void write_key(rsa_keypair_id const & ident);
   void read_key_dir();
-  void maybe_read_key_dir();
 public:
   key_store(app_state & a);
   void set_key_dir(system_path const & kd);
@@ -46,8 +45,8 @@ public:
   // primarily for internal use in reading keys back from disk.
   bool put_key_pair_memory(rsa_keypair_id const & ident,
                            keypair const & kp);
-                         
 
+
   void delete_key(rsa_keypair_id const & ident);
 };