On 06/17/2015 08:35 AM, Mikhail Feoktistov wrote:

...

Let network adapter use DHCP server to get network configuration. To do this we use PrlVmDevNet_SetConfigureWithDhcp to enable it and PrlVmDevNet_SetAutoApply to makes necessary settings within guest OS In linux case it creates network startup scripts /etc/sysconfig/network-scripts/ifcfg-ethN and fills it with necessary parameters. (sorry for getting a bit off-topic)

The semantics of this are different for qemu - attaching a network device only presents the hardware to the virtual machine; it doesn't attempt to make any configuration changes in the guest OS. I *think* that the xen and lxc drivers do configure the guest though (at least they pay attention to the <ip> element in the <interface>). So this isn't any criticism of your implementation, I'm just wondering whether or not this will make a difference to any management apps using libvirt.

(actually this also brings to mind an idle thought I've had for awhile - while it's possible to configure an IP address for a qemu guest from the libvirt configuration in some cases (when the guest interface is connected to a libvirt virtual network, you can add a static IP entry to the network's dhcp config), it might be nice to recognize the <ip> element in the <interface>, and use it to dynamically add a static IP entry to the dnsmasq instance for the attached network. Does anyone have an opinion on whether that would be useful/ugly/confusing?) I think it could be useful. Actually, I intended to make a comment to

17.06.2015 16:38, Laine Stump пишет: this patch that we should pay attention to <ip> element in the <interface> and set DHCP only if address isn't configured explicitly, otherwise we should call PrlVmDevNet_SetNetAddresses which will interact with guest tools (if any) and set specified IP addresses with help of guest agent. As for automatic dnsmasq configuration it could be useful too if guest agent is absent. But I wonder, wouldn't this contradict with OpenStack usecase, for example, when dnsmasq is configured with certain IP addresses and other network parameters, and nothing is specified in libvirt config regarding adapter configuration?

...

--- src/vz/vz_sdk.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 98f7a57..090a3ad 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2815,6 +2815,12 @@ static int prlsdkAddNet(PRL_HANDLE sdkdom, pret = PrlVmDevNet_SetMacAddress(sdknet, macstr); prlsdkCheckRetGoto(pret, cleanup);

+ pret = PrlVmDevNet_SetConfigureWithDhcp(sdknet, true); + prlsdkCheckRetGoto(pret, cleanup); + + pret = PrlVmDevNet_SetAutoApply(sdknet, true); + prlsdkCheckRetGoto(pret, cleanup); + if (isCt) { if (net->model) VIR_WARN("Setting network adapter for containers is not "

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