Hi,
On Mon, Jun 07, 2010 at 03:53:19PM +0200, Andreas Mock wrote:
> -----Ursprüngliche Nachricht-----
> Von: Lars Ellenberg <[email protected]>
> Gesendet: 07.06.2010 15:34:53
> An: [email protected]
> Betreff: Re: [Linux-ha-dev] [PATCH] patch for ha_logger.c
>
> >Your patch seems to change a lot of whitespace as well,
> >which makes it not obvious what exactly the functional change is.
>
> Yes, indeed.
> In this case it's better to look at the changed source code as it
> is about one page of source code and the target code is
> much more readable (IMHO of course) as the original one.
>
> IMHO it's also a indication if someone is able to code with a
> little bit of coding style. Whitespace and indention is a part of
> it. I changed cluttered tabs to spaces and changed the
> different versions of whitespacing around braces.
That's a matter of taste.
> It would be a pitty to "enhance" this piece of code with the
> argument that the patch has to much difference.
> The way pointer arithmetic is done is IMHO not the cleanest
> and therefore defensive coding style.
That may be true.
> >But thanks for pointing out the place where things could go wrong.
> >I think the original intention was more like
> >
> >diff -r c447fc25e119 logd/ha_logger.c
> >--- a/logd/ha_logger.c Fri Apr 23 19:32:43 2010 +0200
> >+++ b/logd/ha_logger.c Mon Jun 07 15:30:55 2010 +0200
> >@@ -105,12 +105,11 @@
> > if (p + len > endp && p > buf) {
> > if (LogToDaemon(priority,buf,
> > strnlen(buf, 1024),FALSE)
> > ==HA_OK){
> >+ p = buf;
> > continue;
> > }else{
> > return EXIT_FAIL;
> > }
> >- /* NOTREACHED */
> >- /* p = buf; */
> > }
> > if (len > sizeof(buf) - 1) {
> > if (LogToDaemon(priority,*argv,
> >
> >
> >
> >and the "NOTREACHED" comment added two and a half years later
> >was correct, but failed to see and fix the bug instead.
> >
> >Not tested either, but if that on-line patch works,
> >this would be the change I'd go for.
>
> Please not with the argument that the diff is shorter. ;-)
That argument is very important too. Many thanks for your
effort, but this time Lars' patch wins. In case it works, that is :)
Thanks,
Dejan
> Best regards
> Andreas Mock
> _______________________________________________________
> Linux-HA-Dev: [email protected]
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/
_______________________________________________________
Linux-HA-Dev: [email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
