[libvirt] [PATCH] dots should be valid characters in interface names
Howdy! I spent some time scratching my head this evening, as libvirt was stripping the target/@dev entry from my interface[@type='ethernet']. Turns out that this was caused by the interface name containing a period, which is rejected by isValidIfname(). As the kernel allows the creation of such interfaces, it is improper for libvirt to reject them. The attached one-liner (built against RHEL6b1's libvirt but still applicable against current master as of 10c681622) fixes this issue.
On 05/19/2010 10:41 PM, Charles Duffy wrote:
Howdy!
I spent some time scratching my head this evening, as libvirt was stripping the target/@dev entry from my interface[@type='ethernet'].
Turns out that this was caused by the interface name containing a period, which is rejected by isValidIfname(). As the kernel allows the creation of such interfaces, it is improper for libvirt to reject them.
The attached one-liner (built against RHEL6b1's libvirt but still applicable against current master as of 10c681622) fixes this issue.
I think we need to add ":" there as well (at least - maybe there are others), don't we?
On 05/19/2010 10:05 PM, Laine Stump wrote:
On 05/19/2010 10:41 PM, Charles Duffy wrote:
The attached one-liner (built against RHEL6b1's libvirt but still applicable against current master as of 10c681622) fixes this issue.
I think we need to add ":" there as well (at least - maybe there are others), don't we?
In dev_valid_name() in net/core/dev.c, the following restrictions are enforced as of 2.6.27: - Empty strings are disallowed - Length must not meet or exceed 16 (IFNAMSIZ) - '.' and '..' are each invalid as freestanding names (but accepted as part of a longer string) - whitespace and '/' characters are prohibited at any point in the string ...and those are the only limitations in play. Frankly, I'm a bit surprised colons are allowed -- my intuition was that they would be permitted only for named IPs rather than devices -- but such is the logic given here, and a bit of experimentation shows that the kernel accepts them (though they trigger bugs in iproute2, which appears in at least one place to make the same assumption I did; fun!) A revised patch is attached. This lifts its logic from its kernel counterpart, and is updated only to permit forward slashes (which, while disallowed for interface names with the kernel, are required for *device* names -- for which the ESX driver happens to overload this field. Ugh).
On 05/19/2010 11:54 PM, Charles Duffy wrote:
A revised patch is attached. This lifts its logic from its kernel counterpart, and is updated only to permit forward slashes (which, while disallowed for interface names with the kernel, are required for *device* names -- for which the ESX driver happens to overload this field. Ugh).
Failed to run "make syntax-check" on the last rev. Oops.
On 05/20/2010 01:06 AM, Charles Duffy wrote:
On 05/19/2010 11:54 PM, Charles Duffy wrote:
A revised patch is attached. This lifts its logic from its kernel counterpart, and is updated only to permit forward slashes (which, while disallowed for interface names with the kernel, are required for *device* names -- for which the ESX driver happens to overload this field. Ugh).
Hm. You know, I'm wondering if we should have "isValidIfname" at all in domain_conf.c. The thing is that if you think about different platforms that this could be running on (Windows, Linux, Solaris, etc), and the number of different drivers that go through here (Qemu, Xen, ESX, Virtualbox, Phyp, openvz, ONE, UML), it seems like we have to allow everything to cover all bases. Indeed, there is very little else in domain_conf.c that does validity checking, probably for just this reason. Charles, Laine, what do you think about just removing this check completely? -- Chris Lalancette
On Thu, May 20, 2010 at 10:12:35AM -0400, Chris Lalancette wrote:
On 05/20/2010 01:06 AM, Charles Duffy wrote:
On 05/19/2010 11:54 PM, Charles Duffy wrote:
A revised patch is attached. This lifts its logic from its kernel counterpart, and is updated only to permit forward slashes (which, while disallowed for interface names with the kernel, are required for *device* names -- for which the ESX driver happens to overload this field. Ugh).
Hm. You know, I'm wondering if we should have "isValidIfname" at all in domain_conf.c. The thing is that if you think about different platforms that this could be running on (Windows, Linux, Solaris, etc), and the number of different drivers that go through here (Qemu, Xen, ESX, Virtualbox, Phyp, openvz, ONE, UML), it seems like we have to allow everything to cover all bases. Indeed, there is very little else in domain_conf.c that does validity checking, probably for just this reason.
Yep, if the check is platform/hypervisor specific, then it shouldn't be in the XML parsers. The parsers should only attempt to validate syntactic correctness, not semantic correctness (with exception of semantics that can be validated based on the capabilities data which accurately reflects the hypervisor specific rules in that case) Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 05/20/2010 10:12 AM, Chris Lalancette wrote:
On 05/20/2010 01:06 AM, Charles Duffy wrote:
On 05/19/2010 11:54 PM, Charles Duffy wrote:
A revised patch is attached. This lifts its logic from its kernel counterpart, and is updated only to permit forward slashes (which, while disallowed for interface names with the kernel, are required for *device* names -- for which the ESX driver happens to overload this field. Ugh).
Hm. You know, I'm wondering if we should have "isValidIfname" at all in domain_conf.c. The thing is that if you think about different platforms that this could be running on (Windows, Linux, Solaris, etc), and the number of different drivers that go through here (Qemu, Xen, ESX, Virtualbox, Phyp, openvz, ONE, UML), it seems like we have to allow everything to cover all bases. Indeed, there is very little else in domain_conf.c that does validity checking, probably for just this reason.
Charles, Laine, what do you think about just removing this check completely?
interface_conf.c also parses interface names, and has no validity checking - anything inside the quotes is acceptable. So yeah, I have no problem with not having a check. (All this talk about valid characters has me thinking about the plethora of websites that have validity checkers for email addresses that don't allow "+". Grrr.)
On 05/20/2010 09:12 AM, Chris Lalancette wrote:
On 05/20/2010 01:06 AM, Charles Duffy wrote:
On 05/19/2010 11:54 PM, Charles Duffy wrote:
A revised patch is attached. This lifts its logic from its kernel counterpart, and is updated only to permit forward slashes (which, while disallowed for interface names with the kernel, are required for *device* names -- for which the ESX driver happens to overload this field. Ugh).
Hm. You know, I'm wondering if we should have "isValidIfname" at all in domain_conf.c. The thing is that if you think about different platforms that this could be running on (Windows, Linux, Solaris, etc), and the number of different drivers that go through here (Qemu, Xen, ESX, Virtualbox, Phyp, openvz, ONE, UML), it seems like we have to allow everything to cover all bases. Indeed, there is very little else in domain_conf.c that does validity checking, probably for just this reason.
Charles, Laine, what do you think about just removing this check completely?
WORKSFORME as well. I'm presuming that it's still appropriate to check against an empty string. Does the attached look right? If so, I'll update the associated RHEL6 ticket [https://bugzilla.redhat.com/show_bug.cgi?id=593907] appropriately.
On 05/20/2010 03:06 PM, Charles Duffy wrote:
Charles, Laine, what do you think about just removing this check completely?
WORKSFORME as well.
I'm presuming that it's still appropriate to check against an empty string. Does the attached look right? If so, I'll update the associated RHEL6 ticket [https://bugzilla.redhat.com/show_bug.cgi?id=593907] appropriately.
Looks like Chris posted a similar patch, but I like yours better. But why the additional inclusion of "c-ctype.h"? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2010 04:45 PM, Eric Blake wrote:
On 05/20/2010 03:06 PM, Charles Duffy wrote:
Charles, Laine, what do you think about just removing this check completely?
WORKSFORME as well.
I'm presuming that it's still appropriate to check against an empty string. Does the attached look right? If so, I'll update the associated RHEL6 ticket [https://bugzilla.redhat.com/show_bug.cgi?id=593907] appropriately.
Looks like Chris posted a similar patch, but I like yours better. But why the additional inclusion of "c-ctype.h"?
Accidental -- updated patch attached -- though I'm perfectly happy with Chris's version too (though it probably ought to clean up the now-unused VALID_IFNAME_CHARS).
participants (5)
-
Charles Duffy -
Chris Lalancette -
Daniel P. Berrange -
Eric Blake -
Laine Stump