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

于 2013-3-12 3:18, 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 > do you mean check prop->name == NULL? > In is_read_only() & get_mig_ssh_tmp_key() > * Probably should initialize prop.value_string = NULL just to be clear OK to add but not necessary, since they are declared as static and will be initialized by default. But a comment in API libvirt_cim_config_get() as "*prop must be initialized with valid default value" seems good to tip caller. > * Call to libvirt_cim_config_get() does not check return status - > perhaps make setting prop.have_read conditional on return value... > This will make the process continue reading config when a property do not exist, reduce performance. Instead of that, I think it is right to read only once when boot up. If user modify the config, he can reboot the process. > > src/Virt_VSMigrationService.c > In ssh_key_cp(): > * Do you need a path to 'cp' (eg /sbin/cp)? Possible ,maybe a config as "CP=/sbin/cp"? > * Is it worth a stat() afterwards to ensure the copy occurred since > your fgets() isn't returning anything? good idea. > * Other thoughts - should there be a "stat" and "rm" before? Perhaps > another safety ensure we copy something? good idea to avoid reimplement "popen".

> > 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) > It is OK to do so. > > 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? > I don't think delete is needed, since the model of provider to run. In actually case the user may start dozens of threads with one key, delete in one thread while other thread is still using it,may cause exception. This is the user's responsibility to clean it since he knows where it would be copied to in config file, we just need to document it. > > 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? > In normal case yes a page will be good, but now we have only config file as documents. I think config file can play the role well now. > > _______________________________________________ > Libvirt-cim mailing list > Libvirt-cim(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvirt-cim >