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

[Peter Krempa]

On 03/29/13 18:39, Eric Blake wrote:
> 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);

Regarding a comment later in the review. This line is flawed and the 
default and most commonly used prefix should be 0x52, 0x54. ...

>
> 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://, ...

esx uses the VMX backend to do the parsing and other stuff ... [2]

>
>>
>> +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.

... thus this doesn't make a semantic change. The funny stuff is that 
the test suite doesn't care about this kind of mistake ...

>> +++ 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?

[2] ... so adding here is in fact accurate and also the changes in the 
tests confirm this.

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

Peter