On Mon, Nov 15, 2010 at 03:27:39PM +0100, Bernd Schubert wrote: > Hello Lars, > > firstly thanks a lot for your super fast review and reply. I will send > updated > patches on Wednesday. > > On Friday, November 12, 2010, Lars Ellenberg wrote: > > On Fri, Nov 12, 2010 at 05:04:39PM +0100, [email protected] wrote: > > > # HG changeset patch > > > # User Bernd Schubert <[email protected]> > > > # Date 1289577717 -3600 > > > # Node ID 39cc45ad8013fbf725254974070407fee2e1027b > > > # Parent ad13ee71cc6672092b13a73e89c7b0894faa5146 > > > cl_log: Simplify a function > > > > > > Signed-off-by: Bernd Schubert <[email protected]> > > > > > > diff --git a/lib/clplumbing/cl_log.c b/lib/clplumbing/cl_log.c > > > --- a/lib/clplumbing/cl_log.c > > > +++ b/lib/clplumbing/cl_log.c > > > @@ -534,7 +534,7 @@ cl_direct_log(int priority, const char* > > > > > > entity =cl_log_entity; > > > > > > } > > > > > > - pristr = use_priority_str ? prio2str(priority) : NULL; > > > + pristr = use_priority_str ? prio2str(priority) : ""; > > > > > > if (needprivs) { > > > > > > return_to_orig_privs(); > > > > > > @@ -544,17 +544,13 @@ cl_direct_log(int priority, const char* > > > > > > if (entity) { > > > > > > strncpy(common_log_entity, entity, MAXENTITY); > > > > > > } else { > > > > > > - strncpy(common_log_entity, DFLT_ENTITY,MAXENTITY); > > > + strncpy(common_log_entity, DFLT_ENTITY, MAXENTITY); > > > > > > } > > > > > > common_log_entity[MAXENTITY-1] = '\0'; > > > > > > - if (pristr) { > > > - syslog(priority, "[%d]: %s: %s%c", > > > - entity_pid, pristr, buf, 0); > > > - }else { > > > - syslog(priority, "[%d]: %s%c", entity_pid, buf, 0); > > > - } > > > + syslog(priority, "[%d]: %s: %s%c", > > > + entity_pid, pristr, buf, 0); > > > > This leaves an ugly colon. "[1234]: : message here" > > Why? > > What's wrong with an extra test on NULL > > for the use_priority_str == 0 case? > > I would like to avoid two times almost the same syslog() line if possible. > And I didn't mind "[1234]: : message here"... > So we have two choices - either drop the patch,
I don't see anything wrong with the current code there. > or modify the patch, keep the if-condition and already add ": " there > to pristr. > Keeping 'if', we could remove the initial pristr assignment. Sure. But again, why. The code works fine as is, and is not that complex or ugly that it actually needs a cleanup. I don't believe in "cleanup just because we can" ;-) as that quickly leads to matter-of-taste discussions and the bikeshed syndrom. -- : Lars Ellenberg : LINBIT | Your Way to High Availability : DRBD/HA support and consulting http://www.linbit.com DRBD® and LINBIT® are registered trademarks of LINBIT, Austria. _______________________________________________________ Linux-HA-Dev: [email protected] http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/
