Re: [patch NetworkManager] ifcfg-rh: reader: allow device to do not have ipv4 setting.

Wed, Aug 21, 2013 at 06:08:19PM CEST, danw gnome org wrote:
You should add a test case to test-ifcfg-rh for this, to make sure we
don't break it later.

Will try.

Other than that, just nitpicks:

Drop the word "do" in the commit summary line.

+static gchar *get_numbered_tag (gchar *tag_name, gint32 which)

Sure I will split this.

Should be split onto two lines. Also, we don't generally use "gchar"
(just use "char"), and there's no particular reason to use gint32 rather
than int here. (I see that there are some existing gint32s in that
file... I have no idea why.)

With this I always try to be conform with the rest of the code.
Since tag_name is put as a parameter to g_strdup, which has gchar * in
its prototype, I'm using gchar.
That is similar with "gint32 which". read_full_ip4_address() has it as
gint32 as its parameter so I use it as well.

The questiion really is why all over the NM code this is mixed up.
Either g- of non-g- variant should be used (I beliveve that g-)

+static gboolean is_any_ip4_address_defined (shvarFile *ifcfg)

likewise, split onto two lines.


Hm... so this function does the same thing as the code it replaced, but
I'm not sure why the old code did it that way... if there's just a
PREFIX, but no IPADDR, then that shouldn't count as "an IP4 address is
defined". Maybe dcbw will have some insight on this when he gets back.

I'm just following the original code. If you want to change the
behaviour, I suggest to do it in separate patch.

+    if (!value || bootproto_none) {
             /* If there is no BOOTPROTO, no IPADDR, no PREFIX, no NETMASK, but

The comment there should be updated to mention BOOTPROTO=none


-- Dan

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]