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

[Wenchao Xia]

于 2013-3-12 13:53, Wenchao Xia 写道:
> 于 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".
>
   After recheck, I think "rm" can't be used here, it have a risk
to break other thread. I guess better to provide a new method
in the class:

copy_ssh_key(char *path_src);
delete_ssh_key();

   Let the user call it manually.

>>
>> 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvirt-cim
>>
>
>


-- 
Best Regards

Wenchao Xia