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.

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.

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).

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.