> core: add logging macro _LOG() and _LOGD() to nm-device.c

I don't think we need the str_if_set() stuff in __LOG(), since the device should *always* have an 'iface'.  If it doesn't, something is wrong.

But more than that, I'm a fan of using explicit arguments for stuff, since it's clearer what's going on, so I'd actually prefer:

#define __LOG(device, level, domain, ...) \
	G_STMT_START { \
		if (LOGL_##level <= LOGL_DEBUG) \
			NM_LOG(level, domain, device, "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), device, _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
		else \
			nm_log_##level (LOGD_##domain, "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), device, _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
	} G_STMT_END
#define _LOG(device, level, domain, ...)        __LOG(device, level, domain, __VA_ARGS__)
#define _LOGD(device, ...)                      __LOG(device, debug, DEVICE, __VA_ARGS__)

...

_LOG (self, warn, HW, "failed to look up interface index");

that's also more flexible since we can determine which NMDevice to use, especially in the case of master/slave stuff.

But really, in the case of warn/err/info, why should that get passed as an argument?  What's wrong with _LOG_INFO, _LOG_WARN, _LOG_ERR?  I think that's more explicit than passing the level as an argument, and most of the time what's important with a log message is the level it's printed at.

Other than that, the rest of the changes look fine to me.

(In reply to comment #1)
> > core: add logging macro _LOG() and _LOGD() to nm-device.c
> 
> I don't think we need the str_if_set() stuff in __LOG(), since the device
> should *always* have an 'iface'.  If it doesn't, something is wrong.

I think it could be useful to log during construction/finalization (during finalization, you have to take care of dangling pointers). This seems useful thing for debugging... the first and the last message of an NMDevice.


diff --git i/src/devices/nm-device.c w/src/devices/nm-device.c
index b24d35a..9a6f695 100644
--- i/src/devices/nm-device.c
+++ w/src/devices/nm-device.c
@@ -7104,8 +7104,10 @@ static void
 nm_device_init (NMDevice *self)
 {
     NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
 
+    _LOGD ("init NMDevice");
+
     priv->type = NM_DEVICE_TYPE_UNKNOWN;
     priv->capabilities = NM_DEVICE_CAP_NM_SUPPORTED;
     priv->state = NM_DEVICE_STATE_UNMANAGED;
     priv->state_reason = NM_DEVICE_STATE_REASON_NONE;
@@ -7329,17 +7331,18 @@ finalize (GObject *object)
     g_slist_free_full (priv->pending_actions, g_free);
     g_clear_pointer (&priv->physical_port_id, g_free);
     g_free (priv->udi);
     g_free (priv->path);
-    g_free (priv->iface);
+    g_clear_pointer (&priv->iface, NULL);
     g_free (priv->ip_iface);
     g_free (priv->driver);
     g_free (priv->driver_version);
     g_free (priv->firmware_version);
     g_free (priv->type_desc);
     if (priv->dhcp_anycast_address)
          g_byte_array_free (priv->dhcp_anycast_address, TRUE);
 
+    _LOGD ("finalize NMDevice");
     G_OBJECT_CLASS (nm_device_parent_class)->finalize (object);
 }
  

I would keep it because 
 (1) str_if_set() is cheap to evaluate
 (2) you only need to specify it at one place
 (3) better be safe then having minor gain of (1) and (2)
 (4) if something is actually wrong, logging should not interfere


> But more than that, I'm a fan of using explicit arguments for stuff, since it's
> clearer what's going on, so I'd actually prefer:
> 
> #define __LOG(device, level, domain, ...) \
>     G_STMT_START { \
>         if (LOGL_##level <= LOGL_DEBUG) \
>             NM_LOG(level, domain, device, "(%s): "
> _NM_UTILS_MACRO_FIRST(__VA_ARGS__), device, _NM_UTILS_MACRO_REST(__VA_ARGS__));
> \
>         else \
>             nm_log_##level (LOGD_##domain, "(%s): "
> _NM_UTILS_MACRO_FIRST(__VA_ARGS__), device, _NM_UTILS_MACRO_REST(__VA_ARGS__));
> \
>     } G_STMT_END
> #define _LOG(device, level, domain, ...)        __LOG(device, level, domain,
> __VA_ARGS__)
> #define _LOGD(device, ...)                      __LOG(device, debug, DEVICE,
> __VA_ARGS__)


Yes, possible. but _LOG() and _LOGD are exactly here to log messages of an object, inside the method of the object.

IMO for "methods", the first argument should be a pointer to @self, and it should be called "self". Hence, requiring to specify the name seems not necessary to me, only a lot of needless writing.

> _LOG (self, warn, HW, "failed to look up interface index");
> 
> that's also more flexible since we can determine which NMDevice to use,
> especially in the case of master/slave stuff.

I think that the cases where @self is not named "self" is a bug (in style) that should be fixed.

I did not find an existing example that supports the argument for master/slave.
In all master/save examples, the @self pointer is called @self, @dev, @device.

For example:
-nm_device_master_check_slave_physical_port (NMDevice *dev, NMDevice *slave,
+nm_device_master_check_slave_physical_port (NMDevice *self, NMDevice *slave,
                                             guint64 log_domain)



Anyway, I pushed a fixup to add a @self to _LOG_FULL() (renamed from __LOG()).
_LOG_FULL() gives you now the most flexibility.


> But really, in the case of warn/err/info, why should that get passed as an
> argument?  What's wrong with _LOG_INFO, _LOG_WARN, _LOG_ERR?  I think that's
> more explicit than passing the level as an argument, and most of the time
> what's important with a log message is the level it's printed at.

We already spoke about adding a trace level. Suppose we have such _LOG/_LOGD defines for several classes, we would have to add there a _LOG_TRACE too.

I don't understand why we have nm_log_info() et al. I also don't understand the advantage of _LOG_INFO().



Ok, one advantage: _LOG_INFO is easier to grep then '_LOG \?(info'.
Especially:
  _LOG(
       info, DEVICE, "hello world");
is hard to grep (but doesn't matter, since it is against our coding style).



now we have in nm-device.c:

#define _LOG_FULL(level, domain, self, ...)
#define _LOG(level, domain, ...)
#define _LOGD(...)


we could also imagine all kind of _LOG_<level>, with/without @domain parameter, with/without @self parameter. I just don't see the advantage these.

_LOGD() is as short as it gets, for the most common usecase (debug logging with @self, domain=DEVICE).


What are your concrete suggestions?




One thing I doubt is to shorten the log-domain.

e.g.:
_LOG (warn, DEVICE, ...
_LOG (warn, DEVICE | LOGD_AUTOIP4, ...
instead of
_LOG (warn, LOGD_DEVICE, ...
_LOG (warn, LOGD_DEVICE | LOGD_AUTOIP4, ...

Upside: shorter to write.
Downside: cannot grep for LOGD_DEVICE and cscope gets confused too.

Maybe I should change that in NM_LOG?





Pushed one additional commit, otherwise specifying NM_LOG_LAZY broke tests.


I wanted to make NM_LOG_LAZY the default thing to do. How do you feel about changing that? Note that I never saw a logging line with relevant side-effects. So, the change should be fine.

hm...

#define nm_log(domain, level, ...)
#define NM_LOG(level, domain, self, ...)
#define nm_log_info (domain, ...)
nm_logging_enabled (guint32 level, guint64 domain);

All functions should have always the same order of domain/level.

The pre-existing order as used by nm_log, _nm_log is less logical to me.

which one should I change?

(note that only very few calls to nm_log()/_nm_log() exist, so changing either one should be simple).

> utils: add _NM_UTILS_MACRO_FIRST and _NM_UTILS_MACRO_REST macros

_NM_UTILS_MACRO_FIRST is a neat hack. _NM_UTILS_MACRO_REST is an ugly hack. And you can avoid it anyway (see later).



> core/logging: deprecate name nm_log_dbg() in favor of nm_log_debug()

There's no reason to deprecate things inside src/. Either stick with dbg or rename it everywhere. (perl -pi -e 's/nm_log_dbg/nm_log_debug/;' `find src -name \*.[ch]`)

(I have no problem with nm_log_dbg(), but I have no problem with nm_log_debug() either. My biggest problem actually is with nm_log_warn(), because I always expect it to be nm_log_warning(), like g_warning().)



> core/logging: log lazily only after checking for nm_logging_enabled()

> Shaddow _nm_log() by a macro that checks first whether logging

"Shadow"

> This has the benefit, that the logging arguments don't have
> to be evaluated if logging is disabled.

Not opposed to this, but not sure we need it; we've already got nm_logging_enabled(), and in lots of cases where the arguments are expensive to generate, they have to be freed afterward as well, so you need to do the check yourself anyway.



> core/logging: add new logging macro NM_LOG()

As with the dbg/debug thing, we shouldn't have two systems. If we want to change to a single log macro/function, then we should drop the old ones.

(I don't really have a preference.)

>+        if (LOGL_##level <= LOGL_DEBUG) \

well, ok, the fact that you have to do this in NM_LOG() is one argument for having separate macros I guess.

Although really, I'd just make _nm_log() take a "self" argument and prepend the "[%p] " itself (like it does with the line number, etc). And then you don't need _NM_UTILS_MACRO_REST.

>+    nm_log_##level (LOGD_##domain, _NM_UTILS_MACRO_FIRST(__VA_ARGS__) _NM_UTILS_MACRO_REST(__VA_ARGS__)); \

I don't like "LOGD_##domain" for the reasons you said above. (Especially that it shortens the first domain but not any following ones.)



> core/trivial: change local variable names to @self

> static gboolean nm_device_master_add_slave (NMDevice *dev, NMDevice *slave, gboolean configure);
> static void nm_device_slave_notify_enslave (NMDevice *dev, gboolean success);
>-static void nm_device_slave_notify_release (NMDevice *dev, NMDeviceStateReason reason);
>+static void nm_device_slave_notify_release (NMDevice *self, NMDeviceStateReason reason);

It's weird that you changed notify_release() here, but not the two functions immediately above it...

I totally support being more consistent about the name of the self argument; it's really annoying when you move or copy code from one function to another and then it doesn't compile because one of the functions uses "device" and the other uses "dev".



> core: add logging macro _LOG() and _LOGD() to nm-device.c

why do you need __LOG? shouldn't this work:

#define _LOG(level, domain, ...)   NM_LOG(level, domain, self, __VA_ARGS__)
#define _LOGD(...)                 NM_LOG(debug, DEVICE, self, __VA_ARGS__)

?

(In reply to comment #4)
> > utils: add _NM_UTILS_MACRO_FIRST and _NM_UTILS_MACRO_REST macros
> 
> _NM_UTILS_MACRO_FIRST is a neat hack. _NM_UTILS_MACRO_REST is an ugly hack. And
> you can avoid it anyway (see later).

I don't agree, I think it is useful (see later).


> > core/logging: deprecate name nm_log_dbg() in favor of nm_log_debug()
> 
> There's no reason to deprecate things inside src/. Either stick with dbg or
> rename it everywhere. (perl -pi -e 's/nm_log_dbg/nm_log_debug/;' `find src
> -name \*.[ch]`)
> 
> (I have no problem with nm_log_dbg(), but I have no problem with nm_log_debug()
> either. My biggest problem actually is with nm_log_warn(), because I always
> expect it to be nm_log_warning(), like g_warning().)

It is more about consistency.

LOG_ERR => nm_log_err
LOG_WARN => nm_log_warn
LOGL_DEBUG => nm_log_debug

(ok, I admit, I dislike that we abbreviate ERR, but not DEBUG. But
that is how it is already exposed through DBUS API).

For now, I would add the alias nm_log_debug(). We can rename them all later (if
we choose to). I am not sure I want to change existing code for beauty reasons,
but I want to have the "correctly" named macro available and use it in future code.
Ok?




> > core/logging: log lazily only after checking for nm_logging_enabled()
> 
> > Shaddow _nm_log() by a macro that checks first whether logging
> 
> "Shadow"

Done


> > This has the benefit, that the logging arguments don't have
> > to be evaluated if logging is disabled.
> 
> Not opposed to this, but not sure we need it; we've already got
> nm_logging_enabled(), and in lots of cases where the arguments are expensive to
> generate, they have to be freed afterward as well, so you need to do the check
> yourself anyway.

but it also allows to refactoring creating the logmessage in a separate function:

  char buf[1024];
  nm_log_debug (LOGD_CORE, "%s", _construct_complex_message (buf));

or with gs_free:

  gs_free char *str = NULL;

  nm_log_debug (LOGD_CORE, "%s", (str = _construct_complex_message ()));


The only good reason not to do this, if we fear that we have logging statements
with side effects. Such logging would actually be a lingering bug, but it would
hit after the change.

Are you ok, with making this default already and omit the #ifdef NM_LOG_LAZY?


> > core/logging: add new logging macro NM_LOG()
> 
> As with the dbg/debug thing, we shouldn't have two systems. If we want to
> change to a single log macro/function, then we should drop the old ones.

I think that nm_log() (and it's wrappers nm_log_debug(), etc) are there to log a
generic statement. NM_LOG() is to log a message belonging to an object, with
printing the @self pointer value.

I renamed NM_LOG() to nm_log_obj(), I think that makes its intended use clearer.
(but it collides with nm_log_xyz() names, where LOGL_XYZ is a level, ... hfff).


> (I don't really have a preference.)
> 
> >+        if (LOGL_##level <= LOGL_DEBUG) \
> 
> well, ok, the fact that you have to do this in NM_LOG() is one argument for
> having separate macros I guess.

well. I don't have to do this, but it seems reasonable. For non-debug logging printing
a pointer value is ugly.


> Although really, I'd just make _nm_log() take a "self" argument and prepend the
> "[%p] " itself (like it does with the line number, etc). And then you don't
> need _NM_UTILS_MACRO_REST.

I wouldn't do that, because nm_log() (and _nm_log()) only prints a text (with 
format argument). Messages such as
  NetworkManager (version 0.9.11.0-8130.741ee7d051.fc20) is starting...
or messages for singleton objects don't have a meaningful @self pointer.


> >+    nm_log_##level (LOGD_##domain, _NM_UTILS_MACRO_FIRST(__VA_ARGS__) _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
> 
> I don't like "LOGD_##domain" for the reasons you said above. (Especially that
> it shortens the first domain but not any following ones.)

Changed.


> > core/trivial: change local variable names to @self
> 
> > static gboolean nm_device_master_add_slave (NMDevice *dev, NMDevice *slave, gboolean configure);
> > static void nm_device_slave_notify_enslave (NMDevice *dev, gboolean success);
> >-static void nm_device_slave_notify_release (NMDevice *dev, NMDeviceStateReason reason);
> >+static void nm_device_slave_notify_release (NMDevice *self, NMDeviceStateReason reason);
> 
> It's weird that you changed notify_release() here, but not the two functions
> immediately above it...

I only changed it where I needed it because the new _LOG() macros required
a @self argument.

The last commit is WIP, and if we agree on the shape of the branch, I might rework
the entire file.


> I totally support being more consistent about the name of the self argument;
> it's really annoying when you move or copy code from one function to another
> and then it doesn't compile because one of the functions uses "device" and the
> other uses "dev".
> 
> 
> 
> > core: add logging macro _LOG() and _LOGD() to nm-device.c
> 
> why do you need __LOG? shouldn't this work:
> 
> #define _LOG(level, domain, ...)   NM_LOG(level, domain, self, __VA_ARGS__)
> #define _LOGD(...)                 NM_LOG(debug, DEVICE, self, __VA_ARGS__)

I changed __LOG to _LOG_FULL which accepts a different self pointer.
Now we would have

_LOG_FULL
_LOG
_LOGD



Also swapped the order of level/domains for nm_log()


Repushed for review.

Reworked and repushed several things.

Now, you have to specify:
  nm_log_obj (LOGL_DEBUG, ...
instead of
  nm_log_obj (debug, ...

The argument is the same as for shortening the domain.

But since

  _LOG (LOGL_INFO, LOGD_DEVICE, ...

is now quite verbose (but probably frequently used), I added a _LOGI() macro for that.

You might want to drop "test: remove unused file test/nm-dhcp-opt-test.c" since danw is going to kill test/ in danw/reorg anyway...

> core/logging: add new logging macro nm_log_obj()

I initially thought we should add G_OBJECT_TYPE_NAME() somewhere in the log message too, since that's often useful instead of just a pointer address.  Thoughts?  Something like "[%p][%s]" maybe?

Branch looks good to me.

One note about 'self' though...  I think it's great to use that in NM core, but in the C libraries (util/glib) I've tried to stay away from using 'self' in the public function prototypes, since that's often less descriptive for API consumers than device/manager/connection/access_point/etc.  It's also what glib/GTK do and I've found it useful as a consumer of those APIs in the past.  Even so, we should be consistent in each module, so even in libnm-glib we should always use 'device' (not dev) or 'ap' (not access_point) etc.

(In reply to comment #8)
> > core/logging: add new logging macro nm_log_obj()
> 
> I initially thought we should add G_OBJECT_TYPE_NAME() somewhere in the log
> message too, since that's often useful instead of just a pointer address. 
> Thoughts?  Something like "[%p][%s]" maybe?

Seems a bit verbose. I added

  _LOGD ("constructor(): %s", G_OBJECT_TYPE_NAME (self));

to the constructor() function, so we get a very first hello-message, which associates the pointer value and it's type. The type is not going to change, and we can easily grep it. (Of course, this does not work, if you turn on DEBUG logging only after the device was created).

Anyway, having the new macro, we can change it anytime in *one* place and adjust the format to whatever we want.


But this made me thinking, that there is a distinction whether we want to print messages for a GObject or an opaque pointer. I added nm_log_obj() and  nm_log_ptr() for that.


> One note about 'self' though...  I think it's great to use that in NM core, but
> in the C libraries (util/glib) I've tried to stay away from using 'self' in the
> public function prototypes, since that's often less descriptive for API
> consumers than device/manager/connection/access_point/etc.  It's also what
> glib/GTK do and I've found it useful as a consumer of those APIs in the past. 
> Even so, we should be consistent in each module, so even in libnm-glib we
> should always use 'device' (not dev) or 'ap' (not access_point) etc.

The used name is defined locally in nm-device.c itself:

+#define _LOG(level, domain, ...) _LOG_FULL(level,      domain,      self, __VA_ARGS__)
+#define _LOGD(...)               _LOG_FULL(LOGL_DEBUG, LOGD_DEVICE, self, __VA_ARGS__)
+#define _LOGI(...)               _LOG_FULL(LOGL_INFO,  LOGD_DEVICE, self, __VA_ARGS__)
 
So, if we would decide that @self in NMDevice shall be "device" (and "ap" in NMAccessPoint), the _LOG() macros would allow for that..




I reworked now the entire nm-device.c file. I think the result is neat.



Together with the lazy evaluation of logging, the following is a nice pattern (I think):

-    char *cmd;
+    gs_free char *tmp_str = NULL;
 
-    if (nm_logging_enabled (LOGL_DEBUG, log_domain)) {
-         cmd = g_strjoinv (" ", (gchar **) args);
-         nm_log_dbg (log_domain, "(%s): running '%s'",
-                     nm_device_get_iface (self),
-                     cmd);
-         g_free (cmd);
-    }
+    _LOG (LOGL_DEBUG, log_domain, "ping: running '%s'",
+          (tmp_str = g_strjoinv (" ", (gchar **) args)));



I think these _LOG() macros are a neat and should be adopted to other files as well.

(In reply to comment #5)
> For now, I would add the alias nm_log_debug(). We can rename them all later (if
> we choose to). I am not sure I want to change existing code for beauty reasons,
> but I want to have the "correctly" named macro available and use it in future
> code.
> Ok?

No. There is no reason to have two names for a purely-internal function. Either rename it everywhere, or don't add the new name. (I don't care which.)

> Are you ok, with making this default already and omit the #ifdef NM_LOG_LAZY?

I don't care whether we make the macros lazy or not, but don't make it optional.

> I renamed NM_LOG() to nm_log_obj(), I think that makes its intended use
> clearer.

ok

> > Although really, I'd just make _nm_log() take a "self" argument and prepend the
> > "[%p] " itself (like it does with the line number, etc). And then you don't
> > need _NM_UTILS_MACRO_REST.
> 
> I wouldn't do that, because nm_log() (and _nm_log()) only prints a text (with 
> format argument). Messages such as
>   NetworkManager (version 0.9.11.0-8130.741ee7d051.fc20) is starting...
> or messages for singleton objects don't have a meaningful @self pointer.

Right, I meant, nm_log_obj() would pass its self arg to _nm_log(), and nm_log() would pass NULL to _nm_log(). And then if _nm_log() got a non-NULL self, it would decide whether or not to show it, based on level, etc.



> core/logging: replace usage of G_STRLOC
> 
> On GCC version before 3.x, G_STRLOC includes the function name.

Uh... that seems like a mistake in the macro. At any rate, GCC that old wouldn't support __attribute__((cleanup)), so you couldn't compile NM with it anyway.



> core/logging: define nm_log() to checking for nm_logging_enabled() first

"to check for"



> fixup! core/logging: add new logging macro nm_log_obj()

Why isn't _nm_log_obj()/nm_log_obj() defined in terms of _nm_log_ptr()/nm_log_ptr()?



> core: add logging macro _LOG() and _LOGD() to nm-device.c

>+#define _LOG_FULL(level, domain, self, ...) \
>+	G_STMT_START { \
>+		if ((level) <= LOGL_DEBUG) { \
>+			nm_log_obj ((level), (domain), self, "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), self ? str_if_set (nm_device_get_iface (self), "(null)") : "(none)" _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
>+		} else { \
>+			nm_log ((level), (domain), "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), self ? str_if_set (nm_device_get_iface (self), "(null)") : "(none)" _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
>+		} \
>+	} G_STMT_END

The question from the last review still stands. nm_log_obj() already deals with the debug vs non-debug special case, so why do you also need to deal with it here? Won't this work?:

    #define _LOG_FULL(level, domain, self, ...) \
        G_STMT_START { \
            nm_log_obj ((level), (domain), self, "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), self ? str_if_set (nm_device_get_iface (self), "(null)") : "(none)" _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
        } G_STMT_END


I don't really like the names either... I don't think having to type out the log level in full was really killing anyone. Also, the fact that you have to do something completely different in the case where you're logging against multiple log domains.

(In reply to comment #10)
> (In reply to comment #5)
> > For now, I would add the alias nm_log_debug(). We can rename them all later (if
> > we choose to). I am not sure I want to change existing code for beauty reasons,
> > but I want to have the "correctly" named macro available and use it in future
> > code.
> > Ok?
> 
> No. There is no reason to have two names for a purely-internal function. Either
> rename it everywhere, or don't add the new name. (I don't care which.)

originally, I called _nm_log_##level, so it was more important to have the function name match the LOGL_##level name.
(ok, it was still ugly, I had to define _LOGL_debug too :) ). Anyway, I removed it, so this patch is even less useful.

I still dislike the difference, but I don't want to refactor all the code now. Drop it.



> > Are you ok, with making this default already and omit the #ifdef NM_LOG_LAZY?
> 
> I don't care whether we make the macros lazy or not, but don't make it
> optional.

ok


> > I renamed NM_LOG() to nm_log_obj(), I think that makes its intended use
> > clearer.
> 
> ok

ok


> > > Although really, I'd just make _nm_log() take a "self" argument and prepend the
> > > "[%p] " itself (like it does with the line number, etc). And then you don't
> > > need _NM_UTILS_MACRO_REST.
> > 
> > I wouldn't do that, because nm_log() (and _nm_log()) only prints a text (with 
> > format argument). Messages such as
> >   NetworkManager (version 0.9.11.0-8130.741ee7d051.fc20) is starting...
> > or messages for singleton objects don't have a meaningful @self pointer.
> 
> Right, I meant, nm_log_obj() would pass its self arg to _nm_log(), and nm_log()
> would pass NULL to _nm_log(). And then if _nm_log() got a non-NULL self, it
> would decide whether or not to show it, based on level, etc.

I understand that nm_log() could ignore NULL self arguments in case where we don't have a valid NULL argument. But that already shows that it is wrong. I feel, nm_log() should log a message (ok, it appends the time+STRLOC, but basically, it logs *one string*). Then, build on top of that we have more specialized functions/macros such as nm_log_obj() or nm_log_ptr().



> > core/logging: replace usage of G_STRLOC
> > 
> > On GCC version before 3.x, G_STRLOC includes the function name.
> 
> Uh... that seems like a mistake in the macro. At any rate, GCC that old
> wouldn't support __attribute__((cleanup)), so you couldn't compile NM with it
> anyway.

dropped.


> > core/logging: define nm_log() to checking for nm_logging_enabled() first
> 
> "to check for"

fixed


> > fixup! core/logging: add new logging macro nm_log_obj()
> 
> Why isn't _nm_log_obj()/nm_log_obj() defined in terms of
> _nm_log_ptr()/nm_log_ptr()?

to make it more explicit, that they are *not* the same. They just
happen to be identical for now. Anyway. I changed it.



> > core: add logging macro _LOG() and _LOGD() to nm-device.c
> 
> >+#define _LOG_FULL(level, domain, self, ...) \
> >+	G_STMT_START { \
> >+		if ((level) <= LOGL_DEBUG) { \
> >+			nm_log_obj ((level), (domain), self, "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), self ? str_if_set (nm_device_get_iface (self), "(null)") : "(none)" _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
> >+		} else { \
> >+			nm_log ((level), (domain), "(%s): " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), self ? str_if_set (nm_device_get_iface (self), "(null)") : "(none)" _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
> >+		} \
> >+	} G_STMT_END
> 
> The question from the last review still stands. nm_log_obj() already deals with
> the debug vs non-debug special case, so why do you also need to deal with it
> here? Won't this work?:
> 
>     #define _LOG_FULL(level, domain, self, ...) \
>         G_STMT_START { \
>             nm_log_obj ((level), (domain), self, "(%s): "
> _NM_UTILS_MACRO_FIRST(__VA_ARGS__), self ? str_if_set (nm_device_get_iface
> (self), "(null)") : "(none)" _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
>         } G_STMT_END

Again, to make it more explicit. But I changed it too.



> I don't really like the names either... I don't think having to type out the
> log level in full was really killing anyone. Also, the fact that you have to do
> something completely different in the case where you're logging against
> multiple log domains.

I changed it somehow. Do you like it better?

I did not make the names longer. Do you feel strongly about it? What would you like better instead?

I like that the names are now consistent, eg, I/E/W/D refer to the level and the domain is specified manually.  I'm now OK with the state of the branch.

I'm somewhat ambivalent on the names; I don't care which way we go.

sure, looks good

Merged to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=c123a24dc472b867d9e8a0f83776d258d156f0c1