[libvirt] [PATCH] Don't validate disk type in virsh attach-disk
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached. Thanks, Cole
Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
Hi Cole, Your patch would do the job. Before: $ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: No support floppy in command 'attach-disk' with the patch: $ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: this function is not supported by the hypervisor: virDomainAttachDevice But consider what happens with e.g., "--type bogus". Before, it'd mention the invalid type, "bogus". With the patch, it doesn't (at least not using the test driver). So maybe it'd be better to keep the up-front type check. Of course, if we do keep the test in cmdAttachDisk, the format string should be changed to this: vshError(ctl, FALSE, _("No %s support in command 'attach-disk'"), type);
commit 3481061f6ed960269d9b29a4a1380367d557cf37 Author: Cole Robinson <crobinso@redhat.com> Date: Fri Feb 27 10:47:49 2009 -0500
Let virt driver validate disk type for us in virsh attach-disk.
diff --git a/src/virsh.c b/src/virsh.c index 298dde0..5c9c49f 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -5022,13 +5022,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) type = vshCommandOptString(cmd, "type", NULL); mode = vshCommandOptString(cmd, "mode", NULL);
- if (type) { - if (STRNEQ(type, "cdrom") && STRNEQ(type, "disk")) { - vshError(ctl, FALSE, _("No support %s in command 'attach-disk'"), type); - goto cleanup; - } - } - if (driver) { if (STREQ(driver, "file") || STREQ(driver, "tap")) { isFile = 1;
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Jim Meyering wrote:
Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
Hi Cole, Your patch would do the job. Before:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: No support floppy in command 'attach-disk'
with the patch:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: this function is not supported by the hypervisor: virDomainAttachDevice
But consider what happens with e.g., "--type bogus". Before, it'd mention the invalid type, "bogus". With the patch, it doesn't (at least not using the test driver). So maybe it'd be better to keep the up-front type check.
The idea here is that virsh isn't the one to decide. It could very well be that driver foo doesn't support cdrom devices, so even though virsh lets it through, it's still not valid. Only AttachDevice can give us the full picture. Sounds like we should just implement AttachDevice for the test driver: it could parse the device xml and unconditionally insert it into the guest config. This would certainly be useful in testing the above virsh commands (and for testing device attaches in virt-manager, among others no doubt). Thanks, Cole
Cole Robinson wrote:
Jim Meyering wrote:
Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
Hi Cole, Your patch would do the job. Before:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: No support floppy in command 'attach-disk'
with the patch:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: this function is not supported by the hypervisor: virDomainAttachDevice
But consider what happens with e.g., "--type bogus". Before, it'd mention the invalid type, "bogus". With the patch, it doesn't (at least not using the test driver). So maybe it'd be better to keep the up-front type check.
The idea here is that virsh isn't the one to decide. It could very well be that driver foo doesn't support cdrom devices, so even though virsh lets it through, it's still not valid. Only AttachDevice can give us the full picture.
Sure, I understand that, but if the result of dropping the harmless (once fixed) up-front check is a diagnostic that is less helpful, why drop it? Especially with a 6 or 7-parameter command, it's important to point out which part is causing trouble, whenever possible. The alternative is to give better diagnostics from each and every driver.
Sounds like we should just implement AttachDevice for the test driver: it could parse the device xml and unconditionally insert it into the
Changing how the test driver handles this case won't give a better diagnostic for all the other drivers.
guest config. This would certainly be useful in testing the above virsh commands (and for testing device attaches in virt-manager, among others
On Mon, Mar 02, 2009 at 05:29:58PM +0100, Jim Meyering wrote:
Cole Robinson wrote:
Jim Meyering wrote:
Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
Hi Cole, Your patch would do the job. Before:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: No support floppy in command 'attach-disk'
with the patch:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: this function is not supported by the hypervisor: virDomainAttachDevice
But consider what happens with e.g., "--type bogus". Before, it'd mention the invalid type, "bogus". With the patch, it doesn't (at least not using the test driver). So maybe it'd be better to keep the up-front type check.
The idea here is that virsh isn't the one to decide. It could very well be that driver foo doesn't support cdrom devices, so even though virsh lets it through, it's still not valid. Only AttachDevice can give us the full picture.
Sure, I understand that, but if the result of dropping the harmless (once fixed) up-front check is a diagnostic that is less helpful, why drop it?
Especially with a 6 or 7-parameter command, it's important to point out which part is causing trouble, whenever possible.
THe point is though, that this parameter is *not* causing trouble with the test driver. The original warning made it appear as if hotplug worked for the test driver, but 'floppy' was not supported. This is not actaully the case - hotplug is not supported at all, and the new error message reflects this accurately for the test driver.
The alternative is to give better diagnostics from each and every driver.
Sounds like we should just implement AttachDevice for the test driver: it could parse the device xml and unconditionally insert it into the
Changing how the test driver handles this case won't give a better diagnostic for all the other drivers.
At the same time though you don't want to fill virsh up with a pile of different error reporting logic for each & every driver. The fine grained error reporting needs to be in the drivers themselves, so all users of the API get correct error messages, and not just virsh. As such having it in virsh is at best duplicating information, or at worst just giving wrong information. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote: ...
Changing how the test driver handles this case won't give a better diagnostic for all the other drivers.
At the same time though you don't want to fill virsh up with a pile of different error reporting logic for each & every driver. The fine grained error reporting needs to be in the drivers themselves, so all users of the API get correct error messages, and not just virsh. As such having it in virsh is at best duplicating information, or at worst just giving wrong information.
We agree on that: virsh's UI should give good diagnostics, and those should be derivable from the results returned by the API. While that's fine in the long run, it's a shame to see a short-term regression (worse diagnostic) while we wait for all of the drivers to perform this check. Actually, it seems like each driver could expose which types it accepts (or provide a validType predicate), and then virsh could use that. Cole, Note that if you do drop the existing test, you'll have to make an additional change. Otherwise, an invalid "type" could hose the resulting XML. You'd have to xml-quote that string before adding it to the XML you eventually pass to virDomainAttachDevice.
On Mon, Mar 02, 2009 at 11:18:24AM -0500, Cole Robinson wrote:
Jim Meyering wrote:
Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
Hi Cole, Your patch would do the job. Before:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: No support floppy in command 'attach-disk'
with the patch:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: this function is not supported by the hypervisor: virDomainAttachDevice
But consider what happens with e.g., "--type bogus". Before, it'd mention the invalid type, "bogus". With the patch, it doesn't (at least not using the test driver). So maybe it'd be better to keep the up-front type check.
The idea here is that virsh isn't the one to decide. It could very well be that driver foo doesn't support cdrom devices, so even though virsh lets it through, it's still not valid. Only AttachDevice can give us the full picture.
Sounds like we should just implement AttachDevice for the test driver: it could parse the device xml and unconditionally insert it into the guest config. This would certainly be useful in testing the above virsh commands (and for testing device attaches in virt-manager, among others no doubt).
I concurr - the test driver should support all possible APIs and XML element options that we have available - the more the better. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Mar 02, 2009 at 05:06:48PM +0100, Jim Meyering wrote:
Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
Hi Cole, Your patch would do the job. Before:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: No support floppy in command 'attach-disk'
with the patch:
$ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly error: this function is not supported by the hypervisor: virDomainAttachDevice
But consider what happens with e.g., "--type bogus". Before, it'd mention the invalid type, "bogus". With the patch, it doesn't (at least not using the test driver). So maybe it'd be better to keep the up-front type check.
IMHO, the new diagnostic is correct, because the test driver does not support *any* hotplug whatsoever. Thus there is no concept of a particular type being bogus for the test driver - floppy, cdrom, disk, bogus, they're all equally unsupported by the test driver, since it has no hotplug support, which is what the new error message shows more clearly. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Feb 27, 2009 at 11:14:09AM -0500, Cole Robinson wrote:
virsh attempts to validate the requested disk type, rather than just let the underlying driver do it. This was erroneously denying floppy device attaches. Patch attached.
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering