I think this is a nice improvement. ack
On Mon, Jun 20, 2011 at 7:53 AM, Russell Bryant
<russell(a)russellbryant.net> wrote:
1) Refactor the network API a bit to avoid exposing any sigar data
structures. This makes it so the usage of sigar is an internal
implementation detail and is not exposed at all through the API.
2) Fix memory leaks of the interface list. All users of the function to
get the list of network interfaces leaked some or all of the memory.
This is now cleaned up. There is documentation on how this cleanup
should be performed in network.h.
3) Fix memory leaks of the MAC address of a network interface. The API
call returned dynamically allocated memory and the users of the API call
did not clean up. This function now accepts a buffer to hold the
result, which avoids the need for allocating any memory.
4) Fix memory leaks of the IP address of a network interface. The same
situation as the MAC address API apply here.
5) Add doxygen comments for various things along the way.
Signed-off-by: Russell Bryant <russell(a)russellbryant.net>
---
src/include/matahari/network.h | 105 ++++++++++++++++++++++++++++++-
src/lib/network.c | 134 ++++++++++++++++++++++++++++------------
src/network/network-dbus.c | 26 +++++---
src/network/network-qmf.cpp | 24 +++++---
4 files changed, 227 insertions(+), 62 deletions(-)
diff --git a/src/include/matahari/network.h b/src/include/matahari/network.h
index b35d054..89effe4 100644
--- a/src/include/matahari/network.h
+++ b/src/include/matahari/network.h
@@ -28,13 +28,110 @@
#include <glib.h>
#include <stdint.h>
-extern GList *mh_network_get_interfaces(void);
-extern const char *mh_network_get_ip_address(const char *iface);
-extern const char *mh_network_get_mac_address(const char *iface);
+/**
+ * An opaque type for a network interface
+ */
+struct mh_network_interface;
+
+/**
+ * Flags regarding the state of a network interface.
+ */
+enum mh_network_interface_flags {
+ MH_NETWORK_IF_UP = (1 << 0),
+ MH_NETWORK_IF_DOWN = (1 << 1),
+};
+
+/**
+ * Get the name of a network interface
+ *
+ * \param[in] iface the network interface
+ *
+ * \return the name for the provided network interface
+ */
+extern const char *
+mh_network_interface_get_name(const struct mh_network_interface *iface);
+
+/**
+ * Get flags for the state of a network interface
+ *
+ * \param[in] iface the network interface
+ *
+ * \return mh_network_interface_flags
+ */
+uint64_t
+mh_network_interface_get_flags(const struct mh_network_interface *iface);
+
+/**
+ * Destroy a network interface
+ *
+ * This destroys an allocated mh_network_interface. This function should
+ * be used in combination with g_list_free_full() on the rest from
+ * mh_network_get_interfaces(). For example:
+ *
+ * \code
+ * GList *iface_list;
+ *
+ * iface_list = mh_network_get_interfaces();
+ * ... do stuff with the iface_list ...
+ * g_list_free_full(iface_list, mh_network_interface_destroy);
+ * \endcode
+ */
+extern void
+mh_network_interface_destroy(gpointer data);
+
+/**
+ * Get a list of network interfaces.
+ *
+ * \note Don't forget to free this list when done with it! To do so, use
+ * g_list_free_full(iface_list, mh_network_interface_destroy).
+ *
+ * \return A list of network interfaces. The data for each list item is a
+ * struct mh_network_interface.
+ */
+extern GList *
+mh_network_get_interfaces(void);
+
+/**
+ * Get the IP address of a network interface
+ *
+ * \param[in] iface network interface name
+ * \param[out] buf buffer to hold the IP address
+ * \param[in] len length of buf
+ *
+ * \return buf, for convenience
+ */
+extern const char *
+mh_network_get_ip_address(const char *iface, char *buf, size_t len);
+
+/**
+ * Get the MAC address of a network interface
+ *
+ * \param[in] iface network interface name
+ * \param[out] buf buffer to hold the MAC address
+ * \param[in] len length of buf
+ *
+ * \return buf, for convenience
+ */
+extern const char *
+mh_network_get_mac_address(const char *iface, char *buf, size_t len);
extern void mh_network_stop(const char *iface);
extern void mh_network_start(const char *iface);
extern void mh_network_restart(const char *iface);
-extern void mh_network_status(const char *iface, uint64_t *flags);
+
+/**
+ * Get the status of a network interface
+ *
+ * \param[in] iface the network interface
+ * \param[out] flags a bit field of flags set that indicate the status of the
+ * interface. See mh_network_interface_flags for a list of possible
+ * flags.
+ *
+ * \retval 0 network interface was found
+ * \retval non-zero network interface was not found and the value of flags
+ * is undefined.
+ */
+extern int
+mh_network_status(const char *iface, uint64_t *flags);
#endif /* __NETWORK_H */
diff --git a/src/lib/network.c b/src/lib/network.c
index d502d34..9a1687b 100644
--- a/src/lib/network.c
+++ b/src/lib/network.c
@@ -29,6 +29,36 @@
MH_TRACE_INIT_DATA(mh_network);
+struct mh_network_interface {
+ sigar_net_interface_config_t ifconfig;
+};
+
+const char *
+mh_network_interface_get_name(const struct mh_network_interface *iface)
+{
+ return iface->ifconfig.name;
+}
+
+uint64_t
+mh_network_interface_get_flags(const struct mh_network_interface *iface)
+{
+ uint64_t flags = 0;
+
+ if (iface->ifconfig.flags & SIGAR_IFF_UP) {
+ flags = MH_NETWORK_IF_UP;
+ } else {
+ flags = MH_NETWORK_IF_DOWN;
+ }
+
+ return flags;
+}
+
+void
+mh_network_interface_destroy(gpointer data)
+{
+ g_free(data);
+}
+
static GList *
query_interface_table(void)
{
@@ -37,30 +67,42 @@ query_interface_table(void)
GList *interfaces = NULL;
sigar_t* sigar;
sigar_net_interface_list_t iflist;
- sigar_net_interface_config_t *ifconfig;
sigar_open(&sigar);
+
status = sigar_net_interface_list_get(sigar, &iflist);
- if (status == SIGAR_OK) {
- for (lpc = 0; lpc < iflist.number; lpc++) {
- ifconfig = g_new0(sigar_net_interface_config_t, 1);
- status = sigar_net_interface_config_get(sigar, iflist.data[lpc],
- ifconfig);
- if (status == SIGAR_OK)
- interfaces = g_list_prepend(interfaces, ifconfig);
+ if (status != SIGAR_OK) {
+ goto return_cleanup;
+ }
+
+ for (lpc = 0; lpc < iflist.number; lpc++) {
+ struct mh_network_interface *iface;
+
+ iface = g_malloc0(sizeof(*iface));
+
+ status = sigar_net_interface_config_get(sigar, iflist.data[lpc],
+ &iface->ifconfig);
+
+ if (status != SIGAR_OK) {
+ g_free(iface);
+ continue;
}
- sigar_net_interface_list_destroy(sigar, &iflist);
+
+ interfaces = g_list_prepend(interfaces, iface);
}
+
+ sigar_net_interface_list_destroy(sigar, &iflist);
+
+return_cleanup:
sigar_close(sigar);
+
return interfaces;
}
GList *
mh_network_get_interfaces(void)
{
- GList *list = NULL;
- list = query_interface_table();
- return list;
+ return query_interface_table();
}
void
@@ -82,64 +124,76 @@ mh_network_restart(const char *iface)
network_os_start(iface);
}
-void
+int
mh_network_status(const char *iface, uint64_t *flags)
{
GList *list = NULL;
GList *plist;
- sigar_net_interface_config_t *ifconfig;
+ int res = 1;
list = mh_network_get_interfaces();
for (plist = g_list_first(list); plist; plist = g_list_next(plist)) {
- ifconfig = (sigar_net_interface_config_t *)plist->data;
- if ((g_str_equal(ifconfig->name, iface)) == TRUE) {
- *flags = ifconfig->flags;
+ struct mh_network_interface *mh_iface = plist->data;
+
+ if ((g_str_equal(mh_network_interface_get_name(mh_iface), iface)) == TRUE) {
+ *flags = mh_network_interface_get_flags(mh_iface);
+ res = 0;
}
}
+ g_list_free_full(list, mh_network_interface_destroy);
+
+ return res;
}
const char *
-mh_network_get_ip_address(const char *iface)
+mh_network_get_ip_address(const char *iface, char *buf, size_t len)
{
GList *list = NULL;
GList *plist;
- sigar_net_interface_config_t *ifconfig;
char addr_str[SIGAR_INET6_ADDRSTRLEN];
- char *paddr_str;
list = mh_network_get_interfaces();
for (plist = g_list_first(list); plist; plist = g_list_next(plist)) {
- ifconfig = (sigar_net_interface_config_t *)plist->data;
- if ((g_str_equal(ifconfig->name, iface)) == TRUE) {
- sigar_net_address_to_string(NULL, &ifconfig->address, addr_str);
- paddr_str = g_strdup(addr_str);
- return paddr_str;
+ struct mh_network_interface *iface = plist->data;
+
+ if ((g_str_equal(mh_network_interface_get_name(iface), iface)) == TRUE) {
+ sigar_net_address_to_string(NULL, &iface->ifconfig.address,
addr_str);
+ strncpy(buf, addr_str, len - 1);
+ buf[len - 1] = '\0';
+ break;
}
}
- return NULL;
+ g_list_free_full(list, mh_network_interface_destroy);
+
+ return buf;
}
const char *
-mh_network_get_mac_address(const char *iface)
+mh_network_get_mac_address(const char *iface, char *buf, size_t len)
{
GList *list = NULL;
GList *plist;
- sigar_net_interface_config_t *ifconfig;
- char *mac;
+
+ if (len) {
+ *buf = '\0';
+ }
list = mh_network_get_interfaces();
for (plist = g_list_first(list); plist; plist = g_list_next(plist)) {
- ifconfig = (sigar_net_interface_config_t *)plist->data;
- if ((g_str_equal(ifconfig->name, iface)) == TRUE) {
- mac = g_strdup_printf("%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
- ifconfig->hwaddr.addr.mac[0],
- ifconfig->hwaddr.addr.mac[1],
- ifconfig->hwaddr.addr.mac[2],
- ifconfig->hwaddr.addr.mac[3],
- ifconfig->hwaddr.addr.mac[4],
- ifconfig->hwaddr.addr.mac[5]);
- return mac;
+ struct mh_network_interface *iface = plist->data;
+
+ if ((g_str_equal(mh_network_interface_get_name(iface), iface)) == TRUE) {
+ snprintf(buf, len, "%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
+ iface->ifconfig.hwaddr.addr.mac[0],
+ iface->ifconfig.hwaddr.addr.mac[1],
+ iface->ifconfig.hwaddr.addr.mac[2],
+ iface->ifconfig.hwaddr.addr.mac[3],
+ iface->ifconfig.hwaddr.addr.mac[4],
+ iface->ifconfig.hwaddr.addr.mac[5]);
+ break;
}
}
- return NULL;
+ g_list_free_full(list, mh_network_interface_destroy);
+
+ return buf;
}
diff --git a/src/network/network-dbus.c b/src/network/network-dbus.c
index 31040d2..c639cb3 100644
--- a/src/network/network-dbus.c
+++ b/src/network/network-dbus.c
@@ -23,8 +23,6 @@
#include <stdio.h>
#include <string.h>
-#include <sigar.h>
-
#include "matahari/mh_dbus_common.h"
/* Network methods */
@@ -47,10 +45,13 @@ static enum status
interface_status(const char *iface)
{
uint64_t flags = 0;
+
mh_network_status(iface, &flags);
- if (flags & SIGAR_IFF_UP)
+ if (flags & MH_NETWORK_IF_UP) {
return RUNNING;
+ }
+
return INACTIVE; /* Inactive */
}
@@ -62,7 +63,6 @@ Network_list(Matahari *matahari, DBusGMethodInvocation *context)
GError* error = NULL;
GList *interface_list;
GList *plist;
- sigar_net_interface_config_t *ifconfig;
char **list;
int i = 0;
@@ -81,14 +81,14 @@ Network_list(Matahari *matahari, DBusGMethodInvocation *context)
for (plist = g_list_first(interface_list);
plist;
plist = g_list_next(plist)) {
- ifconfig = (sigar_net_interface_config_t *)plist->data;
- list[i++] = strdup(ifconfig->name);
+ struct mh_network_interface *iface = plist->data;
+ list[i++] = strdup(mh_network_interface_get_name(iface));
}
list[i] = NULL; // Sentinel
dbus_g_method_return(context, list);
g_strfreev(list);
- g_list_free(interface_list);
+ g_list_free_full(interface_list, mh_network_interface_destroy);
return TRUE;
}
@@ -152,14 +152,18 @@ gboolean
Network_get_ip_address(Matahari *matahari, const char *iface,
DBusGMethodInvocation *context)
{
+ char buf[64];
GError* error = NULL;
+
if (!check_authorization(NETWORK_BUS_NAME ".get_ip_address", &error,
context)) {
dbus_g_method_return_error(context, error);
return FALSE;
}
- dbus_g_method_return(context, g_strdup(mh_network_get_ip_address(iface)));
+ dbus_g_method_return(context,
+ g_strdup(mh_network_get_ip_address(iface, buf, sizeof(buf))));
+
return TRUE;
}
@@ -168,13 +172,17 @@ Network_get_mac_address(Matahari *matahari, const char *iface,
DBusGMethodInvocation *context)
{
GError* error = NULL;
+ char buf[32];
+
if (!check_authorization(NETWORK_BUS_NAME ".get_mac_address", &error,
context)) {
dbus_g_method_return_error(context, error);
return FALSE;
}
- dbus_g_method_return(context, g_strdup(mh_network_get_mac_address(iface)));
+ dbus_g_method_return(context,
+ g_strdup(mh_network_get_mac_address(iface, buf, sizeof(buf))));
+
return TRUE;
}
diff --git a/src/network/network-qmf.cpp b/src/network/network-qmf.cpp
index 475589b..e8a1d05 100644
--- a/src/network/network-qmf.cpp
+++ b/src/network/network-qmf.cpp
@@ -34,8 +34,6 @@ extern "C" {
#include "matahari/logging.h"
#include "matahari/network.h"
#include "matahari/host.h"
-#include <sigar.h>
-#include <sigar_format.h>
};
class NetAgent : public MatahariAgent
@@ -63,14 +61,17 @@ static int
interface_status(const char *iface)
{
uint64_t flags = 0;
- if (iface == NULL)
+
+ if (iface == NULL) {
return 3;
+ }
mh_network_status(iface, &flags);
- if (flags & SIGAR_IFF_UP) {
+ if (flags & MH_NETWORK_IF_UP) {
return 0;
}
+
return 1; /* Inactive */
}
@@ -102,14 +103,15 @@ NetAgent::invoke(qmf::AgentSession session, qmf::AgentEvent event,
GList *interface_list = NULL;
_qtype::Variant::List s_list;
- sigar_net_interface_config_t *ifconfig = NULL;
interface_list = mh_network_get_interfaces();
for (plist = g_list_first(interface_list); plist;
plist = g_list_next(plist)) {
- ifconfig = (sigar_net_interface_config_t *)plist->data;
- s_list.push_back(ifconfig->name);
+ struct mh_network_interface *iface =
+ static_cast<struct mh_network_interface
*>(plist->data);
+ s_list.push_back(mh_network_interface_get_name(iface));
}
+ g_list_free_full(interface_list, mh_network_interface_destroy);
event.addReturnArgument("iface_map", s_list);
} else if (methodName == "start") {
int rc = interface_status(
@@ -134,11 +136,15 @@ NetAgent::invoke(qmf::AgentSession session, qmf::AgentEvent event,
event.addReturnArgument("status", interface_status(
event.getArguments()["iface"].asString().c_str()));
} else if (methodName == "get_ip_address") {
+ char buf[64];
event.addReturnArgument("ip", mh_network_get_ip_address(
- event.getArguments()["iface"].asString().c_str()));
+ event.getArguments()["iface"].asString().c_str(),
+ buf, sizeof(buf)));
} else if (methodName == "get_mac_address") {
+ char buf[32];
event.addReturnArgument("mac", mh_network_get_mac_address(
- event.getArguments()["iface"].asString().c_str()));
+ event.getArguments()["iface"].asString().c_str(),
+ buf, sizeof(buf)));
} else {
session.raiseException(event, MH_NOT_IMPLEMENTED);
goto bail;
--
1.7.5.4
_______________________________________________
Matahari mailing list
Matahari(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/matahari