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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list