conf: duplicate interface name instead of MAC provided to lookup the interface

[libvirt] [PATCH] conf: duplicate interface name instead of MAC provided to lookup the interface

Posted by Pavel Hrdina 7 years, 6 months ago

Introduced by 6094d6ec7fc9ea3e28c18c880b76858f06a8b129.
Found by running libvirt-perl tests.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/conf/virinterfaceobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
index a6814a6aee..21d76e7507 100644
--- a/src/conf/virinterfaceobj.c
+++ b/src/conf/virinterfaceobj.c
@@ -182,7 +182,7 @@ virInterfaceObjListFindByMACStringCb(void *payload,
     virObjectLock(obj);
 
     if (STRCASEEQ(obj->def->mac, data->matchStr)) {
-        if (VIR_STRDUP(data->macs[data->nmacs], data->matchStr) < 0) {
+        if (VIR_STRDUP(data->macs[data->nmacs], obj->def->name) < 0) {
             data->error = true;
             goto cleanup;
         }
-- 
2.13.6

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

Re: [libvirt] [PATCH] conf: duplicate interface name instead of MAC provided to lookup the interface

Posted by Andrea Bolognani 7 years, 6 months ago

On Fri, 2017-10-20 at 16:07 +0200, Pavel Hrdina wrote:
> Introduced by 6094d6ec7fc9ea3e28c18c880b76858f06a8b129.
> Found by running libvirt-perl tests.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/conf/virinterfaceobj.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
> index a6814a6aee..21d76e7507 100644
> --- a/src/conf/virinterfaceobj.c
> +++ b/src/conf/virinterfaceobj.c
> @@ -182,7 +182,7 @@ virInterfaceObjListFindByMACStringCb(void *payload,
>      virObjectLock(obj);
>  
>      if (STRCASEEQ(obj->def->mac, data->matchStr)) {
> -        if (VIR_STRDUP(data->macs[data->nmacs], data->matchStr) < 0) {
> +        if (VIR_STRDUP(data->macs[data->nmacs], obj->def->name) < 0) {
>              data->error = true;
>              goto cleanup;
>          }

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

As an aside, I think 'macs' is a pretty bad name for the array, since
we're not storing MAC addresses but rather interface names, so using
either 'matches' or 'names' would be much better IMHO. Do you want me
to send a follow-up patch performing the rename?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] conf: duplicate interface name instead of MAC provided to lookup the interface

Posted by Pavel Hrdina 7 years, 6 months ago

On Fri, Oct 20, 2017 at 04:25:13PM +0200, Andrea Bolognani wrote:
> On Fri, 2017-10-20 at 16:07 +0200, Pavel Hrdina wrote:
> > Introduced by 6094d6ec7fc9ea3e28c18c880b76858f06a8b129.
> > Found by running libvirt-perl tests.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/conf/virinterfaceobj.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
> > index a6814a6aee..21d76e7507 100644
> > --- a/src/conf/virinterfaceobj.c
> > +++ b/src/conf/virinterfaceobj.c
> > @@ -182,7 +182,7 @@ virInterfaceObjListFindByMACStringCb(void *payload,
> >      virObjectLock(obj);
> >  
> >      if (STRCASEEQ(obj->def->mac, data->matchStr)) {
> > -        if (VIR_STRDUP(data->macs[data->nmacs], data->matchStr) < 0) {
> > +        if (VIR_STRDUP(data->macs[data->nmacs], obj->def->name) < 0) {
> >              data->error = true;
> >              goto cleanup;
> >          }
> 
> Reviewed-by: Andrea Bolognani <abologna@redhat.com>

Thanks

> As an aside, I think 'macs' is a pretty bad name for the array, since
> we're not storing MAC addresses but rather interface names, so using
> either 'matches' or 'names' would be much better IMHO. Do you want me
> to send a follow-up patch performing the rename?

That would be good idea, feel free to send it and probably push it as
trivial.

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

Re: [libvirt] [PATCH] conf: duplicate interface name instead of MAC provided to lookup the interface

Posted by John Ferlan 7 years, 6 months ago


On 10/20/2017 10:28 AM, Pavel Hrdina wrote:
> On Fri, Oct 20, 2017 at 04:25:13PM +0200, Andrea Bolognani wrote:
>> On Fri, 2017-10-20 at 16:07 +0200, Pavel Hrdina wrote:
>>> Introduced by 6094d6ec7fc9ea3e28c18c880b76858f06a8b129.
>>> Found by running libvirt-perl tests.
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>  src/conf/virinterfaceobj.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
>>> index a6814a6aee..21d76e7507 100644
>>> --- a/src/conf/virinterfaceobj.c
>>> +++ b/src/conf/virinterfaceobj.c
>>> @@ -182,7 +182,7 @@ virInterfaceObjListFindByMACStringCb(void *payload,
>>>      virObjectLock(obj);
>>>  
>>>      if (STRCASEEQ(obj->def->mac, data->matchStr)) {
>>> -        if (VIR_STRDUP(data->macs[data->nmacs], data->matchStr) < 0) {
>>> +        if (VIR_STRDUP(data->macs[data->nmacs], obj->def->name) < 0) {
>>>              data->error = true;
>>>              goto cleanup;
>>>          }
>>
>> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> Thanks
> 
>> As an aside, I think 'macs' is a pretty bad name for the array, since
>> we're not storing MAC addresses but rather interface names, so using
>> either 'matches' or 'names' would be much better IMHO. Do you want me
>> to send a follow-up patch performing the rename?
> 
> That would be good idea, feel free to send it and probably push it as
> trivial.
> 
> Pavel
> 

Doh - thanks for catching this... Who knew the perl tests would do
better than the libvirt tests.

As for [n]macs and naming - that's why I preferred the non-descript
nElems, elems, and maxelems.... <sigh>.

John

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