*** Bug 726171 has been marked as a duplicate of this bug. ***

The branch seems fine to me.

Pushed one fixup! + one additional small patch

> platform: add nm_platform_sysctl_get_int_checked() function

>+ * value. The returned value will always be in the range between @min and @max
>+ * (included) or @fallback.

"inclusive"



> core: add ASSERT_SAVE_PATH_COMPONENT function

>+const char *_ASSERT_SAVE_PATH_COMPONENT (const char *name)

linebreak after *

The "SAVE" in the name seems irrelevant/incorrect. "ASSERT_VALID_PATH_COMPONENT"?

>+	if (name)
>+		nm_log_err (LOGD_CORE, "Failed asserting path component: NULL");
>+	else
>+		nm_log_err (LOGD_CORE, "Failed asserting path component: \"%s\"", name);
>+	g_assert_not_reached ();
>+	g_return_val_if_reached ("XXXXX");
>+	return "XXXXX";

Using g_error() instead of nm_log_err()+g_assert_not_reached() might be more idiomatic, and also avoids the need for you to have to care about G_DISABLE_ASSERT, since it's G_GNUC_NORETURN no matter what. (Not that you should care about G_DISABLE_ASSERT anyway, but...)

>+const char *_ASSERT_SAVE_PATH_COMPONENT (const char *name);
>+#define ASSERT_SAVE_PATH_COMPONENT(name) (_ASSERT_SAVE_PATH_COMPONENT(name))

what is this indirection for?



> core: use ASSERT_SAVE_PATH_COMPONENT

>+	ifname = ASSERT_SAVE_PATH_COMPONENT (ifname);

Technically, this is unnecessary, because the kernel-enforced rules for interface names are stricter than what ASSERT_SAVE_PATH_COMPONENT checks (and if they weren't, then sysfs would become unusable, so we know this won't change).

If you want to keep a check anyway, I'd say to put it in NMDevice when priv->ifname gets set.

Note also that NMPlatformLinux's sysctl_get() already does:

	g_assert (!strstr (path, "/.."));

> core: use nm_platform_sysctl_get() to read sysctl in NMDeviceAdsl

>+		/* FIXME: unref during construction is wrong */
> 		g_object_unref (object);
> 		return NULL;

It doesn't crash any more. That was reverted. As for it giving a warning, so does returning NULL from constructor()...



> core: use nm_platform_sysctl_get() to read infiniband mode

>+	if (g_file_test (mode_path, G_FILE_TEST_EXISTS))
>+		contents = nm_platform_sysctl_get (mode_path);

You aren't supposed to have to do that. nm_platform_sysctl_get() only logs a debug message if it gets ENOENT, and even if you have PLATFORM:DEBUG turned on, there aren't going to be enough of those messages to be annoying. So just call it unconditionally.



> core: use nm_platform_sysctl_get() to read sysfs value

vague compared to the other commit messages. "to read wifi rfkill value"?

(In reply to comment #4)

Repushed branch.

New is commit:
>> core: refactor nm_utils_ascii_str_to_int64()
>> platform: relax assert when checking pathnames for accessing sysctl



> > core: add ASSERT_SAVE_PATH_COMPONENT function
> 
> >+const char *_ASSERT_SAVE_PATH_COMPONENT (const char *name)
> 
> linebreak after *
> 
> The "SAVE" in the name seems irrelevant/incorrect.
> "ASSERT_VALID_PATH_COMPONENT"?

Renamed. "save" as in "secure", but ASSERT_VALID_PATH_COMPONENT is a better name.


 
> >+	if (name)
> >+		nm_log_err (LOGD_CORE, "Failed asserting path component: NULL");
> >+	else
> >+		nm_log_err (LOGD_CORE, "Failed asserting path component: \"%s\"", name);
> >+	g_assert_not_reached ();
> >+	g_return_val_if_reached ("XXXXX");
> >+	return "XXXXX";
> 
> Using g_error() instead of nm_log_err()+g_assert_not_reached() might be more
> idiomatic, and also avoids the need for you to have to care about
> G_DISABLE_ASSERT, since it's G_GNUC_NORETURN no matter what. (Not that you
> should care about G_DISABLE_ASSERT anyway, but...)

I did not change this, because there should be a <error> log in nm-logging, and then it should assert both based on NDEBUG and G_DISABLE_ASSERT.



> >+const char *_ASSERT_SAVE_PATH_COMPONENT (const char *name);
> >+#define ASSERT_SAVE_PATH_COMPONENT(name) (_ASSERT_SAVE_PATH_COMPONENT(name))
> 
> what is this indirection for?

Removed. It was there to easily remove it with one #if directive. But we can always do that later (since it's NM internal API anyway).




> > core: use ASSERT_SAVE_PATH_COMPONENT
> 
> >+	ifname = ASSERT_SAVE_PATH_COMPONENT (ifname);
> 
> Technically, this is unnecessary, because the kernel-enforced rules for
> interface names are stricter than what ASSERT_SAVE_PATH_COMPONENT checks (and
> if they weren't, then sysfs would become unusable, so we know this won't
> change).

The ifname comes from platform link cache, which is not directly the same as what the kernel sends (it certainly should, but that is what the assert is there for).

Btw. against every assert one could argue, that the assert is useless because it ~cannot~ possibly fail. Which is however sometimes to hard to prove and the assert is also there to guard against future changes.



> If you want to keep a check anyway, I'd say to put it in NMDevice when
> priv->ifname gets set.

I had that before, but NMDevice is more then a simple class to model kernel interfaces. Sometimes the ifname has different meanings (e.g. NMDeviceBt). So, I could imagine that ifname is something different (which is fine, as long as we don't use those names to access sysctl paths).


> Note also that NMPlatformLinux's sysctl_get() already does:
> 
>     g_assert (!strstr (path, "/.."));

I saw that and considered removing it. I reconsidered it again.
See commit:
>> platform: relax assert when checking pathnames for accessing sysctl 



 
> > core: use nm_platform_sysctl_get() to read sysctl in NMDeviceAdsl
> 
> >+		/* FIXME: unref during construction is wrong */
> > 		g_object_unref (object);
> > 		return NULL;
> 
> It doesn't crash any more. That was reverted. As for it giving a warning, so
> does returning NULL from constructor()...

Sure, but it is still a bug which should be fixed one day (the glib warning indicates tells you that too).



> > core: use nm_platform_sysctl_get() to read infiniband mode
> 
> >+	if (g_file_test (mode_path, G_FILE_TEST_EXISTS))
> >+		contents = nm_platform_sysctl_get (mode_path);
> 
> You aren't supposed to have to do that. nm_platform_sysctl_get() only logs a
> debug message if it gets ENOENT, and even if you have PLATFORM:DEBUG turned on,
> there aren't going to be enough of those messages to be annoying. So just call
> it unconditionally.

Changed.




> > core: use nm_platform_sysctl_get() to read sysfs value
> 
> vague compared to the other commit messages. "to read wifi rfkill value"?

Changed.

(In reply to comment #5)
> > The "SAVE" in the name seems irrelevant/incorrect.
> > "ASSERT_VALID_PATH_COMPONENT"?
> 
> Renamed. "save" as in "secure", but ASSERT_VALID_PATH_COMPONENT is a better
> name.

Ohhh, you mean "safe", not "save". Makes sense now.

> > > core: use nm_platform_sysctl_get() to read sysctl in NMDeviceAdsl
> > 
> > >+		/* FIXME: unref during construction is wrong */
> > > 		g_object_unref (object);
> > > 		return NULL;
> > 
> > It doesn't crash any more. That was reverted. As for it giving a warning, so
> > does returning NULL from constructor()...
> 
> Sure, but it is still a bug which should be fixed one day (the glib warning
> indicates tells you that too).

But highlighting the unref-during-construction and NOT the "return NULL" suggests that this would be a fix:

    g_idle_add (g_object_unref, object);
    return NULL;

But it isn't.

(And actually, since this is all internal API, we can just fix it to use GInitable anyway, and filing a bug reminding us to do that would be better than leaving a FIXME. Which I've now done: bug 727164.)



> core: add ASSERT_VALID_PATH_COMPONENT function

>+const char *ASSERT_VALID_PATH_COMPONENT (const char *name);

add G_GNUC_WARN_UNUSED_RESULT to the end (before the ";"), so gcc will warn if you do

    ASSERT_VALID_PATH_COMPONENT (ifname);

rather than

    ifname = ASSERT_VALID_PATH_COMPONENT (ifname);



> core: refactor nm_utils_ascii_str_to_int64()

>+		if (len >= sizeof (buf))
>+			s = str_free = g_malloc (len + 1);

I initially started to point out that if you skipped over leading 0s yourself, then you could guarantee that "len >= sizeof (buf)" implied overflow. But that makes negative numbers and non-base-10 trickier, so it might not actually be a simplification.



> platform: relax assert when checking pathnames for accessing sysctl

The commit message implies that we shouldn't be checking for "/../" at all, but then all you do is change "/.." to "/../". Maybe just kill the first paragraph of the message?

(In reply to comment #6)
> (In reply to comment #5)
> > > The "SAVE" in the name seems irrelevant/incorrect.
> > > "ASSERT_VALID_PATH_COMPONENT"?
> > 
> > Renamed. "save" as in "secure", but ASSERT_VALID_PATH_COMPONENT is a better
> > name.
> 
> Ohhh, you mean "safe", not "save". Makes sense now.

Oh, right :$



> > > > core: use nm_platform_sysctl_get() to read sysctl in NMDeviceAdsl
> > > 
> > > >+		/* FIXME: unref during construction is wrong */
> > > > 		g_object_unref (object);
> > > > 		return NULL;
> > > 
> > > It doesn't crash any more. That was reverted. As for it giving a warning, so
> > > does returning NULL from constructor()...
> > 
> > Sure, but it is still a bug which should be fixed one day (the glib warning
> > indicates tells you that too).
> 
> But highlighting the unref-during-construction and NOT the "return NULL"
> suggests that this would be a fix:
> 
>     g_idle_add (g_object_unref, object);
>     return NULL;
> 
> But it isn't.
> 
> (And actually, since this is all internal API, we can just fix it to use
> GInitable anyway, and filing a bug reminding us to do that would be better than
> leaving a FIXME. Which I've now done: bug 727164.)

Ok, removed FIXME comment entirely.



"Done" to the rest, and repushed.

Changes are 3 new fixup! commits, and rewording of commit message for the "relax assert" commit.

> core: refactor nm_utils_ascii_str_to_int64()

if (str_free)
    g_free (str_free);

g_free() is NULL-safe, so I don't think we need the if().

The rest looks fine.

(In reply to comment #8)
> > core: refactor nm_utils_ascii_str_to_int64()
> 
> if (str_free)
>     g_free (str_free);
> 
> g_free() is NULL-safe, so I don't think we need the if().
> 
> The rest looks fine.

g_free() does:

void
g_free (gpointer mem)
{
  if (G_LIKELY (mem))
    glib_mem_vtable.free (mem);
  TRACE(GLIB_MEM_FREE((void*) mem));
} 



and this, for the very exotic case, that (str_free != NULL) -- because usually the buf[64] array is large enough. This being a "reusable library function", I think it is worth the extra line.

I will change it however to

- if (str_free)
+ if (G_UNLIKELY (str_free))

Ok, no problem.

This is not performance critical code. It is better to have it be clear and readable than to get an extra nanosecond of speed.

Merged to master as:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=7ef622b447a1759350353c578d65fed6b065f457

this branch is long merged. Closing.