Re: [libvirt] [PATCH] Fix adding ports to OVS bridges without VLAN tags
On 09/04/2012 05:03 PM, Kyle Mestery (kmestery) wrote:
On Aug 31, 2012, at 9:09 AM, Daniel Veillard wrote:
> On Fri, Aug 31, 2012 at 01:32:34PM +0000, Kyle Mestery (kmestery) wrote:
>> On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
>>> On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote:
> [...]
>>> Still there is something which looks wrong, if we don't have a profileID
>>> why do we end up with "" instead of NULL ? I'm seeing various
tests for
>>> profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me.
>>> if there is no data, store NULL ! Then test for profileID instead of
>>> profileID[0]. Then there is no risk of a crash because abscence of data
>>> led to NULL instead of an empty string, the code is more resilient !
>>>
>>> I expect a followup patch cleaning this up, but after 0.10.1 ...
>>> thanks !
>>>
>> Thanks Daniel, I'll work on the followup patch today.
> No hurry, because I just rolled 0.10.1 out so that won't make it
> (and it's not urgent). Giving 0.10.1 a try would be nice too,
>
> thanks !
>
> Daniel
Hi Daniel:
Picking this back up. struct _virNetDevVPortProfile contains the following:
/* this member is used when virtPortType == 802.1Qbh|openvswitch */
/* this is a null-terminated character string */
char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
To address your comments around checking for profileID[0], we could make
profileID here a pointer, and allocate it when we allocate a struct
_virNetDevVPortProfile
object. But to me, having a fixed length string in this structure doesn't seem
wrong.
Copying Laine here as well for his comments, but I'm inclined to leave things as they
I'm not sure why all the strings in virNetDevVPortProfile were made
fixed length, but that's what we ended up with (possibly because the
initial fields were all fixed length, and the person adding things that
could be variable length wanted to avoid potentially forgetting to free
a subordinate memory chunk when freeing a virNetDevVPortProfile? Or
maybe netlink really does limit the maximum length of the profileID?).
As a matter of fact, that's why the "xxx_specified" booleans are there -
because there is no implicit "specified" flag due to a pointer being
NULL/non-NULL.
I'm with Daniel in preferring the string attributes of
virNetDevVPortProfile to each be a pointer to a separate chunk of memory
(and we should maybe do that in a cleanup patch), but in this case the
new code added by Kyle is correct.