These are two patches that povides WPA wireless functionality both as command line options and in kickstart. They also change the way we communicate with the Network Manager when creating wireless connections (all WPA, WEP and unprotected) to use D-Bus.
--- loader/loader.c | 2 + loader/loader.h | 4 +- loader/net.c | 335 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- loader/net.h | 3 + 4 files changed, 342 insertions(+), 2 deletions(-)
diff --git a/loader/loader.c b/loader/loader.c index 2a5d21e..98dfda9 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1050,6 +1050,8 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData) { loaderData->mtu = argToLong(v); } else if (!strcasecmp(k, "wepkey")) { loaderData->wepkey = g_strdup(v); + } else if (!strcasecmp(k, "wpakey")) { + loaderData->wpakey = g_strdup(v); } else if (!strcasecmp(k, "linksleep")) { num_link_checks = argToLong(v); } else if (!strcasecmp(k, "nicdelay")) { diff --git a/loader/loader.h b/loader/loader.h index d761a4a..b81fc40 100644 --- a/loader/loader.h +++ b/loader/loader.h @@ -130,7 +130,9 @@ struct loaderData_s { int bootIf_set; char * netCls; int netCls_set; - char *ipv4, *netmask, *gateway, *dns, *domain, *hostname, *peerid, *ethtool, *subchannels, *portname, *essid, *wepkey, *nettype, *ctcprot, *options, *macaddr; + char *ipv4, *netmask, *gateway, *dns, *domain, *hostname, *peerid, *ethtool; + char *subchannels, *portname, *nettype, *ctcprot, *options, *macaddr; + char *essid, *wepkey, *wpakey; #ifdef ENABLE_IPV6 char *ipv6; char *ipv6prefix; diff --git a/loader/net.c b/loader/net.c index 21c8b1e..04e0cd6 100644 --- a/loader/net.c +++ b/loader/net.c @@ -54,6 +54,14 @@ #include "windows.h" #include "ibft.h"
+#include <nm-device.h> +#include <nm-setting-connection.h> +#include <nm-setting-wireless.h> +#include <nm-setting-ip4-config.h> +#include <nm-utils.h> +#include <dbus/dbus.h> +#include <dbus/dbus-glib.h> + /* boot flags */ extern uint64_t flags;
@@ -1869,6 +1877,8 @@ int chooseNetworkInterface(struct loaderData_s * loaderData) { * kickstart install so that we can do things like grab the ks.cfg from * the network */ int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) { + int rc=-1; + char *ip_method;
if ((is_nm_connected() == TRUE) && (loaderData->netDev != NULL) && (loaderData->netDev_set == 1)) @@ -1876,8 +1886,36 @@ int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
iface_init_iface_t(iface);
- return activateDevice(loaderData, iface); + if (loaderData->ipinfo_set == 1) { + ip_method = "manual"; + } + else { + ip_method = "auto"; + } + if (loaderData->essid != NULL) { + if (loaderData->wepkey != NULL) { + rc = add_and_activate_wifi_connection(&(loaderData->netDev), + loaderData->essid, "wep", loaderData->wepkey, + ip_method, loaderData->ipv4); + } + else if (loaderData->wpakey != NULL) { + rc = add_and_activate_wifi_connection(&(loaderData->netDev), + loaderData->essid, "wpa", loaderData->wpakey, + ip_method, loaderData->ipv4); + } + else { + rc = add_and_activate_wifi_connection(&(loaderData->netDev), + loaderData->essid, "unprotected", NULL, + ip_method, loaderData->ipv4); + } + if (0 == rc) { + loaderData->netDev_set = 1; + return(0); + } + else logMessage(ERROR, "wifi activation failed"); + }
+ return(activateDevice(loaderData, iface)); }
int disconnectDevice(char *device) { @@ -2229,4 +2267,299 @@ int isURLRemote(char *url) { } }
+gboolean byte_array_cmp(const GByteArray *array, char *string) +{ + //returns TRUE if array and char* contain the same strings + int i=0; + gboolean ret = TRUE; + if (array->len != strlen(string)) { + return FALSE; + } + while (i<array->len && ret) { + ret = ret && (array->data[i] == string[i]); + i++; + } + return ret; +} + +NMAccessPoint* get_best_ap(NMDeviceWifi *device, char *ssid) { + const GPtrArray *aps; + int i; + NMAccessPoint *candidate = NULL; + guint8 max = 0; + aps = nm_device_wifi_get_access_points(device); + + if (aps == NULL) + { + return NULL; + } + + for (i = 0; i < aps->len; i++) { + NMAccessPoint *ap = g_ptr_array_index(aps, i); + const GByteArray *byte_ssid = nm_access_point_get_ssid(ap); + if (byte_array_cmp(byte_ssid, ssid)) { + if (nm_access_point_get_strength(ap) > max) { + max = nm_access_point_get_strength(ap); + candidate = ap; + } + } + } + return candidate; +} + +gboolean get_device_and_ap(NMClient *client, char **in_out_iface, char *ssid, + NMDeviceWifi **out_device, NMAccessPoint **out_ap) +{ + //returns TRUE if device and ap (according to iface and ssid) + //were found + //in_out_iface, out_device, out_ap + const GPtrArray *devices; + int i; + + devices = nm_client_get_devices(client); + for (i = 0; i < devices->len; i++) + { + NMDevice *candidate = g_ptr_array_index(devices, i); + char *dev_iface = strdup((char *)nm_device_get_iface(candidate)); + if (strcmp(*in_out_iface, "")) + { + if (strcmp(dev_iface, *in_out_iface)) + { + continue; + } + } + if (NM_IS_DEVICE_WIFI(candidate)) + { + NMAccessPoint *ap = NULL; + ap = get_best_ap((NMDeviceWifi*)candidate, ssid); + if (ap != NULL) + { + *out_device = (NMDeviceWifi*)candidate; + *out_ap = ap; + *in_out_iface = strdup(dev_iface); + return TRUE; + } + } + } + return FALSE; +} + + +static void +add_cb (NMClient *client, + const char *connection_path, + const char *active_path, + GError *error, + gpointer user_data) +{ + if (error) + logMessage(ERROR, "Error adding wifi connection: %s", error->message); +} + +guint32 ip_str_to_nbo (char* ip) //get NBO representation of ip address +{ + struct in_addr tmp_addr = { 0 }; + + inet_pton (AF_INET, ip, &tmp_addr); + return tmp_addr.s_addr; +} + + +int add_and_activate_wifi_connection (char **iface, char *ssid, + char *protection, char *password, char *ip_method, char *address) + //return values mean: + //0 - everything ok + //1 - cannot open connection to D-Bus + //2 - cannot create NM Client + //3 - wifi disabled (hardware switch) + //4 - bad ssid format + //5 - no wifi device knowing about network with given ssid + //6 - activation failed (timed out) +{ + NMClient *client = NULL; + NMDeviceWifi *device = NULL; + NMAccessPoint *ap = NULL; + GMainLoop *loop; + GMainContext *ctx; + DBusGConnection *DBconnection; + GError *error; + gboolean success; + gint8 count=0, ret; + gboolean switched = FALSE; + + if (*iface == NULL) *iface = ""; + error = NULL; + DBconnection = dbus_g_bus_get (DBUS_BUS_SYSTEM, + &error); + if (DBconnection == NULL) + { + g_error_free (error); + return(1); + } + + client = nm_client_new(); + if (!client) { + return(2); + } + + if (!nm_client_wireless_hardware_get_enabled(client)) + { + return(3); + } + + if (!nm_client_wireless_get_enabled(client)) + { + nm_client_wireless_set_enabled(client, TRUE); + } + + GByteArray *ssid_ba; + int ssid_len; + + ssid_len = strlen (ssid); + if (!ssid || !ssid_len || (ssid_len > 32)) + { + return(4); + } + ssid_len = strlen (ssid); + ssid_ba = g_byte_array_sized_new (ssid_len); + g_byte_array_append (ssid_ba, (unsigned char *) ssid, ssid_len); + + loop = g_main_loop_new(NULL, FALSE); + ctx = g_main_loop_get_context(loop); + + while (g_main_context_pending(ctx)) + { + g_main_context_iteration(ctx, FALSE); + } + + /* display status */ + if (FL_CMDLINE(flags)) { + printf(_("Waiting for NetworkManager to activate wifi.\n")); + } else { + winStatus(55, 3, NULL, + _("Waiting for NetworkManager to activate wifi.\n"), 0); + } + + while (count < 45 && (!switched)) + { + while (g_main_context_pending(ctx)) + { + g_main_context_iteration(ctx, FALSE); + } + success = get_device_and_ap(client, iface, ssid, &device, &ap); + if (success) + { + switched = TRUE; + } + sleep(1); + count++; + } + + newtPopWindow(); + + if (!success) + { + return(5); + } + + NMConnection *connection; + NMSettingConnection *s_con; + NMSettingWireless *s_wireless; + NMSettingWirelessSecurity *s_sec; + NMSettingIP4Config *s_ip; + char *uuid; + + connection = nm_connection_new(); + + s_con = (NMSettingConnection*) nm_setting_connection_new(); + uuid = nm_utils_uuid_generate(); + g_object_set (G_OBJECT (s_con), + NM_SETTING_CONNECTION_UUID, uuid, + NM_SETTING_CONNECTION_ID, ssid, + NM_SETTING_CONNECTION_TYPE, "802-11-wireless", + NULL); + g_free (uuid); + nm_connection_add_setting (connection, NM_SETTING (s_con)); + + s_wireless = (NMSettingWireless*) nm_setting_wireless_new(); + g_object_set (G_OBJECT (s_wireless), + NM_SETTING_WIRELESS_SSID, ssid_ba, + NM_SETTING_WIRELESS_MODE, "infrastructure", + NULL); + if (!strcmp(protection, "wep") || (!strcmp(protection,"wpa"))) + { + g_object_set (G_OBJECT (s_wireless), + NM_SETTING_WIRELESS_SEC, "802-11-wireless-security", + NULL); + } + nm_connection_add_setting (connection, NM_SETTING (s_wireless)); + + if (!strcmp(protection, "wep")) + { + s_sec = (NMSettingWirelessSecurity*) nm_setting_wireless_security_new(); + g_object_set (G_OBJECT (s_sec), + NM_SETTING_WIRELESS_SECURITY_KEY_MGMT, "none", + NM_SETTING_WIRELESS_SECURITY_WEP_TX_KEYIDX, 0, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, 1, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY0, password, + NULL); + if (strlen(password) == 32) + { + g_object_set (G_OBJECT (s_sec), + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, 2, + NULL); + } + nm_connection_add_setting (connection, NM_SETTING (s_sec)); + } + else if (!strcmp(protection, "wpa")) + { + s_sec = (NMSettingWirelessSecurity*) nm_setting_wireless_security_new(); + g_object_set (G_OBJECT (s_sec), + NM_SETTING_WIRELESS_SECURITY_KEY_MGMT, "wpa-psk", + NM_SETTING_WIRELESS_SECURITY_PSK, password, + NULL); + nm_connection_add_setting (connection, NM_SETTING (s_sec)); + } + + if (!strcmp(ip_method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL)) + { + GPtrArray *addresses = g_ptr_array_new(); + GArray *address_array = g_array_new(FALSE, FALSE, sizeof(guint32)); + guint32 nbo_ip = ip_str_to_nbo(address); + guint32 mask = 24; + guint32 gw = 0; + + g_array_append_val(address_array, nbo_ip); + g_array_append_val(address_array, mask); + g_array_append_val(address_array, gw); + + g_ptr_array_add(addresses, address_array); + + s_ip = (NMSettingIP4Config*) nm_setting_ip4_config_new(); + g_object_set (G_OBJECT (s_ip), + NM_SETTING_IP4_CONFIG_METHOD, "manual", + NM_SETTING_IP4_CONFIG_ADDRESSES, addresses, + NULL); + nm_connection_add_setting(connection, NM_SETTING (s_ip)); + } + + const char *ap_path = nm_object_get_path((NMObject*) ap); + nm_client_add_and_activate_connection(client, connection, + (NMDevice*) device, ap_path, (NMClientAddActivateFn) add_cb, + NULL); + + ret = wait_for_iface_activation(*iface); + newtPopWindow(); + if (ret == 0) + { + g_main_loop_unref(loop); + return(0); + } + + *iface = NULL; + g_main_loop_unref(loop); + return(6); + +} + /* vim:set shiftwidth=4 softtabstop=4: */ diff --git a/loader/net.h b/loader/net.h index 2a74cdf..90a08b6 100644 --- a/loader/net.h +++ b/loader/net.h @@ -82,6 +82,9 @@ int wait_for_iface_activation(char * ifname); int wait_for_iface_disconnection(char *ifname); int isURLRemote(char *url); int isValidIPv4Address(const char *address); +int add_and_activate_wifi_connection (char **iface, char *ssid, + char *protection, char *password, + char *ip_method, char *address); #ifdef ENABLE_IPV6 int isValidIPv6Address(const char *address); #endif
On 05/18/2011 07:27 AM, Vratislav Podzimek wrote:
- GByteArray *ssid_ba;
- int ssid_len;
- ssid_len = strlen (ssid);
- if (!ssid || !ssid_len || (ssid_len> 32))
- {
return(4);
- }
- ssid_len = strlen (ssid);
- ssid_ba = g_byte_array_sized_new (ssid_len);
What on earth? Why are you taking strlen(ssid) twice? Why are you checking if it's NULL *after* calling strlen() on it? That's a segfault. Why isn't there an enum for these return codes? They should be symbolic. What happens if ssid_len is 31?
No.
On 05/18/2011 07:27 AM, Vratislav Podzimek wrote:
loader/loader.c | 2 + loader/loader.h | 4 +- loader/net.c | 335 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- loader/net.h | 3 + 4 files changed, 342 insertions(+), 2 deletions(-)
diff --git a/loader/loader.c b/loader/loader.c index 2a5d21e..98dfda9 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1050,6 +1050,8 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData) { loaderData->mtu = argToLong(v); } else if (!strcasecmp(k, "wepkey")) { loaderData->wepkey = g_strdup(v);
} else if (!strcasecmp(k, "wpakey")) {
loaderData->wpakey = g_strdup(v); } else if (!strcasecmp(k, "linksleep")) { num_link_checks = argToLong(v); } else if (!strcasecmp(k, "nicdelay")) {
diff --git a/loader/loader.h b/loader/loader.h index d761a4a..b81fc40 100644 --- a/loader/loader.h +++ b/loader/loader.h @@ -130,7 +130,9 @@ struct loaderData_s { int bootIf_set; char * netCls; int netCls_set;
- char *ipv4, *netmask, *gateway, *dns, *domain, *hostname, *peerid, *ethtool, *subchannels, *portname, *essid, *wepkey, *nettype, *ctcprot, *options, *macaddr;
- char *ipv4, *netmask, *gateway, *dns, *domain, *hostname, *peerid, *ethtool;
- char *subchannels, *portname, *nettype, *ctcprot, *options, *macaddr;
- char *essid, *wepkey, *wpakey; #ifdef ENABLE_IPV6 char *ipv6; char *ipv6prefix;
diff --git a/loader/net.c b/loader/net.c index 21c8b1e..04e0cd6 100644 --- a/loader/net.c +++ b/loader/net.c @@ -54,6 +54,14 @@ #include "windows.h" #include "ibft.h"
+#include<nm-device.h> +#include<nm-setting-connection.h> +#include<nm-setting-wireless.h> +#include<nm-setting-ip4-config.h> +#include<nm-utils.h> +#include<dbus/dbus.h> +#include<dbus/dbus-glib.h>
- /* boot flags */ extern uint64_t flags;
@@ -1869,6 +1877,8 @@ int chooseNetworkInterface(struct loaderData_s * loaderData) {
- kickstart install so that we can do things like grab the ks.cfg from
- the network */
int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
int rc=-1;
char *ip_method;
if ((is_nm_connected() == TRUE)&& (loaderData->netDev != NULL)&& (loaderData->netDev_set == 1))
@@ -1876,8 +1886,36 @@ int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
iface_init_iface_t(iface);
- return activateDevice(loaderData, iface);
- if (loaderData->ipinfo_set == 1) {
ip_method = "manual";
- }
- else {
ip_method = "auto";
- }
- if (loaderData->essid != NULL) {
if (loaderData->wepkey != NULL) {
rc = add_and_activate_wifi_connection(&(loaderData->netDev),
loaderData->essid, "wep", loaderData->wepkey,
ip_method, loaderData->ipv4);
}
else if (loaderData->wpakey != NULL) {
rc = add_and_activate_wifi_connection(&(loaderData->netDev),
loaderData->essid, "wpa", loaderData->wpakey,
ip_method, loaderData->ipv4);
}
else {
rc = add_and_activate_wifi_connection(&(loaderData->netDev),
loaderData->essid, "unprotected", NULL,
ip_method, loaderData->ipv4);
}
This style doesn't match our normal coding style.
if (0 == rc) {
Nor does this.
loaderData->netDev_set = 1;
return(0);
}
else logMessage(ERROR, "wifi activation failed");
}
return(activateDevice(loaderData, iface));
return is not a function.
}
int disconnectDevice(char *device) { @@ -2229,4 +2267,299 @@ int isURLRemote(char *url) { } }
+gboolean byte_array_cmp(const GByteArray *array, char *string) +{
- //returns TRUE if array and char* contain the same strings
- int i=0;
- gboolean ret = TRUE;
- if (array->len != strlen(string)) {
return FALSE;
- }
- while (i<array->len&& ret) {
ret = ret&& (array->data[i] == string[i]);
i++;
- }
- return ret;
+}
Why isn't this just memcmp()?
+NMAccessPoint* get_best_ap(NMDeviceWifi *device, char *ssid) {
- const GPtrArray *aps;
- int i;
- NMAccessPoint *candidate = NULL;
- guint8 max = 0;
- aps = nm_device_wifi_get_access_points(device);
- if (aps == NULL)
- {
return NULL;
- }
Style again.
- for (i = 0; i< aps->len; i++) {
NMAccessPoint *ap = g_ptr_array_index(aps, i);
const GByteArray *byte_ssid = nm_access_point_get_ssid(ap);
if (byte_array_cmp(byte_ssid, ssid)) {
if (nm_access_point_get_strength(ap)> max) {
max = nm_access_point_get_strength(ap);
candidate = ap;
}
}
- }
- return candidate;
+}
+gboolean get_device_and_ap(NMClient *client, char **in_out_iface, char *ssid,
NMDeviceWifi **out_device, NMAccessPoint **out_ap)
Please don't name arguments this way.
+{
- //returns TRUE if device and ap (according to iface and ssid)
- //were found
- //in_out_iface, out_device, out_ap
- const GPtrArray *devices;
- int i;
- devices = nm_client_get_devices(client);
- for (i = 0; i< devices->len; i++)
- {
NMDevice *candidate = g_ptr_array_index(devices, i);
char *dev_iface = strdup((char *)nm_device_get_iface(candidate));
This needs to be checked for failure.
if (strcmp(*in_out_iface, ""))
{
if (strcmp(dev_iface, *in_out_iface))
{
continue;
}
}
if (NM_IS_DEVICE_WIFI(candidate))
{
NMAccessPoint *ap = NULL;
ap = get_best_ap((NMDeviceWifi*)candidate, ssid);
if (ap != NULL)
{
*out_device = (NMDeviceWifi*)candidate;
*out_ap = ap;
*in_out_iface = strdup(dev_iface);
return TRUE;
}
}
- }
- return FALSE;
+}
+static void +add_cb (NMClient *client,
const char *connection_path,
const char *active_path,
GError *error,
gpointer user_data)
+{
- if (error)
logMessage(ERROR, "Error adding wifi connection: %s", error->message);
The style issues continue like this all the way down.
Thanks for working on this. Is there a bug number this addresses?
A couple general style points:
* I don't really like declaring variables in the middle of code.
* In expressions like:
if (a == 1 || (b == 2))
Please leave out the unnecessary parens in the second clause. I see several places you're doing that now. I know we do it in anaconda from time to time, and I don't like it there either. They've just not been changed because we haven't needed to work on that code yet.
* Please stick to the indentation and brace layout style of the file you're working in. I know we are pretty inconsistent with that too.
* In general, anaconda puts the brace on the same line as the conditional and omits the braces entirely if there's only one expression that would be in the block.
@@ -1869,6 +1877,8 @@ int chooseNetworkInterface(struct loaderData_s * loaderData) {
- kickstart install so that we can do things like grab the ks.cfg from
- the network */
int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
int rc=-1;
char *ip_method;
if ((is_nm_connected() == TRUE) && (loaderData->netDev != NULL) && (loaderData->netDev_set == 1))
ip_method would make more sense as a set of #defines so we don't have to call a function on every comparison.
+gboolean get_device_and_ap(NMClient *client, char **in_out_iface, char *ssid,
NMDeviceWifi **out_device, NMAccessPoint **out_ap)
+{
- //returns TRUE if device and ap (according to iface and ssid)
- //were found
- //in_out_iface, out_device, out_ap
- const GPtrArray *devices;
- int i;
- devices = nm_client_get_devices(client);
- for (i = 0; i < devices->len; i++)
- {
NMDevice *candidate = g_ptr_array_index(devices, i);
char *dev_iface = strdup((char *)nm_device_get_iface(candidate));
if (strcmp(*in_out_iface, ""))
{
if (strcmp(dev_iface, *in_out_iface))
{
continue;
}
}
if (NM_IS_DEVICE_WIFI(candidate))
{
NMAccessPoint *ap = NULL;
ap = get_best_ap((NMDeviceWifi*)candidate, ssid);
if (ap != NULL)
{
*out_device = (NMDeviceWifi*)candidate;
*out_ap = ap;
*in_out_iface = strdup(dev_iface);
You don't need to strdup dev_iface here, since you've already strduped it from the result of nm_device_get_iface earlier.
return TRUE;
}
}
There should be an else here to free(dev_iface) too.
- }
- return FALSE;
+}
+int add_and_activate_wifi_connection (char **iface, char *ssid,
- char *protection, char *password, char *ip_method, char *address)
protection should also be a set of defines for ease of comparison too.
//return values mean:
//0 - everything ok
//1 - cannot open connection to D-Bus
//2 - cannot create NM Client
//3 - wifi disabled (hardware switch)
//4 - bad ssid format
//5 - no wifi device knowing about network with given ssid
//6 - activation failed (timed out)
Making these defines as well would allow for more self-documenting code.
+{
- NMClient *client = NULL;
- NMDeviceWifi *device = NULL;
- NMAccessPoint *ap = NULL;
- GMainLoop *loop;
- GMainContext *ctx;
- DBusGConnection *DBconnection;
- GError *error;
- gboolean success;
- gint8 count=0, ret;
- gboolean switched = FALSE;
Can any of client, device, ap, loop, ctx, or DBconnection be freed before the end of the function?
- GByteArray *ssid_ba;
- int ssid_len;
- ssid_len = strlen (ssid);
- if (!ssid || !ssid_len || (ssid_len > 32))
- {
return(4);
- }
- ssid_len = strlen (ssid);
- ssid_ba = g_byte_array_sized_new (ssid_len);
- g_byte_array_append (ssid_ba, (unsigned char *) ssid, ssid_len);
pjones already responded here, but I'll second it. I don't understand the ordering here.
- loop = g_main_loop_new(NULL, FALSE);
- ctx = g_main_loop_get_context(loop);
- while (g_main_context_pending(ctx))
- {
g_main_context_iteration(ctx, FALSE);
- }
- /* display status */
- if (FL_CMDLINE(flags)) {
printf(_("Waiting for NetworkManager to activate wifi.\n"));
- } else {
winStatus(55, 3, NULL,
_("Waiting for NetworkManager to activate wifi.\n"), 0);
- }
- while (count < 45 && (!switched))
- {
while (g_main_context_pending(ctx))
{
g_main_context_iteration(ctx, FALSE);
}
success = get_device_and_ap(client, iface, ssid, &device, &ap);
if (success)
{
switched = TRUE;
}
sleep(1);
count++;
- }
You can nuke switched here and just break out at the "if success" point.
- newtPopWindow();
This needs to be protected with an if (!FL_CMDLINE(flags)).
- NMConnection *connection;
- NMSettingConnection *s_con;
- NMSettingWireless *s_wireless;
- NMSettingWirelessSecurity *s_sec;
- NMSettingIP4Config *s_ip;
- char *uuid;
Can any of connection, s_con, s_wireless, s_sec, or s_ip be freed before the end of the function?
- if (!strcmp(ip_method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL))
- {
GPtrArray *addresses = g_ptr_array_new();
GArray *address_array = g_array_new(FALSE, FALSE, sizeof(guint32));
guint32 nbo_ip = ip_str_to_nbo(address);
guint32 mask = 24;
guint32 gw = 0;
Again with the freeing question for addresses and address_array.
- Chris
On 05/18/2011 05:34 PM, Chris Lumens wrote:
Thanks for working on this. Is there a bug number this addresses?
There is a feature http://fedoraproject.org/wiki/Anaconda/Features/WirelessSupport
Adding WPA is a request that came up at FudCON in Tempe.
Radek
Hi guys, first of all, thanks for these pieces of advice I'd like to learn writing code the way it should be written. I fixed my patches according to your suggestions (with some exceptions - see comments below).
On Wed, 2011-05-18 at 11:34 -0400, Chris Lumens wrote:
Thanks for working on this. Is there a bug number this addresses?
A couple general style points:
I don't really like declaring variables in the middle of code.
In expressions like:
if (a == 1 || (b == 2))
Please leave out the unnecessary parens in the second clause. I see several places you're doing that now. I know we do it in anaconda from time to time, and I don't like it there either. They've just not been changed because we haven't needed to work on that code yet.
Please stick to the indentation and brace layout style of the file you're working in. I know we are pretty inconsistent with that too.
no problem, but you are right about inconsistency
- In general, anaconda puts the brace on the same line as the conditional and omits the braces entirely if there's only one expression that would be in the block.
@@ -1869,6 +1877,8 @@ int chooseNetworkInterface(struct loaderData_s * loaderData) {
- kickstart install so that we can do things like grab the ks.cfg from
- the network */
int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
int rc=-1;
char *ip_method;
if ((is_nm_connected() == TRUE) && (loaderData->netDev != NULL) && (loaderData->netDev_set == 1))
ip_method would make more sense as a set of #defines so we don't have to call a function on every comparison.
changed to ip_method_manual flag (this way we can use loaderData->ipinfo_set value)
+gboolean get_device_and_ap(NMClient *client, char **in_out_iface, char *ssid,
NMDeviceWifi **out_device, NMAccessPoint **out_ap)
+{
- //returns TRUE if device and ap (according to iface and ssid)
- //were found
- //in_out_iface, out_device, out_ap
- const GPtrArray *devices;
- int i;
- devices = nm_client_get_devices(client);
- for (i = 0; i < devices->len; i++)
- {
NMDevice *candidate = g_ptr_array_index(devices, i);
char *dev_iface = strdup((char *)nm_device_get_iface(candidate));
if (strcmp(*in_out_iface, ""))
{
if (strcmp(dev_iface, *in_out_iface))
{
continue;
}
}
if (NM_IS_DEVICE_WIFI(candidate))
{
NMAccessPoint *ap = NULL;
ap = get_best_ap((NMDeviceWifi*)candidate, ssid);
if (ap != NULL)
{
*out_device = (NMDeviceWifi*)candidate;
*out_ap = ap;
*in_out_iface = strdup(dev_iface);
You don't need to strdup dev_iface here, since you've already strduped it from the result of nm_device_get_iface earlier.
fixed
return TRUE;
}
}
There should be an else here to free(dev_iface) too.
fixed
- }
- return FALSE;
+}
+int add_and_activate_wifi_connection (char **iface, char *ssid,
- char *protection, char *password, char *ip_method, char *address)
protection should also be a set of defines for ease of comparison too.
changed, set of defines in net.h starting with WIFI_PROTECTION_
//return values mean:
//0 - everything ok
//1 - cannot open connection to D-Bus
//2 - cannot create NM Client
//3 - wifi disabled (hardware switch)
//4 - bad ssid format
//5 - no wifi device knowing about network with given ssid
//6 - activation failed (timed out)
Making these defines as well would allow for more self-documenting code.
changed, set of defines in net.h starting with WIFI_ACTIVATION_
+{
- NMClient *client = NULL;
- NMDeviceWifi *device = NULL;
- NMAccessPoint *ap = NULL;
- GMainLoop *loop;
- GMainContext *ctx;
- DBusGConnection *DBconnection;
- GError *error;
- gboolean success;
- gint8 count=0, ret;
- gboolean switched = FALSE;
Can any of client, device, ap, loop, ctx, or DBconnection be freed before the end of the function?
client, device and ap are used at the end of the function the others also cannnot be freed I've tried it and it made problems (SIGABRTs)
- GByteArray *ssid_ba;
- int ssid_len;
- ssid_len = strlen (ssid);
- if (!ssid || !ssid_len || (ssid_len > 32))
- {
return(4);
- }
- ssid_len = strlen (ssid);
- ssid_ba = g_byte_array_sized_new (ssid_len);
- g_byte_array_append (ssid_ba, (unsigned char *) ssid, ssid_len);
pjones already responded here, but I'll second it. I don't understand the ordering here.
bad result of rewriting old code, fixed
- loop = g_main_loop_new(NULL, FALSE);
- ctx = g_main_loop_get_context(loop);
- while (g_main_context_pending(ctx))
- {
g_main_context_iteration(ctx, FALSE);
- }
- /* display status */
- if (FL_CMDLINE(flags)) {
printf(_("Waiting for NetworkManager to activate wifi.\n"));
- } else {
winStatus(55, 3, NULL,
_("Waiting for NetworkManager to activate wifi.\n"), 0);
- }
- while (count < 45 && (!switched))
- {
while (g_main_context_pending(ctx))
{
g_main_context_iteration(ctx, FALSE);
}
success = get_device_and_ap(client, iface, ssid, &device, &ap);
if (success)
{
switched = TRUE;
}
sleep(1);
count++;
- }
You can nuke switched here and just break out at the "if success" point.
good point, thanks
- newtPopWindow();
This needs to be protected with an if (!FL_CMDLINE(flags)).
fixed
- NMConnection *connection;
- NMSettingConnection *s_con;
- NMSettingWireless *s_wireless;
- NMSettingWirelessSecurity *s_sec;
- NMSettingIP4Config *s_ip;
- char *uuid;
Can any of connection, s_con, s_wireless, s_sec, or s_ip be freed before the end of the function?
no, they cannot be freed
- if (!strcmp(ip_method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL))
- {
GPtrArray *addresses = g_ptr_array_new();
GArray *address_array = g_array_new(FALSE, FALSE, sizeof(guint32));
guint32 nbo_ip = ip_str_to_nbo(address);
guint32 mask = 24;
guint32 gw = 0;
Again with the freeing question for addresses and address_array.
freed
- Chris
Anaconda-devel-list mailing list Anaconda-devel-list@redhat.com https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--- loader/kickstart.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 62 insertions(+), 3 deletions(-)
diff --git a/loader/kickstart.c b/loader/kickstart.c index ff5f6c4..379b849 100644 --- a/loader/kickstart.c +++ b/loader/kickstart.c @@ -647,10 +647,44 @@ cleanup: Py_XDECREF(attr); }
+int processKickstartWifi (struct loaderData_s * loaderData) { + char *ip_method = ""; + int rc = -1; + + if (loaderData->ipinfo_set == 1) { + ip_method = "manual"; + } + else { + ip_method = "auto"; + } + + if (loaderData->essid != NULL) { + if (loaderData->wepkey != NULL) { + rc = add_and_activate_wifi_connection(&(loaderData->netDev), loaderData->essid, + "wep", loaderData->wepkey, ip_method, loaderData->ipv4); + } + else if (loaderData->wpakey != NULL) { + rc = add_and_activate_wifi_connection(&(loaderData->netDev), loaderData->essid, + "wpa", loaderData->wpakey, ip_method, loaderData->ipv4); + } + else { + rc = add_and_activate_wifi_connection(&(loaderData->netDev), loaderData->essid, + "unprotected", NULL, ip_method, loaderData->ipv4); + } + } + + if (0 == rc) loaderData->netDev_set = 1; + else logMessage(ERROR, "wifi activation in kickstart failed"); + return(rc); +} + + static void setKickstartNetwork(struct loaderData_s * loaderData, PyObject *handler) { Py_ssize_t i; PyObject *list = getDataList(handler, "network"); iface_t iface; + gboolean device_flushed = FALSE; + char *cmdline_device = NULL;
if (!list) return; @@ -686,6 +720,8 @@ static void setKickstartNetwork(struct loaderData_s * loaderData, PyObject *hand loaderData->essid = NULL; free(loaderData->wepkey); loaderData->wepkey = NULL; + free(loaderData->wpakey); + loaderData->wpakey = NULL; loaderData->mtu = 0;
#ifdef ENABLE_IPV6 @@ -749,6 +785,11 @@ static void setKickstartNetwork(struct loaderData_s * loaderData, PyObject *hand loaderData->netDev_set = 1; logMessage(INFO, "kickstart network command - device %s", loaderData->netDev); } else { + cmdline_device = strdup(loaderData->netDev); + loaderData->netDev_set = 0; + free(loaderData->netDev); + loaderData->netDev = NULL; + device_flushed = TRUE; logMessage(INFO, "kickstart network command - unspecified device"); }
@@ -758,6 +799,7 @@ static void setKickstartNetwork(struct loaderData_s * loaderData, PyObject *hand _setNetworkString(ele, "ethtool", &loaderData->ethtool, NULL); _setNetworkString(ele, "essid", &loaderData->essid, NULL); _setNetworkString(ele, "wepkey", &loaderData->wepkey, NULL); + _setNetworkString(ele, "wpakey", &loaderData->wpakey, NULL);
attr = getObject(ele, "noipv4", 0); if (isTrue(attr)) @@ -810,15 +852,32 @@ static void setKickstartNetwork(struct loaderData_s * loaderData, PyObject *hand FL_EARLY_NETWORKING(flags) || ibft_present())) { logMessage(INFO, "activating first device from kickstart because network is needed"); - activateDevice(loaderData, &iface); + if (processKickstartWifi(loaderData) != 0) { + if (device_flushed) { + loaderData->netDev = strdup(cmdline_device); + loaderData->netDev_set = 1; + free(cmdline_device); + cmdline_device = NULL; + device_flushed = FALSE; + } + activateDevice(loaderData, &iface); + } continue; }
- attr = getObject(ele, "activate", 0); if (isTrue(attr)) { logMessage(INFO, "activating because --activate flag is set"); - activateDevice(loaderData, &iface); + if (processKickstartWifi(loaderData) != 0) { + if (device_flushed) { + loaderData->netDev = strdup(cmdline_device); + loaderData->netDev_set = 1; + free(cmdline_device); + cmdline_device = NULL; + device_flushed = FALSE; + } + activateDevice(loaderData, &iface); + } } else { logMessage(INFO, "not activating becuase --activate flag is not set"); }
On 05/18/2011 01:27 PM, Vratislav Podzimek wrote:
These are two patches that povides WPA wireless functionality both as command line options and in kickstart. They also change the way we communicate with the Network Manager when creating wireless connections (all WPA, WEP and unprotected) to use D-Bus.
The integration into loader network configuration paths looks reasonable to me (given their poor state). Some things apart from what Chris and Peter remarked:
1) Is it possible to let NM generate UUID for the new connection by not setting the value? We don't generate connection UUID anywhere in acaconda and I'd like to keep this commitment.
+ + connection = nm_connection_new(); + + s_con = (NMSettingConnection*) nm_setting_connection_new(); + uuid = nm_utils_uuid_generate(); + g_object_set (G_OBJECT (s_con), + NM_SETTING_CONNECTION_UUID, uuid,
... just by leaving out this setting
+ NM_SETTING_CONNECTION_ID, ssid, + NM_SETTING_CONNECTION_TYPE, "802-11-wireless", + NULL); + g_free (uuid); + nm_connection_add_setting (connection, NM_SETTING (s_con));
2) We should remove our handling of keyfiles from loader (creating in writeEnabledNetInfo), and unify behaviour for wep and wpa in anaconda (stage2), e.g. copy also wpa keyfiles to target system - instead of writeWepkeyFile, we would probably just copy keyfiles we find. Also in kickstart stage2 processing, we should either stop writing out wep key file (this would happen in the case when the device is not activated in loader), or write out also wpa keys. I'm for the former.
3) Should we mention something about the behaviour of options in documentation? Or perhaps document it at least internally. For example behaviour regarding device specified/unspecified vs. ssid option present.
Radek
anaconda-devel@lists.stg.fedoraproject.org