[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



于 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 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);



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]