On Thu, Jan 16, 2025 at 01:54:20PM +1030, Jack Burton wrote:
> On Wed, 15 Jan 2025 14:07:56 +0100
> Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> > On Tue, Dec 31, 2024 at 06:30:02PM +1030, Jack Burton wrote:
> > > Fix memory leaks in some members of struct kvp.
> 
> ...
> 
> > > cvs server: Diffing .
> > > Index: connection.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v
> > > diff -u -p -r1.21 connection.c
> > > --- connection.c	5 Dec 2015 06:38:18 -0000	1.21
> > > +++ connection.c	30 Dec 2024 05:56:32 -0000
> > > @@ -327,11 +327,13 @@ conn_gen_kvp(struct connection *c, struc
> > >  			kvp[i].key = strdup("MaxConnections");  
> > 
> > I think here it would make sense to kill the strdup and the
> > KVP_KEY_ALLOCED. Since the key can be a static string.
> 
> Yes absolutely; and same for the other example you pointed out too.
> 
> My patch #3 replaces both of those with a more generic approach anyway.
> 
> ...
> 
> > I wonder if pdu_to_text() should actually alloc the kvp key and value
> > instead of fiddling with the pdu buffer internals.
> 
> Perhaps.  If we adopt a convention that the kvp key & value are always
> alloced individually, then we can dispense with the alloced-or-not
> distinction (and the kvp flags) altogether.  That might help to
> simplify things in a few places.  The cost is negligible, since we only
> use kvps for text/login pdus (so it won't affect performance of scsi
> command/data pdus).
> 
> 
> > I will commit this diff with the above mentioned changes tomorrow.
> 
> Thanks; much appreciated.

Diff is in. I guess we could really always allocate value and always use a
const char for the key (I see no reason to support or enforce allocation
there).

-- 
:wq Claudio