[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] vmx: Better Workstation vmx handling



2012/2/22 Jean-Baptiste Rouault <jean-baptiste rouault diateam net>:
> This patch adds support for vmx files with empty networkName
> values (which is the case for vmx generated by Workstation).
>
> It also adds support for vmx containing NATed network interfaces.
> ---
>  src/vmx/vmx.c |   29 ++++++++++++++++++-----------
>  1 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 823d5df..3cc3b10 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2418,12 +2418,20 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
>     }
>
>     /* vmx:networkName -> def:data.bridge.brname */
> -    if ((connectionType == NULL ||
> -         STRCASEEQ(connectionType, "bridged") ||
> -         STRCASEEQ(connectionType, "custom")) &&
> -        virVMXGetConfigString(conf, networkName_name, &networkName,
> -                              false) < 0) {
> -        goto cleanup;
> +    if (connectionType == NULL ||
> +        STRCASEEQ(connectionType, "bridged") ||
> +        STRCASEEQ(connectionType, "custom")) {
> +        if (virVMXGetConfigString(conf, networkName_name, &networkName,
> +                                  true) < 0)
> +            goto cleanup;

When networkName is NULL then there was no ethernet0.networkName entry
in the VMX file at all. Setting it to an empty string here will result
in an ethernet0.networkName = "" entry in the VMX file when the domain
XML config is reformatted to VMX again.

I'm not sure that this is correct. In general, a missing VMX entry
means that the hypervisor should use the default value for this entry,
but explicitly giving it an empty value will probably result in
different behavior.

As VIR_DOMAIN_NET_TYPE_BRIDGE requires a bridge name networkName
cannot be NULL. So we need a special string to indicate that
ethernet0.networkName is missing. This could be an empty string. To do
this virVMXFormatEthernet needs to be changed to not add an
ethernet0.networkName entry when def->data.bridge.brname is an empty
string.

A minor problem with this approach is that parsing and reformatting
and VMX file with ethernet0.networkName = "" will result in removing
this entry as "" is used as special value now.

> +        if (networkName == NULL) {
> +            networkName = strdup("");
> +            if (networkName == NULL) {
> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +        }
>     }
>
>     /* vmx:vnet -> def:data.ifname */
> @@ -2447,11 +2455,10 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
>                   connectionType, connectionType_name);
>         goto cleanup;
>     } else if (STRCASEEQ(connectionType, "nat")) {
> -        /* FIXME */
> -        VMX_ERROR(VIR_ERR_INTERNAL_ERROR,
> -                  _("No yet handled value '%s' for VMX entry '%s'"),
> -                  connectionType, connectionType_name);
> -        goto cleanup;
> +        (*def)->type = VIR_DOMAIN_NET_TYPE_USER;
> +        (*def)->model = virtualDev;
> +
> +        virtualDev = NULL;
>     } else if (STRCASEEQ(connectionType, "custom")) {
>         (*def)->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
>         (*def)->model = virtualDev;

You're missing the counterpart in virVMXFormatEthernet to handle
VIR_DOMAIN_NET_TYPE_USER.

And this needs extra testcases in the vmx2xml and xml2vmx tests to
cover this additions to the VMX parser before I can ACK it.

A good start but it needs a v2.

-- 
Matthias Bolte
http://photron.blogspot.com


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]