[libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command

[Xu He Jie]

于 2011年11月09日 07:08, Eric Blake 写道:
> On 11/08/2011 12:16 AM, Xu He Jie wrote:
>> As the description of removing CDROM media from
>> http://wiki.libvirt.org/page/QEMUSwitchToLibvirt#eject_DEV
>
> Hmm.
> virsh attach-disk --type cdrom --mode readonly myguest "" hdc
> might look a bit nicer as:
> virsh attach-disk --type cdrom --mode readonly myguest --target hdc
> except that we marked --source as a required argument, so we have to 
> provide something even when there is no real source. So I agree that 
> we need your patch at a bare minimum to support this documented 
> command line for adding a cdrom drive without a disk.
Is there any other method of removing media from cdrom? I try with:
detach-disk myguest hdc
It told me: 'unsupported configuration: device type 'disk' cannot be 
detached'

>
> Meanwhile, conf/domain_conf.c has this comment:
>
> /* People sometimes pass a bogus '' source path
> when they mean to omit the source element
> completely. eg CDROM without media. This is
> just a little compatability check to help
> those broken apps */
> if (source && STREQ(source, ""))
> VIR_FREE(source);
>
> So while passing an empty string down through the XML eventually does 
> the right thing, we make it sound like it is a gross hack, and that it 
> would be nicer if virsh weren't one of "those broken apps" that 
> exploits the xml hack.
>
>>
>> Add flag 'VSH_OFLAG_EMPTY_OK' to the option 'source' of attach-disk
>>
>> Signed-off-by: Xu He Jie<xuhj at linux.vnet.ibm.com>
>> ---
>> tools/virsh.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 5544a41..e7eae74 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -11569,7 +11569,7 @@ static const vshCmdInfo info_attach_disk[] = {
>>
>> static const vshCmdOptDef opts_attach_disk[] = {
>> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
>> - {"source", VSH_OT_DATA, VSH_OFLAG_REQ, N_("source of disk device")},
>> + {"source", VSH_OT_DATA, VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, 
>> N_("source of disk device")},
>
> ACK to your change, but I'm reformatting it to fit 80 columns, then 
> squashing in this additional change before pushing so that virsh will 
> be a model citizen:

Thanks!

>
> diff --git i/tools/virsh.c w/tools/virsh.c
> index e7eae74..eed727b 100644
> --- i/tools/virsh.c
> +++ w/tools/virsh.c
> @@ -11569,7 +11569,8 @@ static const vshCmdInfo info_attach_disk[] = {
>
> static const vshCmdOptDef opts_attach_disk[] = {
> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> - {"source", VSH_OT_DATA, VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, 
> N_("source of disk device")},
> + {"source", VSH_OT_DATA, VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
> + N_("source of disk device")},
> {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")},
> {"driver", VSH_OT_STRING, 0, N_("driver of disk device")},
> {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")},
> @@ -11754,6 +11755,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd 
> *cmd)
>
> if (vshCommandOptString(cmd, "source", &source) <= 0)
> goto cleanup;
> + /* Allow empty string as a placeholder that implies no source, for
> + * use in adding a cdrom drive with no disk. */
> + if (!*source)
> + source = NULL;
>
> if (vshCommandOptString(cmd, "target", &target) <= 0)
> goto cleanup;
> @@ -11808,9 +11813,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd 
> *cmd)
> if (driver || subdriver || cache)
> virBufferAddLit(&buf, "/>\n");
>
> - virBufferAsprintf(&buf, " <source %s='%s'/>\n",
> - (isFile) ? "file" : "dev",
> - source);
> + if (source)
> + virBufferAsprintf(&buf, " <source %s='%s'/>\n",
> + (isFile) ? "file" : "dev",
> + source);
> virBufferAsprintf(&buf, " <target dev='%s'/>\n", target);
> if (mode)
> virBufferAsprintf(&buf, " <%s/>\n", mode);
>