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