Re: [Libvir] [PATCH] check file format in virsh attach/detach-device

[Daniel Veillard <veillard redhat com>]

On Tue, Jul 31, 2007 at 06:46:19PM +0900, Masayuki Sunou wrote:
> Hi
> 
> virsh attach/detach-devich does not check file format now.
> 
> This patch checks config file format is XML or not.
> (in virsh attach/detach-device)
> If file format is not XML, just return the error with message.
> 
> Signed-off-by: Masayuki Sunou <fj1826dm aa jp fujitsu com>
>                Atsushi SAKAI  <sakaia jp fujitsu com>

  Sorry I will have to reject this ! You are breaking the layering, and
the API.

  http://libvirt.org/html/libvirt-libvirt.html#virDomainAttachDevice

  Returns:	0 in case of success, -1 in case of failure.

your patch makes it return -2 you are breaking the API, this is not acceptable.
An error should be described as precisely as possible when detected. This
should not be handled in an application specific way, so to me this is wrong
in 2 ways.

The routine inside the Xen driver should indicate *why* it could not load the
file, and be as precise as possible, virParseXMLDevice should be refined
to provide a better error report instead. We already have a specific error
code for this VIR_ERR_XML_ERROR, just use it.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/