[libvirt] [PATCH 4/4] parallels: implementation of attach/detach network devices

[Dmitry Guryanov]

On 06/17/2015 03:35 PM, Mikhail Feoktistov wrote:
> In this patch we add VIR_DOMAIN_DEVICE_NET handlers implementation
> for domainAttachDevice and domainDetachDevice callbacks.
>
> As soon as we don't support this operation for hypervisor type domains,
> we implement this functionality for containers only.
>
> In detach procedure we find network device by MAC address.
> Because PrlVmDevNet_GetMacAddress() returns MAC as a UTF-8 encoded
> null-terminated string, we use memcmp() to compare it.
> Also we remove corresponding virtual network by prlsdkDelNetAdapter call.
> ---
>   src/vz/vz_driver.c |   16 +++++++
>   src/vz/vz_sdk.c    |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/vz/vz_sdk.h    |    4 ++
>   3 files changed, 147 insertions(+), 0 deletions(-)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index cef3c77..d9ddd4f 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -1117,6 +1117,14 @@ static int vzDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>               goto cleanup;
>           }
>           break;
> +    case VIR_DOMAIN_DEVICE_NET:
> +        ret = prlsdkAttachNet(privdom, privconn, dev->data.net);
> +        if (ret) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("network attach failed"));

We always report errors where they occurred, so I think you skip this 
message, it will be reported inside prlsdkAttachNet.

> +            goto cleanup;
> +        }
> +        break;
>       default:
>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                          _("device type '%s' cannot be attached"),
> @@ -1186,6 +1194,14 @@ static int vzDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>               goto cleanup;
>           }
>           break;
> +    case VIR_DOMAIN_DEVICE_NET:
> +        ret = prlsdkDetachNet(privdom, privconn, dev->data.net);
> +        if (ret) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("network detach failed"));
> +            goto cleanup;
> +        }
> +        break;
>       default:
>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                          _("device type '%s' cannot be detached"),
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 090a3ad..0f92e52 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -2914,6 +2914,133 @@ static void prlsdkDelNet(vzConnPtr privconn, virDomainNetDefPtr net)
>       PrlHandle_Free(vnet);
>   }
>   
> +int prlsdkAttachNet(virDomainObjPtr dom, parallelsConnPtr privconn, virDomainNetDefPtr net)
> +{
> +    int ret = -1;
> +    parallelsDomObjPtr privdom = dom->privateData;
> +    PRL_HANDLE job = PRL_INVALID_HANDLE;
> +
> +    if (!IS_CT(dom->def)) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("network device cannot be attached"));
> +        goto cleanup;
> +    }
> +
> +    job = PrlVm_BeginEdit(privdom->sdkdom);
> +    if (PRL_FAILED(waitJob(job)))
> +        goto cleanup;
> +
> +    ret = prlsdkAddNet(privdom->sdkdom, privconn, net, IS_CT(dom->def));
> +    if (ret == 0) {
> +        job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE);
> +        if (PRL_FAILED(waitJob(job))) {
> +            ret = -1;
> +            goto cleanup;
> +        }
> +    }
> +
> + cleanup:
There is no cleanup here, so you can just return from the function 
instead of jumping to cleanup section.
> +    return ret;
> +}
> +
> +static int
> +prlsdkGetNetIndex(PRL_HANDLE sdkdom, virDomainNetDefPtr net)
> +{
> +    int idx = -1;
> +    PRL_RESULT pret;
> +    PRL_UINT32 netCount;
> +    PRL_UINT32 i;
> +    PRL_HANDLE adapter = PRL_INVALID_HANDLE;
> +    PRL_UINT32 len;
> +    char adapterMac[PRL_MAC_STRING_BUFNAME];
> +    char netMac[PRL_MAC_STRING_BUFNAME];

Using names 'adapterMac' and 'netMac' is a little confusing, because net 
and adapter are synonyms in this context, I think something like 
'expectedMac' would be better name for netMac.

> +
> +    prlsdkFormatMac(&net->mac, netMac);
> +    pret = PrlVmCfg_GetNetAdaptersCount(sdkdom, &netCount);
> +    prlsdkCheckRetGoto(pret, cleanup);
> +
> +    for (i = 0; i < netCount; ++i) {
> +
> +        pret = PrlVmCfg_GetNetAdapter(sdkdom, i, &adapter);
> +        prlsdkCheckRetGoto(pret, cleanup);
> +
> +        len = sizeof(adapterMac);
> +        memset(adapterMac, 0, sizeof(adapterMac));
> +        pret = PrlVmDevNet_GetMacAddress(adapter, adapterMac, &len);
> +        prlsdkCheckRetGoto(pret, cleanup);
> +
> +        if (memcmp(adapterMac, netMac, PRL_MAC_STRING_BUFNAME)) {
> +
> +            PrlHandle_Free(adapter);
> +            adapter = PRL_INVALID_HANDLE;
> +            continue;
> +        }
> +
> +        idx = i;
> +        break;
> +    }
> +
> + cleanup:
> +    PrlHandle_Free(adapter);
> +    return idx;
> +}
> +
> +static int prlsdkDelNetAdapter(PRL_HANDLE sdkdom, int idx)
> +{
> +    int ret = -1;
> +    PRL_RESULT pret;
> +    PRL_HANDLE sdknet = PRL_INVALID_HANDLE;
> +
> +    pret = PrlVmCfg_GetNetAdapter(sdkdom, idx, &sdknet);
> +    prlsdkCheckRetGoto(pret, cleanup);
> +
> +    pret = PrlVmDev_Remove(sdknet);
> +    prlsdkCheckRetGoto(pret, cleanup);
> +
> +    ret = 0;
> +
> + cleanup:
> +    PrlHandle_Free(sdknet);
> +    return ret;
> +}
> +
> +int prlsdkDetachNet(virDomainObjPtr dom, parallelsConnPtr privconn, virDomainNetDefPtr net)
> +{
> +    int ret = -1, idx = -1;
> +    parallelsDomObjPtr privdom = dom->privateData;
> +    PRL_HANDLE job = PRL_INVALID_HANDLE;
> +
> +    if (!IS_CT(dom->def)) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("network device cannot be detached"));
> +        goto cleanup;
> +    }
> +
> +    idx = prlsdkGetNetIndex(privdom->sdkdom, net);
> +    if (idx < 0)
> +        goto cleanup;

There is a race here: if another thread change network adapters after 
you got idx and before PrlVm_BeginEdit, you will delete wrong adapter.

> +
> +    job = PrlVm_BeginEdit(privdom->sdkdom);
> +    if (PRL_FAILED(waitJob(job)))
> +        goto cleanup;
> +
> +    ret = prlsdkDelNet(privconn, net);
> +    if (ret != 0)
> +        goto cleanup;
> +
> +    ret = prlsdkDelNetAdapter(privdom->sdkdom, idx);
> +    if (ret == 0) {
> +        job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE);
> +        if (PRL_FAILED(waitJob(job))) {
> +            ret = -1;
> +            goto cleanup;
> +        }
> +    }
> +
> + cleanup:
> +    return ret;
> +}
> +
>   static int prlsdkDelDisk(PRL_HANDLE sdkdom, int idx)
>   {
>       int ret = -1;
> diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
> index dd4fecf..cde8904 100644
> --- a/src/vz/vz_sdk.h
> +++ b/src/vz/vz_sdk.h
> @@ -66,3 +66,7 @@ int
>   prlsdkDetachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk);
>   int
>   prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats);
> +int
> +prlsdkAttachNet(virDomainObjPtr dom, parallelsConnPtr privconn, virDomainNetDefPtr net);
> +int
> +prlsdkDetachNet(virDomainObjPtr dom, parallelsConnPtr privconn, virDomainNetDefPtr net);


-- 
Dmitry Guryanov