Re: [libvirt] [PATCH v3] util: systemd: Define MSG_NOSIGNAL if needed

On Wed, Jul 20, 2016 at 03:11:51PM +0100, Justin Clift wrote: > On 20 Jul 2016, at 14:46, Andrea Bolognani <abologna(a)redhat.com&gt; wrote: >> The symbol being missing has been reported as causing build >> failures on OS X. If it's not already defined, define it to >> zero so that it won't have any effect. >> --- >> Changes from v2: >> >> * define MSG_NOSIGNAL to zero if not already defined >> instead of conditionally compiling the code depending >> on it >> >> src/util/virsystemd.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c >> index 969cd68..7d6985b 100644 >> --- a/src/util/virsystemd.c >> +++ b/src/util/virsystemd.c >> @@ -41,6 +41,10 @@ >> >> VIR_LOG_INIT("util.systemd"); >> >> +#ifndef MSG_NOSIGNAL >> +# define MSG_NOSIGNAL 0 >> +#endif >> + >> static void virSystemdEscapeName(virBufferPtr buf, >> const char *name) >> { >> -- >> 2.7.4 > > This compiles fine on OSX too. :) > > One niggle with it though... this is what is looks like in context: > > ******************************************************************* > ... > #include "virerror.h" > #include "virfile.h" > > #define VIR_FROM_THIS VIR_FROM_SYSTEMD > > VIR_LOG_INIT("util.systemd"); > > #ifndef MSG_NOSIGNAL > # define MSG_NOSIGNAL 0 > #endif > > static void virSystemdEscapeName(virBufferPtr buf, > const char *name) > { > static const char hextable[16] = "0123456789abcdef"; > > #define ESCAPE(c) > ... > ******************************************************************* > > Isn't it kind of crying out for a useful comment, so the next person > looking through this code has a good chance to understand why it's > there? ;) > > Maybe something like: > > /* OSX is missing MSG_NOSIGNAL, so we define it here to avoid > * failure during building. > */ Not really - it is pretty obvious what the code is doing so adding such a comment doesn't really add much value.