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

[Jiri Pirko <jiri resnulli us>]

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.

Allright.

> 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

Allright.

> -- Dan