8:56 a.m.
When detaching interface via detach-device or virDomainDetachDevice() we
parse input XML and (probably) generate a random MAC. This leads then into
not finding interface and thus error. When a mac wasn't specified and domain
has exactly one interface, semantic is clear. Otherwise we require <mac> in
input XML.
Michal Privoznik (3):
Introduce flag representing if MAC address of interface was generated
or not.
qemu: Check for generated MACs while detaching interface
xen: Check for generated MACs while detaching interface
src/conf/domain_conf.c | 2 ++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_hotplug.c | 26 ++++++++++++++++++++------
src/xen/xm_internal.c | 3 ++-
4 files changed, 25 insertions(+), 7 deletions(-)
--
1.7.4
8:56 a.m.
New subject: [libvirt] [PATCH 1/3] Introduce flag representing if MAC address of interface was generated or not.
---
src/conf/domain_conf.c | 2 ++
src/conf/domain_conf.h | 1 +
2 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b97c1f0..454f631 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2593,8 +2593,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
(const char *)macaddr);
goto error;
}
+ def->mac_generated = false;
} else {
virCapabilitiesGenerateMac(caps, def->mac);
+ def->mac_generated = true;
}
if (devaddr) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 30aeccc..84e95a1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -346,6 +346,7 @@ typedef virDomainNetDef *virDomainNetDefPtr;
struct _virDomainNetDef {
enum virDomainNetType type;
unsigned char mac[VIR_MAC_BUFLEN];
+ bool mac_generated;
char *model;
union {
struct {
--
1.7.4
10:42 a.m.
New subject: [libvirt] [PATCH 1/3] Introduce flag representing if MAC address of interface was generated or not.
On 02/24/2011 07:56 AM, Michal Privoznik wrote:
---
src/conf/domain_conf.c | 2 ++
src/conf/domain_conf.h | 1 +
2 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b97c1f0..454f631 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2593,8 +2593,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
(const char *)macaddr);
goto error;
}
+ def->mac_generated = false;
} else {
virCapabilitiesGenerateMac(caps, def->mac);
+ def->mac_generated = true;
}
Rather than sticking the flag into the domain definition, I'd rather
pass it via the flags argument to
virDomainDeviceDefParse/virDomainNetDefParseXML - that is, most callers
will want:
virDomainDeviceDefParse(..., 0)
but qemuDomainDetachDevice (in qemu_driver.c) will want:
virDomainDeviceDefParse(..., VIR_DOMAIN_PARSE_NO_GENERATE)
(or maybe VIR_DOMAIN_XML_INACTIVE vs.
VIR_DOMAIN_XML_INACTIVE|VIR_DOMAIN_PARSE_NO_GENERATE)
That is, it's more than just detaching an interface mac addresses that
has an issue. It's anywhere that parsing a device XML snippet to be
used to match an existing device that we want to suppress generation of
additional items in the parse (this includes generating a random mac for
an interface, but may include other generated items). So adding a new
flag that can be used to tell ALL parse routines to avoid generating
extra data is more useful than changing the domain definition to say
whether one piece of information was generated.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:58 a.m.
New subject: [libvirt] [PATCH 1/3] Introduce flag representing if MAC address of interface was generated or not.
On 02/25/2011 05:42 PM, Eric Blake wrote:
On 02/24/2011 07:56 AM, Michal Privoznik wrote:
> ---
> src/conf/domain_conf.c | 2 ++
> src/conf/domain_conf.h | 1 +
> 2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b97c1f0..454f631 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2593,8 +2593,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
> (const char *)macaddr);
> goto error;
> }
> + def->mac_generated = false;
> } else {
> virCapabilitiesGenerateMac(caps, def->mac);
> + def->mac_generated = true;
> }
Rather than sticking the flag into the domain definition, I'd rather
pass it via the flags argument to
virDomainDeviceDefParse/virDomainNetDefParseXML - that is, most callers
will want:
virDomainDeviceDefParse(..., 0)
but qemuDomainDetachDevice (in qemu_driver.c) will want:
virDomainDeviceDefParse(..., VIR_DOMAIN_PARSE_NO_GENERATE)
(or maybe VIR_DOMAIN_XML_INACTIVE vs.
VIR_DOMAIN_XML_INACTIVE|VIR_DOMAIN_PARSE_NO_GENERATE)
That is, it's more than just detaching an interface mac addresses that
has an issue. It's anywhere that parsing a device XML snippet to be
used to match an existing device that we want to suppress generation of
additional items in the parse (this includes generating a random mac for
an interface, but may include other generated items). So adding a new
flag that can be used to tell ALL parse routines to avoid generating
extra data is more useful than changing the domain definition to say
whether one piece of information was generated.
Yes, i firstly thought in similar way. But I don't really need to
suppress generation of random values. I need information whether user
gave say mac in input XML or not. Same apply for pci address (by the
way, we don't support detaching by it now). The idea behind is:
if (<mac was generated> && <pci addr was generated>) {
if (domain->nnets == 1)
device_clone_mac(domain->nnet[0], device)
else
error("don't know which detach")
} else {
search for device
}
If i suppress generation, I wouldn't know if it was generated anyway,
because mac is stored in statically allocated array. One (tricky) way
out of this is to set mac before to some special mac (e.g. all 0's) and
then check for change. But I'd rather avoid it. Changing array to
dynamically allocated is huge amount of work to be done, because it is
hard to find pieces of code touching it, and omitting even one line may
lead into sigegv.
But I agree, sticking the flag into domain definition is not very clean
either.
One way out of this is adding pointer to flag as (another) function
parameter. Would this be acceptable?
Michal
11:40 a.m.
New subject: [libvirt] [PATCH 1/3] Introduce flag representing if MAC address of interface was generated or not.
(I found this mostly-composed message sitting behind a bunch of windows
on my desktop when I decided it was time to reboot just so I could have
an apparently clean slate...)
On 03/01/2011 03:58 AM, Michal Prívozník wrote:
On 02/25/2011 05:42 PM, Eric Blake wrote:
> On 02/24/2011 07:56 AM, Michal Privoznik wrote:
>> ---
>> src/conf/domain_conf.c | 2 ++
>> src/conf/domain_conf.h | 1 +
>> 2 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b97c1f0..454f631 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2593,8 +2593,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
>> (const char *)macaddr);
>> goto error;
>> }
>> + def->mac_generated = false;
>> } else {
>> virCapabilitiesGenerateMac(caps, def->mac);
>> + def->mac_generated = true;
>> }
>
> Rather than sticking the flag into the domain definition, I'd rather
> pass it via the flags argument to
> virDomainDeviceDefParse/virDomainNetDefParseXML - that is, most callers
> will want:
>
> virDomainDeviceDefParse(..., 0)
>
> but qemuDomainDetachDevice (in qemu_driver.c) will want:
>
> virDomainDeviceDefParse(..., VIR_DOMAIN_PARSE_NO_GENERATE)
>
> (or maybe VIR_DOMAIN_XML_INACTIVE vs.
> VIR_DOMAIN_XML_INACTIVE|VIR_DOMAIN_PARSE_NO_GENERATE)
>
> That is, it's more than just detaching an interface mac addresses that
> has an issue. It's anywhere that parsing a device XML snippet to be
> used to match an existing device that we want to suppress generation of
> additional items in the parse (this includes generating a random mac for
> an interface, but may include other generated items). So adding a new
> flag that can be used to tell ALL parse routines to avoid generating
> extra data is more useful than changing the domain definition to say
> whether one piece of information was generated.
>
Personally, I think that a function called "...Parse" should do just
that - parse what is provided an return it in a different format. There
are several cases in libvirt of parse functions that have side effects
like this and, while they are convenient, I think it would be cleaner
for the parsers to stick to what they say their duty is, and for the
callers to do this generation if needed.
Yes, i firstly thought in similar way. But I don't really need to
suppress generation of random values. I need information whether user
gave say mac in input XML or not.
Yes, you don't need to suppress them, but you also don't need to
generate them. You just need to know whether or not the user supplied one.
Same apply for pci address (by the way, we don't support
detaching by
it now). The idea behind is:
if (<mac was generated> && <pci addr was generated>) {
if (domain->nnets == 1)
device_clone_mac(domain->nnet[0], device)
else
error("don't know which detach")
} else {
search for device
}
If i suppress generation, I wouldn't know if it was generated anyway,
because mac is stored in statically allocated array. One (tricky) way
out of this is to set mac before to some special mac (e.g. all 0's)
and then check for change. But I'd rather avoid it. Changing array to
dynamically allocated is huge amount of work to be done, because it is
hard to find pieces of code touching it, and omitting even one line
may lead into sigegv.
In the end, I think that the totally proper way of handling this would
be to either:
1) add a flag to the def called mac_specified (or mac_is_valid or
whatever), that would be 0 when the def is created, and set to 1 any
time a value is given to it,
or
2) as you say, make the mac a dynamically allocated piece of data, so
that a NULL pointer would be an implicit "mac_specified == FALSE".
The same goes for any other piece of data in any other def that is
marked as optional in the RNG.
But that is all a bit pie-in-the-sky, since the existing code works, and
"fixing" it could possibly actually break something :-)
When I recently had the same problem (adding a mac address to a
virNetworkDef), I decided to make the mac address a fixed size array to
simplify memory management and follow the precedent of the mac address
in virDomainNetDef, and add a mac_specified boolean just after it.
Consumers of virNetworkDef who really do need a mac address can generate
one themselves if one isn't given, but those consumers that don't need
one aren't burdened with it. (This has extra importance with objects
that are saved as config, because you otherwise end up with something in
the saved config that the user neither specified nor wants).
But I agree, sticking the flag into domain definition is not very
clean either.
One way out of this is adding pointer to flag as (another) function
parameter. Would this be acceptable?
I think that's even more obfuscated. If having the extra flag in the Def
struct is confusing/unclean, then my vote would go for dynamically
allocating mac when it's there (that's a potential source of null
dereferences and memory leaks, though; everything has a confusion factor
and a price).
8:56 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: Check for generated MACs while detaching interface
When detaching interface without <mac> specified a one is generated
which leads to not found device.
---
src/qemu/qemu_hotplug.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0002af0..54c97db 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1436,18 +1436,32 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver,
virDomainDeviceDefPtr dev,
virBitmapPtr qemuCaps)
{
- int i, ret = -1;
+ int i = 0, ret = -1;
virDomainNetDefPtr detach = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
int vlan;
char *hostnet_name = NULL;
- for (i = 0 ; i < vm->def->nnets ; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
+ if (!vm->def->nnets) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("domain has no interfaces."));
+ goto cleanup;
+ } else if ((vm->def->nnets > 2) &&
(dev->data.net->mac_generated)) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("You must specify mac address in xml file"));
+ goto cleanup;
+ }
- if (!memcmp(net->mac, dev->data.net->mac, sizeof(net->mac))) {
- detach = net;
- break;
+ if ((vm->def->nnets==1) && (dev->data.net->mac_generated)) {
+ detach = vm->def->nets[0];
+ } else {
+ for (i = 0 ; i < vm->def->nnets ; i++) {
+ virDomainNetDefPtr net = vm->def->nets[i];
+
+ if (!memcmp(net->mac, dev->data.net->mac, sizeof(net->mac))) {
+ detach = net;
+ break;
+ }
}
}
--
1.7.4
10:48 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: Check for generated MACs while detaching interface
On 02/24/2011 07:56 AM, Michal Privoznik wrote:
When detaching interface without <mac> specified a one is
generated
which leads to not found device.
---
src/qemu/qemu_hotplug.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0002af0..54c97db 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1436,18 +1436,32 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver,
virDomainDeviceDefPtr dev,
virBitmapPtr qemuCaps)
{
- int i, ret = -1;
+ int i = 0, ret = -1;
virDomainNetDefPtr detach = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
int vlan;
char *hostnet_name = NULL;
- for (i = 0 ; i < vm->def->nnets ; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
+ if (!vm->def->nnets) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("domain has no interfaces."));
+ goto cleanup;
+ } else if ((vm->def->nnets > 2) &&
(dev->data.net->mac_generated)) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("You must specify mac address in xml file"));
+ goto cleanup;
+ }
If we refactor patch 1 to pass an additional flag to the parse routines
to avoid generating the mac in the first place, then this logic needs to
change a bit. It turns into this pseudocode:
match = NULL;
for (i = 0; i < nnets; i++) {
if (nets[i] matches dev) {
if (match) error: ambiguous
match = nets[i]
}
}
if (!match) error: no match
This would also solve Hu Tao's point - if you have two interfaces, it is
then possible to provide xml that lists just the pci address and still
only matches one of the two interfaces, without requiring that the mac
address be provided in the xml.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:56 a.m.
New subject: [libvirt] [PATCH 3/3] xen: Check for generated MACs while detaching interface
---
src/xen/xm_internal.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
index 27cc387..6503d89 100644
--- a/src/xen/xm_internal.c
+++ b/src/xen/xm_internal.c
@@ -1503,7 +1503,8 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml,
for (i = 0 ; i < def->nnets ; i++) {
if (!memcmp(def->nets[i]->mac,
dev->data.net->mac,
- sizeof(def->nets[i]->mac))) {
+ sizeof(def->nets[i]->mac)) ||
+ ((def->nnets == 1) && dev->data.net->mac_generated)) {
virDomainNetDefFree(def->nets[i]);
if (i < (def->nnets - 1))
memmove(def->nets + i,
--
1.7.4
1:24 a.m.
On Thu, Feb 24, 2011 at 03:56:12PM +0100, Michal Privoznik wrote:
When detaching interface via detach-device or virDomainDetachDevice()
we
parse input XML and (probably) generate a random MAC. This leads then into
It's apparently not reasonable to generate random MACs for NICs to be
detached. A better way to fix the problem is to check wheter enough
information is gathered from xml file to uniquely identify the NICs
to be detached, if not, fail and give user a hint. For NICs, the
information could be mac address or pci address.
not finding interface and thus error. When a mac wasn't specified
and domain
has exactly one interface, semantic is clear. Otherwise we require <mac> in
input XML.
Michal Privoznik (3):
Introduce flag representing if MAC address of interface was generated
or not.
qemu: Check for generated MACs while detaching interface
xen: Check for generated MACs while detaching interface
src/conf/domain_conf.c | 2 ++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_hotplug.c | 26 ++++++++++++++++++++------
src/xen/xm_internal.c | 3 ++-
4 files changed, 25 insertions(+), 7 deletions(-)
--
1.7.4
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Thanks,
Hu Tao
5007
days inactive
5019
days old
8 comments
5 participants
participants (5)
-
Eric Blake -
Hu Tao -
Laine Stump -
Michal Privoznik -
Michal Prívozník