[Libvirt-cim] [PATCH V3 11/11] allow ssh based migration with non root's key file

[John Ferlan]

Since I don't have the original email to reply-to, here is a link:

https://www.redhat.com/archives/libvirt-cim/2012-December/msg00033.html


libvirt-cim.conf

Consider changing the following:

+#  Defines a temp key file which would be used as ssh key in ssh migration,
+#  Only valid when SSH_Key is set in migration call.
+#  If SSH_Key is not in a directory owned by root, set this value to a path
+#  owned by root, tells libvirt-cim copy it there before use it. The directory
+#  in the path must exist and totally owned by root.
+#  If SSH_Key is already in a valid place, don't set it to to tell libvirt-cim
+#  use SSH_Key directly.


# Only valid when SSH_Key is set for the migration call. 
#
# In order to allow using a non-root owned SSH Key during migration, set this variable
# to a location to copy the SSH_Key from the SSH_Key path and file not owned by root
# to a directory path and file owned by root. The target path must exist and it must
# be owned by root. If it is not, the migration will fail.
#
# If the SSH_Key is already in a directory owned by root there is no need to set, thus
# allowing libvirt-cim to use the SSH_Key directly.





libxkutil/misc_util.c

In libvirt_cim_config_get()
 * The 'default:' case should probably also which prop->name was being used for the prop->value_type

In is_read_only() & get_mig_ssh_tmp_key()
 * Probably should initialize prop.value_string = NULL just to be clear
 * Call to libvirt_cim_config_get() does not check return status - perhaps make setting prop.have_read conditional on return value...


 src/Virt_VSMigrationService.c
In ssh_key_cp():
 * Do you need a path to 'cp' (eg /sbin/cp)?
 * Is it worth a stat() afterwards to ensure the copy occurred since your fgets() isn't returning anything?
 * Other thoughts - should there be a "stat" and "rm" before? Perhaps another safety ensure we copy something?

In get_msd_values()
 * Since you already print the source ssh_key file, change the the copy src to dest message to just indicate the destination (e.g. "Copying ssh key to '%s'.", tmp_keyfile)


So let's say this is successful, now we leave the 'migrate_ssh_temp_key' around?  Is that a good idea?  Shouldn't there be a corresponding 'unlink()' after we're done?  We're really not a "temporary" file if we keep it around.  In fact, after the connection is made, shouldn't we be able to safely delete the file?


John


Finally, this one seems to require/need an update to documentation "somewhere" to describe that new conf variable...  That is how does one know how to use this unless they read the conf file?