Re: [PATCHv4 9/9] virCaps: get rid of macPrefix field

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

 



On 03/15/2013 09:26 AM, Peter Krempa wrote:
> Use the virDomainXMLConf structure to hold this data and tweak the code
> to avoid semantic change.
> 
> Without configuration the KVM mac prefix is used by default. I chose it
> as it's in the privately administered segment so it should be usable for
> any purposes.
> ---
> 
> Notes:
>     Version 4:
>     - new in series
> 
>  src/conf/capabilities.c          | 14 --------------
>  src/conf/capabilities.h          |  9 ---------
>  src/conf/domain_conf.c           | 26 ++++++++++++++++++++++----
>  src/conf/domain_conf.h           |  3 +++
>  src/esx/esx_driver.c             |  1 -
>  src/libvirt_private.syms         |  3 +--
>  src/libxl/libxl_conf.c           |  2 --
>  src/libxl/libxl_driver.c         |  6 +++++-
>  src/lxc/lxc_conf.c               |  3 ---
>  src/openvz/openvz_conf.c         |  2 --
>  src/openvz/openvz_driver.c       |  2 +-
>  src/parallels/parallels_driver.c | 12 ++++++++----
>  src/phyp/phyp_driver.c           |  4 ----
>  src/qemu/qemu_capabilities.c     |  3 ---
>  src/qemu/qemu_command.c          |  6 +++---
>  src/vbox/vbox_tmpl.c             | 10 +++++++---
>  src/vmware/vmware_conf.c         |  2 --
>  src/vmx/vmx.c                    |  1 +
>  src/xen/xen_driver.c             |  7 ++++++-
>  src/xen/xen_hypervisor.c         |  2 --
>  tests/vmx2xmltest.c              |  1 -
>  tests/xml2vmxtest.c              |  1 -
>  22 files changed, 57 insertions(+), 63 deletions(-)
> 
> +++ b/src/conf/domain_conf.c
> @@ -800,6 +800,16 @@ virDomainXMLConfNew(virDomainDefParserConfigPtr config,
>      if (xmlns)
>          xmlconf->ns = *xmlns;
> 
> +    /* Technically this forbids to use one of Xerox's MAC address prefixes in
> +     * our hypervisor drivers. This shouldn't ever be a problem.
> +     *
> +     * Use the KVM prefix as default as it's in the privately administered
> +     * range */
> +    if (memcmp(xmlconf->config.macPrefix,
> +               (unsigned char[]) {0x00, 0x00, 0x00}, 3))
> +        memcpy(xmlconf->config.macPrefix,
> +               (unsigned char[]) {0x54, 0x52, 0x00}, 3);

I don't see this C99 construct used very often; would it be any more
straightforward to write:

if (xmlconf->conf.macPrefix[0] == 0 &&
    xmlconf->conf.macPrefix[1] == 0 &&
    xmlconf->conf.macPrefix[2] == 0) {
    xmlconf->conf.macPrefix[0] = 0x54;
    xmlconf->conf.macPrefix[1] = 0x52;
}

But that's bikeshedding, so you don't necessarily have to agree.

More importantly, why the magic number '3', instead of...

> +++ b/src/conf/domain_conf.h
> @@ -1975,6 +1975,7 @@ struct _virDomainDefParserConfig {
> 
>      /* data */
>      bool hasWideScsiBus;
> +    unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN];

...the symbolic constant VIR_MAC_PREFIX_BUFLEN?

> +++ b/src/esx/esx_driver.c
> @@ -598,7 +598,6 @@ esxCapsInit(esxPrivate *priv)
>          return NULL;
>      }
> 
> -    virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x0c, 0x29 });

You are removing this prefix from esx://, ...

> 
> +virDomainDefParserConfig libxlDomainDefParserConfig = {
> +    .macPrefix = { 0x00, 0x16, 0x3e },
> +};
> +

> 
> -    /* XXX shouldn't 'borrow' KVM's prefix */
> -    virCapabilitiesSetMacPrefix(caps, (unsigned char []){ 0x52, 0x54, 0x00 });

This is a change to the default lxc:// prefix; since that is a semantic
change, it should probably be a separate patch.

> +++ b/src/vmx/vmx.c
> @@ -521,6 +521,7 @@ VIR_ENUM_IMPL(virVMXControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
> 
>  virDomainDefParserConfig virVMXDomainDefParserConfig = {
>      .hasWideScsiBus = true,
> +    .macPrefix = {0x00, 0x0c, 0x29},
>  };

...but adding it to vmx://.  Are you sure you did this correctly?

I think you need to fix the esx vs. vmx issue in v5.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]