On Thu, Feb 19, 2015 at 06:13:01PM -0700, Jim Fehlig wrote:
> Marek Marczykowski-Górecki wrote:
> > On Thu, Feb 19, 2015 at 03:58:30PM -0700, Jim Fehlig wrote:
> >
> >> Marek Marczykowski-Górecki wrote:
> >>
> >>> On Thu, Feb 19, 2015 at 03:10:13PM -0700, Jim Fehlig wrote:
> >>>
> >>>> Jim Fehlig wrote:
> >>>>
> >>>>> Marek Marczykowski-Górecki wrote:
> >>>>>
> >>>>>> On Thu, Feb 19, 2015 at 01:58:02PM -0700, Jim Fehlig wrote:
> >>>>>>
> >>>>>>> Marek Marczykowski-Górecki wrote:
> >>>>>>>
> >>>>>>>> On Thu, Feb 19, 2015 at 11:43:15AM -0700, Jim Fehlig wrote:
> >>>>>>>>
> >>>>>>>>> Marek Marczykowski-Górecki wrote:
> >>>>>>>>>
> >>>>>>>>>> It will not be possible to detach such device later. Also improve
> >>>>>>>>>> logging in such cases.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> >>>>>>>>>> ---
> >>>>>>>>>> src/libxl/libxl_driver.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >>>>>>>>>> 1 file changed, 39 insertions(+), 2 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> >>>>>>>>>> index ce3a99b..005cc96 100644
> >>>>>>>>>> --- a/src/libxl/libxl_driver.c
> >>>>>>>>>> +++ b/src/libxl/libxl_driver.c
> >>>>>>>>>> @@ -2787,6 +2787,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> >>>>>>>>>> int actualType;
> >>>>>>>>>> libxl_device_nic nic;
> >>>>>>>>>> int ret = -1;
> >>>>>>>>>> + char mac[VIR_MAC_STRING_BUFLEN];
> >>>>>>>>>>
> >>>>>>>>>> /* preallocate new slot for device */
> >>>>>>>>>> if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0)
> >>>>>>>>>> @@ -2801,6 +2802,14 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> >>>>>>>>>>
> >>>>>>>>>> actualType = virDomainNetGetActualType(net);
> >>>>>>>>>>
> >>>>>>>>>> + /* -2 means "multiple matches" so then fail also */
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>> No longer true after commit 2fbae1b2. I think you just want to check if
> >>>>>>>>> virDomainNetFindIdx() >= 0, meaning the def already contains a net
> >>>>>>>>> device with the same mac address.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>> But here the error is when the device *is* found, so the opposite case
> >>>>>>>> than already reported as an error by virDomainNetFindIdx.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>> If you find an idx >= 0, then the domain def already contains a net
> >>>>>>> device with the same mac address, right? In that case, you report an
> >>>>>>> error and return failure from libxlDomainAttachNetDevice().
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>> Right, but if I do not find one (idx == -1), I will proceed with
> >>>>>> (possibly successful) adding the device, while the error was already
> >>>>>> reported by virDomainNetFindIdx.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>> Ah, right :-/.
> >>>>>
> >>>>> Another option: introduce virDomainHasNet() to detect if the domain def
> >>>>> already contains the net device.
> >>>>>
> >>>>>
> >>>>>
> >>>> A better option would be to fix this in libxl, for the benefit of other
> >>>> libxl apps.
> >>>>
> >>>>
> >>> Actually libxl has no problem with duplicated mac addresses, its libvirt
> >>> that makes problem.
> >>>
> >> Yeah, it appears duplicate mac addresses are only valid if on different
> >> PCI devices.
> >>
> >
> > Is that true for libvirt generally (all drivers)?
> >
> >
> >> Back to virDomainHasNet? :-) Or checking for the
> >> duplicate directly in libxlDomainAttachNetDevice()?
> >>
> >
> > What do you mean by "directly"? This is exactly what my patch did (until
> > virDomainNetFindIdx stopped reporting duplicates).
> >
>
> I mean coding up the search for an existing mac directly in
> libxlDomainAttachNetDevice(). E.g.
I see, IMO its better with (reusable...) virDomainHasNet.
> static int
> libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> libxlDomainObjPrivatePtr priv,
> virDomainObjPtr vm,
> virDomainNetDefPtr net)
> {
> int actualType;
> libxl_device_nic nic;
> int ret = -1;
> size_t i;
> virDomainDefPtr def = vm->def;
>
> for (i = 0; i < def->nnets; i++) {
> if (virMacAddrCmp(&def->nets[i]->mac, &net->mac) == 0)
> error;
> }
> ...
> }
>
> Regards,
> Jim
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
Attachment:
pgpKgeVtJgfKI.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list