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?
于 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
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
do you mean check prop->name == NULL? 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@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards 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@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia
participants (2)
-
John Ferlan -
Wenchao Xia