On 9/16/19 10:44 AM, Nikolay Shirokovskiy wrote: So you're saying this previously failed? Catastrophically (SEGV)? Or did it log an error and refuse to start the domain? Hmm, yeah I hadn't ever thought about it, but I guess that makes sense, since that's the way the tunctl utility works by default (opens /dev/net/tun, then does a TUNSETIFF ioctl with an empty string as the name of the device), and it get devices name tap<N>. Yeah, this option is bad/undesirable. 1) (Nevermind - you answered below) How long has this worked? If it's *very* new, and defining a domain with <target dev=''/> used to fail, then it's actually a *good* thing to fail immediately. 2) If you put the check for present-but-empty target dev in virDomainNetDefValidate(), then the validation will be done any time a new device is parsed, but specifically *won't* be done during the domain parse when libvirtd starts up. This avoids the "disappearing domains" problem you mention (and is specifically why the separate validation functions were added - there is one hypervisor-agnostic function for each type of device, and a separate function for the entire domain (all in conf/domain_conf.c), as well as similar hypervisor-specific validation functions in qemu/qemu_domain.c (look for the functions below qemuDomainDeviceDefValidate(), e.g. the function for network interfaces is qemuDomainDeviceDefValidateNetwork(); no, don't ask me why the name uses a different pattern than the hypervisor-agnostic functions!). (You'll want to add this validation in the hypervisor-agnostic function, BTW.) So you're saying that <target dev=''/> has worked since "forever" (as far as you know), until it stopped working in 4.6.0, and the patch you posted this morning fixes it, right? I vote for putting a check in virDomainNetDefValidate() that errors out if it finds target dev present but empty. There may be some who would say "it's existing behavior and has been like this for a long time, so we have to preserve it", but since the schema doesn't allow it and we've never documented it, I think it's reasonable to disallow it (except for, as you say, existing domain definitions at reboot time; NB: there will still be an error if a domain containing this now-illegal setting is ever edited). Yes, we rely on it to fill in the numbers of the vnet<N> devices. That way we let the kernel prevent duplicate device names rather than needing to (attempt to) prevent races between threads, as we have to do with macvtap devices (since macvtap doesn't have this capability).

On 9/16/19 3:44 PM, Laine Stump wrote: Or maybe we should just check for empty but non-null target dev directly in the parser and replace it with a NULL right away. That way anybody whose code is accidentally setting it to '' will get the proper default behavior instead of a strange error. (I wonder if there's any other strings that provoke a different behavior for '' vs null. I think I remember one, but don't recall what it was...)