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

Re: [libvirt] [PATCH] conf: Skip generating random MAC in DetachDevice xml




On 04/18/2016 02:01 PM, Kothapally Madhu Pavan wrote:
> When we try to detach a network device without specifying
> the mac address, random mac address is generated. As the
> generated mac address will not be available in the running
> vm, detaching device will fail erroring out "error:
> operation failed: no device matching mac address
> xx:xx:xx:xx:xx:xx found".

Been on list for a bit without attention - so I'll bite...

Could you perhaps provide some more specifics?  Example that's failing
would really help and of course your thoughts about the next paragraph.

I would think there could be a concern over *matching* which of many
networks was being attempted to be detached.  The 'detach-interface'
virsh page specifically mentions it is recommended to use the mac to
distinguish between the interfaces if more than one are present on the
domain.



> 
> Signed-off-by: Kothapally Madhu Pavan <kmp linux vnet ibm com>
> ---
>  src/conf/domain_conf.c   |    4 ++++
>  src/conf/domain_conf.h   |    3 +++
>  src/libxl/libxl_driver.c |   12 ++++++------
>  src/lxc/lxc_driver.c     |    7 ++++---
>  src/qemu/qemu_driver.c   |    2 ++
>  src/uml/uml_driver.c     |    6 ++++--
>  src/vbox/vbox_common.c   |    6 ++++--
>  src/vz/vz_driver.c       |    4 +++-
>  src/xen/xend_internal.c  |    6 ++++--
>  src/xen/xm_internal.c    |    8 ++++----
>  10 files changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 28248c8..512d877 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8784,6 +8784,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>                             (const char *)macaddr);
>              goto error;
>          }
> +    } else if (flags & VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("mac address not specified in the device xml"));
> +        goto error;

So from any Detach path which you added the flag, if the incoming XML
doesn't have a macaddr, then you get this failure?

I guess from the name I expected :

   } else if !(flags & VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR) {
       virDomainNetGenerateMAC

That is - if I don't have the flag set, then generate the MAC;
otherwise, continue on parsing the XML...  Of course we'll still fail later.

So, I think the better option is, set a flag when we *do* generate the
MAC, then in qemuDomainDetachNetDevice check that generated a mac flag
in order to send a bool to virDomainNetFindIdx to indicate matching the
mac isn't required (expected) and that allows the attempt to match
"other" fields (if possible).

Hopefully that's clear - it's been a long day.

John
>      } else {
>          virDomainNetGenerateMAC(xmlopt, &def->mac);
>      }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1986f53..74692f1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2679,6 +2679,9 @@ typedef enum {
>      VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8,
>      /* allow updates in post parse callback that would break ABI otherwise */
>      VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9,
> +    /* don't generate random mac address when a network device without mac address
> +     * is detached */
> +    VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR = 1 << 10,
>  } virDomainDefParseFlags;
>  
>  typedef enum {
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index bf97c9c..507edcf 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3726,6 +3726,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>      virDomainDefPtr vmdef = NULL;
>      virDomainDeviceDefPtr dev = NULL;
>      int ret = -1;
> +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>  
>      virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
>                    VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
> @@ -3743,9 +3745,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>          goto endjob;
>  
>      if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> -        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
> -                                            cfg->caps, driver->xmlopt,
> -                                            VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> +        if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps,
> +                                            driver->xmlopt, parse_flags)))
>              goto endjob;
>  
>          /* Make a copy for updated domain. */
> @@ -3760,9 +3761,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>      if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
>          /* If dev exists it was created to modify the domain config. Free it. */
>          virDomainDeviceDefFree(dev);
> -        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
> -                                            cfg->caps, driver->xmlopt,
> -                                            VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> +        if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps,
> +                                            driver->xmlopt, parse_flags)))
>              goto endjob;
>  
>          if (libxlDomainDetachDeviceLive(driver, vm, dev) < 0)
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index ef48812..23f0d80 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -5196,6 +5196,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
>      virDomainDefPtr vmdef = NULL;
>      virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
>      int ret = -1;
> +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>      virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> @@ -5213,9 +5215,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
>      if (!(caps = virLXCDriverGetCapabilities(driver, false)))
>          goto cleanup;
>  
> -    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
> -                                             caps, driver->xmlopt,
> -                                             VIR_DOMAIN_DEF_PARSE_INACTIVE);
> +    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps,
> +                                             driver->xmlopt, parse_flags);
>      if (dev == NULL)
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 862c44c..df85fd5 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8486,6 +8486,8 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>          !(flags & VIR_DOMAIN_AFFECT_LIVE))
>          parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
>  
> +    parse_flags |= VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
> +
>      dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
>                                               caps, driver->xmlopt,
>                                               parse_flags);
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 84e1df8..c232435 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -2346,6 +2346,8 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml)
>      virDomainObjPtr vm;
>      virDomainDeviceDefPtr dev = NULL;
>      int ret = -1;
> +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>  
>      umlDriverLock(driver);
>      vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
> @@ -2366,8 +2368,8 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml)
>          goto cleanup;
>      }
>  
> -    dev = virDomainDeviceDefParse(xml, vm->def, driver->caps, driver->xmlopt,
> -                                  VIR_DOMAIN_DEF_PARSE_INACTIVE);
> +    dev = virDomainDeviceDefParse(xml, vm->def, driver->caps,
> +                                  driver->xmlopt, parse_flags);
>      if (dev == NULL)
>          goto cleanup;
>  
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 0cead10..4c539ef 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -4228,6 +4228,8 @@ static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml)
>      virDomainDeviceDefPtr dev = NULL;
>      nsresult rc;
>      int ret = -1;
> +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>  
>      if (!data->vboxObj)
>          return ret;
> @@ -4238,8 +4240,8 @@ static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml)
>  
>      def->os.type = VIR_DOMAIN_OSTYPE_HVM;
>  
> -    dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt,
> -                                  VIR_DOMAIN_DEF_PARSE_INACTIVE);
> +    dev = virDomainDeviceDefParse(xml, def, data->caps,
> +                                  data->xmlopt, parse_flags);
>      if (dev == NULL)
>          goto cleanup;
>  
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index ffa6f45..f5f5395 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -1172,6 +1172,8 @@ static int vzDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>      vzConnPtr privconn = dom->conn->privateData;
>      virDomainDeviceDefPtr dev = NULL;
>      virDomainObjPtr privdom = NULL;
> +    unsigned int parse_flags = VIR_DOMAIN_XML_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>                    VIR_DOMAIN_AFFECT_CONFIG, -1);
> @@ -1184,7 +1186,7 @@ static int vzDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>          goto cleanup;
>  
>      dev = virDomainDeviceDefParse(xml, privdom->def, privconn->driver->caps,
> -                                  privconn->driver->xmlopt, VIR_DOMAIN_XML_INACTIVE);
> +                                  privconn->driver->xmlopt, parse_flags);
>      if (dev == NULL)
>          goto cleanup;
>  
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index cf7cdd0..216c4f4 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -2327,6 +2327,8 @@ xenDaemonDetachDeviceFlags(virConnectPtr conn,
>      int ret = -1;
>      char *xendev = NULL;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
>  
> @@ -2354,8 +2356,8 @@ xenDaemonDetachDeviceFlags(virConnectPtr conn,
>                                       NULL)))
>          goto cleanup;
>  
> -    if (!(dev = virDomainDeviceDefParse(xml, def, priv->caps, priv->xmlopt,
> -                                        VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> +    if (!(dev = virDomainDeviceDefParse(xml, def, priv->caps,
> +                                        priv->xmlopt, parse_flags);
>          goto cleanup;
>  
>      if (virDomainXMLDevID(conn, minidef, dev, class, ref, sizeof(ref)))
> diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
> index ef1a460..ae142a3 100644
> --- a/src/xen/xm_internal.c
> +++ b/src/xen/xm_internal.c
> @@ -1322,6 +1322,8 @@ xenXMDomainDetachDeviceFlags(virConnectPtr conn,
>      int ret = -1;
>      size_t i;
>      xenUnifiedPrivatePtr priv = conn->privateData;
> +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                               VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
>  
> @@ -1340,10 +1342,8 @@ xenXMDomainDetachDeviceFlags(virConnectPtr conn,
>          goto cleanup;
>      def = entry->def;
>  
> -    if (!(dev = virDomainDeviceDefParse(xml, entry->def,
> -                                        priv->caps,
> -                                        priv->xmlopt,
> -                                        VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> +    if (!(dev = virDomainDeviceDefParse(xml, entry->def, priv->caps,
> +                                        priv->xmlopt, parse_flags)))
>          goto cleanup;
>  
>      switch (dev->type) {
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


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