Re: [libvirt] [PATCH v3] network: Bring netdevs online later

On 09/16/2014 04:50 PM, Matthew Rosato wrote: I've probably led you on at least three wild goose chases related to this patch - the first was that I had thought that it would be better to unconditionally wait to ifup *all* network devices. It turns out, though, that regular tap devices which will be connected to a bridge should be ifup'ed and attached to the bridge as soon as possible, so that the forwarding delay timer of the bridge can start to count down. (However, at the same point that macvtap interfaces need to be ifup'ed (i.e. your new qemuInterfaceStartDevice()), tap devices connected to bridges need to have their FDB entries updated (if libvirt is managing the bridge fdb), so it is useful to have a function to call here, it just needs to do something different for different types of connections. Also, your addition of the call to qemuInterfaceStartDevice() during qemuDomainChangeNet() almost certainly came from me telling you that something was needed there. I had forgotten, however, that it isn't possible to change the tap or macvtap device of a network connection without completely removing it anyway, because qemu requires completely detaching both ends of the device in order to change the tap, and that means the device disappears from the guest, in other words it's impossible to *change* it in that way, you have to actually remove and re-add it (at one time I had code to do this, but when it failed to work I inquired and learned of this limitation). Finally, I think it was me that suggested it would be good to stow away the vmOp somewhere so it didn't have to be sent up and down the call stack. When I looked at what you're using it for though (calling virNetDevVPortProfileDisassociate() in case of a failed virNetDevSetOnline() in qemuInterfaceStartDevice()), I could see that if you modify qemuInterfaceStartDevice(s) to return an error on failure, the caller's cleanup will always automatically take care of doing the disassociate for you anyway (this is done in qemuProcessStop() as well as the failure cleanup of qemuDomainAttachNetDevice() - if there is a cleanup path where that isn't called, you've got bigger problems than a dangling 802.11qbh association!). So, we *really* don't need the vmOp :-)) (as aside - it turns out that even if we *did* need to do the disassociate ourselves, analysis of the code shows that there is only a single value of vmOp that would cause any change in behavior, and we're guaranteed that vmOp will *never* be set to that value in the cases where we would be called. I spent *a lot* of time figuring that out before I realized we didn't need to call disassociate *at all*) I've removed this as unnecessary, per paragraph 3 above. This *could* be inferred from the other flags, but I like making it explicit as you have done (in the future the CREATE_WITH_TAP might *not* always be the opposite of CREATE_IFUP). removed, as noted in paragraph 3 at the top. For now calling this for a standard tap device (connected to a bridge) has no effect, but I'm leaving it in as it is going to be useful very soon (to update the fdb at the proper time). Well, actually what makes more sense is to put a single call to qemuInterfaceStartDevice() after all the if-elses. For all cases except DIRECT, BRIDGE, and NETWORK it is a NOP, and for those 3 the only difference will be that it gets called after qemuOpenVhostNet() rather than before, and that makes no practical difference (also sets us up better for having it take effect in the future if/when other interface types need special treatment during "start" phase). Removed, as noted in paragraph 2 at the top. Both of these functions should return errors, since failure to ifup an interface was previously grounds for aborting whatever operation was happening. I like the idea of typecasting the result here and putting in in all possible cases (even if empty) so that anyone adding a new type will be notified that they potentially need to add something. The indentation is off here - the "case" should be at the same level as "switch", with contents of the case indented by 4. Actually, in the case of tap devices connected to a bridge, they should be (and are) created IFF_UP (required to start the forwarding delay timers counting down as soon as possible), so doing this here is redundant. What *is* needed here is to update the bridge's fdb if macTableManager='libvirt'. I'm removing the existing call to virNetDevSetOnline() here, and will add in a call to update the fdb in an upcoming patch. As noted in paragraph 4 at the top - if we properly return an error from this function (and qemuInterfaceStartDevices) then we don't need to call virNetDevVPortProfileDisassociate here - it will be handled in the cleanup of our caller. So I'm removing that bit. again - this should return errors rather than ignoring them. Because I'm adding a "goto cleanup" on failure here, we *will* need the braces, but just for future reference if this code had stayed as is, you would need to remove the braces because the body is a single line. changed both to return int. changed to goto cleanup if qemuInterfaceStartDevices() fails, which will guarantee that disassociate is called in the cleanup) ACK with the above modifications. I've made those (see attached diff) and am pushing this as a bugfix.