Re: [PATCH 5/6] libmultipath: Fix sgio_get_vpd()

From: Martin Wilck <mwilck@suse.com>
To: Bart Van Assche <Bart.VanAssche@wdc.com>,
	"christophe.varoqui@opensvc.com" <christophe.varoqui@opensvc.com>
Cc: "dm-devel@redhat.com" <dm-devel@redhat.com>
Subject: Re: [PATCH 5/6] libmultipath: Fix sgio_get_vpd()
Date: Tue, 06 Mar 2018 00:24:03 +0100	[thread overview]
Message-ID: <1520292243.7660.57.camel@suse.com> (raw)
In-Reply-To: <1520284430.2826.39.camel@wdc.com>

On Mon, 2018-03-05 at 21:13 +0000, Bart Van Assche wrote:
> On Mon, 2018-03-05 at 21:47 +0100, Martin Wilck wrote:
> > On Mon, 2018-03-05 at 19:18 +0000, Bart Van Assche wrote:
> > > On Mon, 2018-03-05 at 20:14 +0100, Martin Wilck wrote:
> > > > Unless you object, I'll repost your series rebased on mine.
> > > 
> > > Hello Martin,
> > > 
> > > Before you start working on that: has your patch series already
> > > been
> > > posted
> > > on the dm-devel mailing list?
> > 
> > Yes. "PATCH v2 00/20] Various multipath-tools fixes" ff.
> > https://www.redhat.com/archives/dm-devel/2018-January/msg00219.html
> 
> Ah, thanks, but unfortunately these patches are no longer in my
> mailbox. I
> pulled these from https://github.com/openSUSE/multipath-tools. I'm
> fine with
> my patches being rebased on top of your series, whether or not the
> following
> issues get addressed:
> * Several patches that are on the upstream-queue branch introduce
> trailing
>   whitespace.
> * The macro FREE_CONST() should never have been introduced.
> Introducing such
>   a macro namely introduces the risk of calling free() for a string
> constant,
>   something that should never happen. Have you considered to declare
>   dynamically allocated strings, e.g. the result of strdup(), as char
> *
>   instead of const char * ? I think with that change the FREE_CONST()
> macro
>   definitions can be removed again.

Another philosophical issue :-) I disagree with you here. The name
FREE_CONST is obvious enough that people should think twice before
using it. I have indeed considered what you propse, but I like being
able to use "const char*" when I work with strings that shouldn't be
changed. An accidental write to a const location is much easier to
overlook than a FREE_CONST(). Freeing a "const char*" isn't always
wrong, just if the pointer was passed in from elsewhere.

This issue is philosophical but not religious to me, so if you and
maybe others are offended so much by FREE_CONST, I may ditch it again.

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel