[libvirt] [PATCH] Clarify behavior or virDomainDetachDevice

[Ján Tomko]

On Fri, Feb 20, 2015 at 12:43:50PM +0000, Daniel P. Berrange wrote:
> On Fri, Feb 20, 2015 at 01:16:24PM +0100, Michal Privoznik wrote:
> > On 20.02.2015 12:39, Ján Tomko wrote:
> > > Doucment that not all attributes are used for matching.

s/Doucment/Document/

> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=872028
> > > ---
> > >  src/libvirt-domain.c | 5 +++++
> > >  tools/virsh.pod      | 4 +++-
> > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > > index 492e90a..a95c096 100644
> > > --- a/src/libvirt-domain.c
> > > +++ b/src/libvirt-domain.c
> > > @@ -8341,6 +8341,11 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml)
> > >   * into S4 state (also known as hibernation) unless you also modify the
> > >   * persistent domain definition.
> > >   *
> > > + * Note that not all attributes from the XML definition are checked.
> > > + * For example, if the mac address and the pci address specified in the XML
> > > + * match an existing interface, but source and interface type do not,
> > > + * the existing interface will be detached.
> > > + *
> > 
> > I don't think we want to advertise this. Users are required to pass full
> > device XML. If we use only a part of that information to find the
> > device, it's our internal implementation. Not a green light for users to
> > pass minimized XMLs.

Should the docs explicity discourage minimized XMLs?
virsh.pod already has this note:
B<Note>: using of partial device definition XML files may lead to
unexpected results as some fields may be autogenerated and thus
match devices other than expected.


> > Same applies for (partly) inconsistent XMLs (inconsistent to domain
> > XML). If we now teach users it's okay to pass XML that differs to its
> > domain counterpart, we are stuck with it forever. I'd await plenty of
> > bug reports that we broke somebody's flow just because he was passing
> > altered XML and we suddenly needed to change the set of attributes that
> > are checked.

Well, given enough users, every change breaks someone's workflow :) [1]
Whether it's documented or not, I don't think we can suddenly start
checking if the source path matches when detaching a cdrom device.

But my intention was to explicitly document that the mentioned behavior
is not a bug, not to commit to that behavior. Re-reading the patch,
my wording was not ambiguous enough.

> > 
> > NACK, sorry.
> 
> Agreed, this docs change will limit our ability to change our impl
> later if we need to.
> 

Would you object to a more general wording, or should we just not
advertise this at all?
For example:

The supplied XML description of the device should be as specific
as its definition in the domain XML. The set of attributes used
to match the device are internal to the drivers. Using a partial definition,
or attempting to detach a device that is not present in the domain XML,
but shares some specific attributes with one that is present
may lead to unexpected results.

Jan

[1] https://xkcd.com/1172/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150220/7ee2473d/attachment-0001.sig>