[libvirt] [PATCH 1/2] virsh: Add options for several commands which change domain config

[Osier Yang]

On 06/21/2011 04:55 PM, Michal Privoznik wrote:
> On 21.06.2011 07:37, Osier Yang wrote:
>> All of the following 7 commands just provide one option (--persistent)
>> for user to specify how to affect the domain:
>>      attach-device
>>      detach-device
>>      attach-disk
>>      detach-disk
>>      attach-interface
>>      detach-interface
>>      update-device
>>
>> However, All of the APIs the above 7 commands use: virDomainAttachDeviceFlags,
>> virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support following
>> 3 flags:
>>      VIR_DOMAIN_AFFECT_CURRENT
>>      VIR_DOMAIN_AFFECT_CONFIG
>>      VIR_DOMAIN_AFFECT_LIVE
>>
>> This patch add two new options (--live, --current), and changes
>> "--persistent" into "--config", just as other similar commands,

> I am not fully convinced about this. I mean - it would be nice to have
> unified options names for these commands, but I am afraid we can't
> change them.

That's what I'm worried about too, changing "--persistent" into
"--config" may affect already existed scripts based on virsh. But 
considering the unification, it might be better to change it.

Or perhaps we can just leave "--persistent" unchanged, but it
might be quite confused for user.

>> e.g. "schedinfo", "vcpupin".
>>
>> And since the APIs are designed as: If no flag is specified, behaviour
>> is different depending on hypervisor, so virsh shouldn't do things like:
>>
>>      if (virDomainIsActive(dom) == 1)
>>          flags |= VIR_DOMAIN_AFFECT_LIVE;
>>
>> This patch removes codes like this, leave the determination for underly
>> hypervisor driver.
>> ---
>>   tools/virsh.c |  242 ++++++++++++++++++++++++++++++++++++++++++---------------
>>   1 files changed, 178 insertions(+), 64 deletions(-)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index abc4614..4cd2e1a 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = {
>>   static const vshCmdOptDef opts_attach_device[] = {
>>       {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
>>       {"file",   VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")},
>> -    {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")},
>> +    {"config", VSH_OT_BOOL, 0, N_("affect next boot")},
>> +    {"live", VSH_OT_BOOL, 0, N_("affect running domain")},
>> +    {"current", VSH_OT_BOOL, 0, N_("affect current domain")},
> Maybe it's a bit early morning for me, but I didn't get this
> description. We are always affecting current domain :) I would append
> 'status' word, resulting in "affect current domain status". This makes
> it clear for me. And maybe change description for 'vcpupin' command as
> well. But that could be a follow up patch.

These descriptions used widely in the codes, agree it's a bit confused.

>
> Otherwise looking good.
>
> Michal