[libvirt] [PATCH] Clarify behavior or virDomainDetachDevice
Doucment that not all attributes are used for matching. 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. + * * Returns 0 in case of success, -1 in case of failure. */ int diff --git a/tools/virsh.pod b/tools/virsh.pod index 50de32c..2c584d3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2441,7 +2441,9 @@ the device does not use managed mode. 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. +expected. Not every device attribute is checked when matching the device. +For example a network interface can be detatched if the mac and PCI addresses +match, even if the type does not. If I<--live> is specified, affect a running domain. If I<--config> is specified, affect the next startup of a persistent domain. -- 2.0.5
On 20.02.2015 12:39, Ján Tomko wrote:
Doucment that not all attributes are used for matching.
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. 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. NACK, sorry. Michal
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.
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. 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.
NACK, sorry.
Agreed, this docs change will limit our ability to change our impl later if we need to. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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/
On 20.02.2015 14:23, Ján Tomko wrote:
<snip/>
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.
Yes, this is definitely better wording. Moreover, going through our docs, it doesn't seem like we have stated explicitly that users are required to pass full XML. Nor in virsh, nor in APIs docs. ACK to this and adding it to Detach APIs too. Michal
On 02/20/2015 04:39 AM, 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(-)
+++ b/tools/virsh.pod @@ -2441,7 +2441,9 @@ the device does not use managed mode.
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. +expected. Not every device attribute is checked when matching the device. +For example a network interface can be detatched if the mac and PCI addresses +match, even if the type does not.
s/detatched/detached/ (of course, now that I've read the rest of this thread, I see that you aren't using this wording after all...) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
Michal Privoznik