On Tue, Sep 16, 2014 at 04:50:53PM -0400, Matthew Rosato wrote: To be honest, I must've looked at my previous reviews after reviewing this patch and I saw I am hesitant in the same places I was last time. Anyway, as I said, I'm not that certain with this part of the codebase, so bear with me here :) Why are you starting the device even if we didn't do that before? Even if users attaching stopped devices on purpose don't concern me, why not just call it for type DIRECT ? And in this hunk it changes the interface up even though the function can still fail. Either all three calls in this file make something new, not related to what you describe in the commit message, or I have missed something. And we also never set them back to the original state which is not very nice. Feel free to speak up, if there's a reason for doing this than we can also adjust documentation. If you fail setting the interface online you clean up, but the caller cannot recognize whether you failed starting the device or not. That means that you can, for example, start the domain, it will have the device, but it won't just be down (maybe it'll be even up), but disconnected from the bridge and we will be looking pretty long time for the problem if a bug report with such criteria comes :) Again, I'm not sure we want to run this in so many cases, but looking at the code, most of the cases are covered by this. Anyway... You will not start the device when destination libvirtd is restarted during QEMU_MIGRATION_PHASE_FINISH2 see the first call from qemuProcessRecoverMigration(). It think it would be safest to run it only when resuming migration (or the cases that are affected) and you cannot determine that based on the @reason (unfortunately, because it looks clean). But it needs to be called only on circa 4 places if I looked correctly. I still see very much unnecessary things being done in this patch and that leads me to thinking I'm still missing something. Because I can express myself better with code than sentences, I'd ask what would a patch like the following one be missing (apart from the fact that I haven't even tried it, so it might not work)? diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h index ea201b3..514ff38 100644 --- i/src/conf/domain_conf.h +++ w/src/conf/domain_conf.h @@ -970,6 +970,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; }; /* Used for prefix of ifname of any network name generated dynamically diff --git i/src/lxc/lxc_process.c w/src/lxc/lxc_process.c index ed30c37..5e2e998 100644 --- i/src/lxc/lxc_process.c +++ w/src/lxc/lxc_process.c @@ -336,7 +336,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net), 0) < 0) + virDomainNetGetActualBandwidth(net), + VIR_NETDEV_MACVLAN_CREATE_IFUP) < 0) goto cleanup; ret = res_ifname; diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index eb72451..8106fff 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; } + /* Save vport profile op for later */ + net->vmOp = vmop; + virObjectUnref(cfg); return rc; } diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 5cf235b..1b68114 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -1903,7 +1903,7 @@ static int qemuDomainResume(virDomainPtr dom) } else if (state == VIR_DOMAIN_PAUSED) { if (qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); @@ -3255,7 +3255,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, if (was_running && virDomainObjIsActive(vm)) { rc = qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_SAVE_CANCELED, - QEMU_ASYNC_JOB_SAVE); + QEMU_ASYNC_JOB_SAVE, false); if (rc < 0) { VIR_WARN("Unable to resume guest CPUs after save failure"); event = virDomainEventLifecycleNewFromObj(vm, @@ -3760,7 +3760,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, if (resume && virDomainObjIsActive(vm)) { if (qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_DUMP) < 0) { + QEMU_ASYNC_JOB_DUMP, false) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -3929,7 +3929,7 @@ static void processWatchdogEvent(virQEMUDriverPtr driver, virDomainObjPtr vm, in ret = qemuProcessStartCPUs(driver, vm, NULL, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_DUMP); + QEMU_ASYNC_JOB_DUMP, false); if (ret < 0) virReportError(VIR_ERR_OPERATION_FAILED, @@ -5644,7 +5644,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (header->was_running && !start_paused) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_RESTORED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, true) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to resume domain")); @@ -12554,7 +12554,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_SNAPSHOT) < 0) { + QEMU_ASYNC_JOB_SNAPSHOT, false) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -13418,7 +13418,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_SNAPSHOT) < 0) { + QEMU_ASYNC_JOB_SNAPSHOT, false) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -14349,7 +14349,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_NONE); + QEMU_ASYNC_JOB_NONE, true); if (rc < 0) goto endjob; virObjectUnref(event); @@ -15117,7 +15117,7 @@ qemuDomainBlockPivot(virConnectPtr conn, if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { virObjectEventPtr event = NULL; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index 284cd5a..ea061b2 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -1314,7 +1314,7 @@ qemuMigrationRestoreDomainState(virConnectPtr conn, virDomainObjPtr vm) /* we got here through some sort of failure; start the domain again */ if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_MIGRATION_CANCELED, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) { + QEMU_ASYNC_JOB_MIGRATION_OUT, false) < 0) { /* Hm, we already know we are in error here. We don't want to * overwrite the previous error, though, so we just throw something * to the logs and hope for the best */ @@ -4919,7 +4919,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, */ if (qemuProcessStartCPUs(driver, vm, dconn, VIR_DOMAIN_RUNNING_MIGRATED, - QEMU_ASYNC_JOB_MIGRATION_IN) < 0) { + QEMU_ASYNC_JOB_MIGRATION_IN, true) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 712a25e..d2f4d31 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -593,7 +593,7 @@ qemuProcessFakeReboot(void *opaque) if (qemuProcessStartCPUs(driver, vm, NULL, reason, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); @@ -2944,7 +2944,7 @@ qemuProcessPrepareMonitorChr(virQEMUDriverConfigPtr cfg, int qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn, virDomainRunningReason reason, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, bool startDirectIfaces) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2961,6 +2961,32 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, } VIR_FREE(priv->lockState); + if (startDirectIfaces) { + size_t i = 0; + + for (i = 0; i < vm->def->nnets; i++){ + virDomainNetDefPtr net = vm->def->nets[i]; + + if (virDomainNetGetActualType(net) != VIR_DOMAIN_NET_TYPE_DIRECT) + continue; + + if (virNetDevSetOnline(net->ifname, true) < 0) { + const char *netdev = virDomainNetGetActualDirectDev(net); + virNetDevVPortProfilePtr vport = + virDomainNetGetActualVirtPortProfile(net); + + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + vport, + &net->mac, + netdev, + -1, + net->vmOp)); + + return -1; + } + } + } + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto release; @@ -3145,7 +3171,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, vm->def->name); if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, true) < 0) { VIR_WARN("Could not resume domain %s", vm->def->name); } break; @@ -3186,7 +3212,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, reason == VIR_DOMAIN_PAUSED_UNKNOWN)) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { VIR_WARN("Could not resume domain %s", vm->def->name); } } @@ -3207,7 +3233,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, reason == VIR_DOMAIN_PAUSED_UNKNOWN)) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { VIR_WARN("Could not resume domain %s", vm->def->name); } } @@ -3262,7 +3288,7 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver, reason == VIR_DOMAIN_PAUSED_UNKNOWN)) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { VIR_WARN("Could not resume domain '%s' after migration to file", vm->def->name); } @@ -4622,7 +4648,7 @@ int qemuProcessStart(virConnectPtr conn, /* Allow the CPUS to start executing */ if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_BOOTED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, true) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); diff --git i/src/qemu/qemu_process.h w/src/qemu/qemu_process.h index 5948ea4..6fce0a8 100644 --- i/src/qemu/qemu_process.h +++ w/src/qemu/qemu_process.h @@ -33,7 +33,8 @@ int qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn, virDomainRunningReason reason, - qemuDomainAsyncJob asyncJob); + qemuDomainAsyncJob asyncJob, + bool startDirectIfaces); int qemuProcessStopCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainPausedReason reason, diff --git i/src/util/virnetdevmacvlan.c w/src/util/virnetdevmacvlan.c index c83341c..3216b25 100644 --- i/src/util/virnetdevmacvlan.c +++ w/src/util/virnetdevmacvlan.c @@ -902,10 +902,9 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; } - if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; - } + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP && + (rc = virNetDevSetOnline(cr_ifname, true)) < 0) + goto disassociate_exit; if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) diff --git i/src/util/virnetdevmacvlan.h w/src/util/virnetdevmacvlan.h index 41aa4e2..41b4014 100644 --- i/src/util/virnetdevmacvlan.h +++ w/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags; int virNetDevMacVLanCreate(const char *ifname, -- Martin

On 09/16/2014 04:50 PM, Matthew Rosato wrote: Matthew, I've already pushed your patch, but am trying to add to it for another related purpose and this bit is bothering me. What is the reason for not calling qemuInterfaceStartDevices() when reason is VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED? Is it just to avoid setting IFF_UP on an interface that is already IFF_UP? If that's the only reason, I would prefer to have it *always* called when the CPUs are started (and a corresponding qemuInterfaceStopDevices() called when the CPUs are stopped). Otherwise, it looks to me like it is possible in some situations (migration error recovery perhaps? search for VIR_DOMAIN_RUNNING_UNPAUSED) to have the CPUs running, but the macvtap interfaces *not* IFF_UP. Do you see (or did you experience?) a problem calling qemuInterfaceStartDevices() for all reasons?

On 12/11/2014 02:18 PM, Matthew Rosato wrote: Okay. I've gone back through the archives of the 3 versions of the patch and understand the origin. I've also tested out pausing/unpausing a guest with a macvtap interface when this extra condition is removed, and networking continues to work with no problems. Since there is no specific experienced reason for this restriction on when to call qemuInterface StartDevices() (but rather was just you making reviewers happy :-)), I'm going to send a patch to remove that conditional, then another patch to add qemuInterfaceStopDevices(), and finally one that adds/removes bridge fdb entries for tap devices when the network has macTableManager='libvirt'.