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
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
