On 01/31/2014 06:43 PM, Michal Privoznik wrote:
> The lack of debug printings might be frustrating in the future.
> Moreover, this function doesn't follow the usual pattern we have in the
> rest of the code:
>
> int ret = -1;
> /* do some work */
> ret = 0;
> cleanup:
> /* some cleanup work */
> return ret;
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/network/bridge_driver.c | 35 +++++++++++++++++++----------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 0b43a67..53c2274 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1995,23 +1995,29 @@ static int
> networkStartNetwork(virNetworkDriverStatePtr driver,
> virNetworkObjPtr network)
> {
> - int ret = 0;
> + int ret = -1;
> +
> + VIR_DEBUG("driver=%p, network=%p", driver, network);
>
> if (virNetworkObjIsActive(network)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("network is already active"));
> - return -1;
> + goto cleanup;
If the network is already active, and has a pending modification (e.g.
made with "virsh net-edit"), the modified network definition will be
stored in network->newDef. But if someone tries to start the network and
it's already active, you're now jumping to cleanup, and cleanup now
calls virNetworkObjUnsetDefTransient(), which will destroy network-def
and *prematurely* move network->newDef into its place. From this point
on, network->def will contain incorrect data, which could have bad
consequences (for example, the wrong set of iptables rules may be
deleted when the network is next destroyed, leaving undesired rules in
effect).
So, while I much prefer the new coding style in the function, you'll
need to privately keep track of whether or not
virNetworkObjSetDefTransient() has been called by this function, and
only "UnsetDef" it if it was "SetDef"ed.
Other than that I think it's okay.
> }
>
> + VIR_DEBUG("Beginning network startup process");
> +
> + VIR_DEBUG("Setting current network def as transient");
> if (virNetworkObjSetDefTransient(network, true) < 0)
> - return -1;
> + goto cleanup;
>
> switch (network->def->forward.type) {
>
> case VIR_NETWORK_FORWARD_NONE:
> case VIR_NETWORK_FORWARD_NAT:
> case VIR_NETWORK_FORWARD_ROUTE:
> - ret = networkStartNetworkVirtual(driver, network);
> + if (networkStartNetworkVirtual(driver, network) < 0)
> + goto cleanup;
> break;
>
> case VIR_NETWORK_FORWARD_BRIDGE:
> @@ -2019,28 +2025,25 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
> case VIR_NETWORK_FORWARD_VEPA:
> case VIR_NETWORK_FORWARD_PASSTHROUGH:
> case VIR_NETWORK_FORWARD_HOSTDEV:
> - ret = networkStartNetworkExternal(driver, network);
> + if (networkStartNetworkExternal(driver, network) < 0)
> + goto cleanup;
> break;
> }
>
> - if (ret < 0) {
> - virNetworkObjUnsetDefTransient(network);
> - return ret;
> - }
> -
> /* Persist the live configuration now that anything autogenerated
> * is setup.
> */
> - if ((ret = virNetworkSaveStatus(driverState->stateDir,
> - network)) < 0) {
> - goto error;
> - }
> + VIR_DEBUG("Writing network status to disk");
> + if (virNetworkSaveStatus(driverState->stateDir, network) < 0)
> + goto cleanup;
>
> - VIR_INFO("Starting up network '%s'", network->def->name);
> network->active = 1;
> + VIR_INFO("Network '%s' started up", network->def->name);
> + ret = 0;
>
> -error:
> +cleanup:
> if (ret < 0) {
> + virNetworkObjUnsetDefTransient(network);
> virErrorPtr save_err = virSaveLastError();
> int save_errno = errno;
> networkShutdownNetwork(driver, network);
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list