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

[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".

>
> 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