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

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?