[libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command
As the description of removing CDROM media from http://wiki.libvirt.org/page/QEMUSwitchToLibvirt#eject_DEV Add flag 'VSH_OFLAG_EMPTY_OK' to the option 'source' of attach-disk Signed-off-by: Xu He Jie --- 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")}, {"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")}, -- 1.7.5.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command
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. 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 --- 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: 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, " \n", - (isFile) ? "file" : "dev", - source); +if (source) +virBufferAsprintf(&buf, " \n", + (isFile) ? "file" : "dev", + source); virBufferAsprintf(&buf, " \n", target); if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode); -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 --- 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, " \n", - (isFile) ? "file" : "dev", - source); + if (source) + virBufferAsprintf(&buf, " \n", + (isFile) ? "file" : "dev", + source); virBufferAsprintf(&buf, " \n", target); if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode); -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command
On 11/09/2011 10:34 AM, Xu He Jie wrote: 于 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 This will detach the CD drive from guest, for just removing the media, "update-device $domain xml" with the source path removed in the xml will work. We planned to add some new virsh commands such as "insert-media", "eject-media" before to do the work, as "update-device" is not quite visiable, and question like this are frequent in upstream, but unfortunately, it's not implemented yet. It told me: 'unsupported configuration: device type 'disk' cannot be detached' Is this the exact error you saw, it's weried if a device of type disk can't be detached. The error expected should be something like: "unsupported configuration: device type of 'cdrom' cannot be detached" Regards Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command
于 2011年11月09日 11:05, Osier Yang 写道: On 11/09/2011 10:34 AM, Xu He Jie wrote: 于 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 This will detach the CD drive from guest, for just removing the media, "update-device $domain xml" with the source path removed in the xml will work. We planned to add some new virsh commands such as "insert-media", "eject-media" before to do the work, as "update-device" is not quite visiable, and question like this are frequent in upstream, but unfortunately, it's not implemented yet. I understood, thanks. It told me: 'unsupported configuration: device type 'disk' cannot be detached' Is this the exact error you saw, it's weried if a device of type disk can't be detached. The error expected should be something like: "unsupported configuration: device type of 'cdrom' cannot be detached" Yes, the xml as below: the command as below: virsh # detach-disk ubuntu-test-vm1 hdc 错误: Failed to detach disk 错误: unsupported configuration: device type 'disk' cannot be detached Regards Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command
On 11/09/2011 11:29 AM, Xu He Jie wrote: 于 2011年11月09日 11:05, Osier Yang 写道: On 11/09/2011 10:34 AM, Xu He Jie wrote: 于 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 This will detach the CD drive from guest, for just removing the media, "update-device $domain xml" with the source path removed in the xml will work. We planned to add some new virsh commands such as "insert-media", "eject-media" before to do the work, as "update-device" is not quite visiable, and question like this are frequent in upstream, but unfortunately, it's not implemented yet. I understood, thanks. It told me: 'unsupported configuration: device type 'disk' cannot be detached' Is this the exact error you saw, it's weried if a device of type disk can't be detached. The error expected should be something like: "unsupported configuration: device type of 'cdrom' cannot be detached" Yes, the xml as below: the command as below: virsh # detach-disk ubuntu-test-vm1 hdc 错误: Failed to detach disk 错误: unsupported configuration: device type 'disk' cannot be detached I see, there is a bug here. This patch fixed it. http://www.redhat.com/archives/libvir-list/2011-November/msg00355.html Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list