于 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
[email protected]
https://www.redhat.com/mailman/listinfo/libvirt-cim
--
Best Regards
Wenchao Xia
_______________________________________________
Libvirt-cim mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvirt-cim
