[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