11:56 a.m.
As Jay already said - the code is of that high quality that we are used
to get from you :). So I have no concerns about the implementation of
your approach. But I have concerns in regard to the approach how this
feature is added to the providers.
First: its not a (let me say) common CIM way to add a property array to
a class and let the client use this array as de-facto command line
interface. What you want to do is giving the client a possibility to
enhance the providers build in functionality - without rewriting the
provider. And this functionality should be configurable by the client.
The idea is valid and the implementation is straightforward.
But what the client gets is a very powerful access point to - not only
the resource managed by this class, but also to - all other resources on
the system. So please don't blame me for bringing up security issues,
but you do not have under control what kind of scripts will be put into
this directory. As you officially need to point clients to this
directory to make clear how to use the CheckParamaters property, its
well known. But deep in my heard I trust the system admins, that they do
not copy some strange scripts in there ;). So I know that this argument
can be very fast invalidated. On the other hand, all scripts ... also of
different authors, because you can have more than one mgmt. app on one
system ... have to make sure, that they can handle input parameters,
that might follow absolutely no logic. But ok, this is the
responsibility of the client to harden its scripts in that way. So these
issues are bound to the implementation approach and something I could
live with. What does not mean, that I like this time-bomb.
As I said before, the way how this feature is added to the schema is
definitely not CIM conform. Its a property misused as command line
interface. That's not what a property is - yet by definition of the word
itself ;). But I don't only want to vote against this patch set without
having a proposal. Normally you start evaluating the necessary
functionality and enhance the existing model. This could mean adding
methods or properties. But we can not foresee what the client wants to
do by its scripts. On the other hand I can imagine that the amount of
checks is limited. First by the intention of CheckParameters - which is
de-facto a replacement for the methods CheckIsVSMigratable...(). And
second by the hypervisor itself. So I suggest to go the CIM way for
implementing this functionality. What you want is enhancing
CheckIsVSMigratable...() and also the executing methods
MigrateVSTo...(), because these methods have to do the check again, as
between the client's CheckIsVSMigratable...() result and the
MigrateVSTo...() request is enough time, that the result of the check
can became invalid in the meantime. I looked a bit through the Migration
Profile and found a class that I suggest to use for this functionality.
That's the description of this class:
"CIM_VirtualSystemMigrationSettingData defines the parameters to control
a virtual system migration implementation, as managed by a
CIM_VirtualSystemMigrationService. It is expected that a migration
implementation will subclass this class to add implementation-specific
migration options."
An instance of this class is transported to all CheckIsVSMigratable...()
and MigrateVSTo...() via the MigrationSettingData parameter. So we stay
absolutely conform to the Migration Profile. The Xen and KVM subclasses
can check properties, that are well know to cause migration problems.
E.g. if the migration depends on the architecture, this class can get a
property of maybe TargetArchitecture, which needs to be checked by the
provider. Your approach of letting the client parameterize scripts
through this class might look like this:
[Description("An array containing the scripts to be executed as part of
the check, if the migration is possible.")]
string ScriptsToExecute[];
[Description("An array containing the parameters given as input to the
scripts defined by ScriptsToExecute. Each entry in this array is related
to the same array entry in ScriptToExecute. A list of parameters for one
script is divided by space characters.")]
string ScriptsParameterList[];
What do you - and all the others in this list interested in this feature
- think about this ?
Thanks ... Heidi
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor