[netcf-devel] [PATCH] Fill in interface details for bridge, bond, and vlan in ncf_if_xml_state.

Laine Stump laine at laine.org
Wed Nov 4 05:51:47 UTC 2009


On 11/03/2009 07:08 PM, David Lutterkort wrote:
> On Mon, 2009-11-02 at 00:08 -0500, Laine Stump wrote:
>    
>> diff --git a/src/dutil.c b/src/dutil.c
>> index d4eea90..d82b3d8 100644
>> --- a/src/dutil.c
>> +++ b/src/dutil.c
>> @@ -509,40 +513,94 @@ int if_is_active(struct netcf *ncf, const char *intf) {
>>      
>    
>> +char *if_bridge_phys_name(struct netcf *ncf, const char *intf) {
>> +    /* We can learn the name of the physical interface associated
>> +     * with this bridge by looking for the name of the one and
>> +     * only link in /sys/class/net/$ifname/brif.
>> +     *
>> +     * The caller of this function must free the string that is
>> +     * returned.
>> +     *
>> +     * For efficiency's sake, this function assumes we've already
>> +     * checked that this is a bridge interface.
>> +     */
>> +    char *ret = NULL;
>> +    char *dirpath = NULL;
>> +    DIR *dir = NULL;
>> +
>> +    xasprintf(&dirpath, "/sys/class/net/%s/brif", intf);
>> +    ERR_NOMEM(dirpath == NULL, ncf);
>> +
>> +    dir = opendir(dirpath);
>> +    if (dir != NULL) {
>> +        struct dirent *d;
>> +
>> +        while ((d = readdir (dir)) != NULL) {
>> +            if (STRNEQ(d->d_name, ".")&&  STRNEQ(d->d_name, "..")) {
>> +                xasprintf(&ret, "%s", d->d_name);
>> +                ERR_NOMEM(ret == NULL, ncf);
>> +                break;
>> +            }
>> +        }
>> +    }
>>      
> It's not common in the virtualization world, but perfectly permissible
> to have more than one interface in a bridge. This code will report only
> one.
>    


Ah, I'd never tried that. Fixed in the re-hashed patch I just sent.

>    
>> +static void add_ethernet_info_cb(struct nl_object *obj, void *arg) {
>> +    struct nl_ethernet_callback_data *cb_data = arg;
>>       struct rtnl_link *iflink = (struct rtnl_link *)obj;
>> -    struct netcf *ncf = cb_data->nif->ncf;
>> +    struct netcf *ncf = cb_data->ncf;
>>
>>       struct nl_addr *addr;
>> -    char mac_str[64];
>>       xmlNodePtr cur;
>>       xmlAttrPtr prop = NULL;
>>
>> -    if (cb_data->mac != NULL)
>> +    /* look for mac address */
>> +    if ((cb_data->mac == NULL)
>> +&&  ((addr = rtnl_link_get_addr(iflink)) != NULL)
>> +&&  !nl_addr_iszero(addr)) {
>> +
>> +        char mac_str[64];
>> +
>> +        nl_addr2str(addr, mac_str, sizeof(mac_str));
>> +
>> +        for (cur = cb_data->root->children; cur != NULL; cur = cur->next) {
>> +            if ((cur->type == XML_ELEMENT_NODE)&&
>> +                xmlStrEqual(cur->name, BAD_CAST "mac")) {
>> +                cb_data->mac = cur;
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (cb_data->mac == NULL) {
>> +            /* No mac node exists in the document, create a new one.
>> +             */
>> +            cb_data->mac = xmlNewDocNode(cb_data->doc, NULL,
>> +                                         BAD_CAST "mac", NULL);
>> +            ERR_NOMEM(cb_data->mac == NULL, ncf);
>> +
>> +            cur = xmlAddChild(cb_data->root, cb_data->mac);
>> +            if (cur == NULL) {
>> +                xmlFreeNode(cb_data->mac);
>> +                cb_data->mac = NULL;
>> +                report_error(ncf, NETCF_ENOMEM, NULL);
>> +                goto error;
>> +            }
>> +        }
>>      
> This type of code (search for an element, add one if it doesn't exist)
> appears in quite a few places now - could that be factored into its own
> static function ?
>    


Yeah, I'd earlier gotten a bad feeling about repeating that code and 
tried making a utility function that added the node, the subnode, and 
the property, but that was just too much. Your suggestion is just the 
right amount. I made one that looks for an existing node and returns it, 
and another that just creates a new node no matter what (and links it to 
the parent). The two of those really reduced the clutter a lot!


>    
>> +    /* Add in type-specific info of master interface */
>> +    master_ifindex = rtnl_link_name2i(ncf->driver->link_cache, master_name);
>> +    ERR_THROW((master_ifindex == RTNL_LINK_NOT_FOUND), ncf, ENETLINK,
>> +              "couldn't find ifindex for vlan master interface `%s`",
>> +              master_name);
>>      
> Just a small stylistic nit: there's no need to enclose the conditions
> for ERR_THROW/ERR_NOMEM in parens.
>    

Adjusted.



More information about the netcf-devel mailing list