[libvirt] [PATCH] Don't validate disk type in virsh attach-disk

[Daniel P. Berrange]

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 :|