From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: Re: relayd: fix dead store
To: Theo Buehler <tb@theobuehler.org>,  Crystal Kolipe <kolipe.c@exoticsilicon.com>
Cc: tech@openbsd.org
Date: Mon, 29 Dec 2025 11:39:12 +0100

On Mon Dec 29, 2025 at 11:14:43AM +0100, Theo Buehler wrote:
> On Mon, Dec 29, 2025 at 10:44:47AM +0100, Rafael Sadowski wrote:
> > First c is never used  so check if carp_group_find is NULL and return.
> > 
> > carp.c:64:7: warning: Although the value stored to 'c' is used in the
> > enclosing expression, the value is never actually read from 'c'
> > [deadcode.DeadSt ores]
> >    64 |         if ((c = carp_group_find(group)) == NULL)
> > 
> > 
> > diff --git a/usr.sbin/relayd/carp.c b/usr.sbin/relayd/carp.c
> > index 6a6a55a0fbb..34b6b6785a2 100644
> > --- a/usr.sbin/relayd/carp.c
> > +++ b/usr.sbin/relayd/carp.c
> > @@ -61,29 +61,31 @@ carp_demote_init(char *group, int force)
> >  	struct carpgroup	*c;
> >  	int			 level;
> >  
> > -	if ((c = carp_group_find(group)) == NULL) {
> 
> I would just do this and leave the rest as it is:
> 
> -	if ((c = carp_group_find(group)) == NULL) {
> +	if (carp_group_find(group) == NULL) {
> 
> [...]
> 
> > +	if (carp_group_find(group) == NULL) {
> 
> If you do want to unindent, then you need to check for != NULL here.
> (If a carp group is found, do nothing and succeed, otherwise create a
> new one and add it to the carpgroups queue)
> 
> > +		return (0);
> > +	}
> 

Thanks Theo and Crystal Kolipe for your feedback. You are absolutely
right about the NULL check, of course.

I would like to unindent. I really like this guard clause pattern, since
the rest of the code follows this pattern as well.

diff --git a/usr.sbin/relayd/carp.c b/usr.sbin/relayd/carp.c
index 6a6a55a0fbb..34f9c16492b 100644
--- a/usr.sbin/relayd/carp.c
+++ b/usr.sbin/relayd/carp.c
@@ -61,29 +61,28 @@ carp_demote_init(char *group, int force)
 	struct carpgroup	*c;
 	int			 level;
 
-	if ((c = carp_group_find(group)) == NULL) {
-		if ((c = calloc(1, sizeof(struct carpgroup))) == NULL) {
-			log_warn("%s: calloc", __func__);
-			return (-1);
-		}
-		if ((c->group = strdup(group)) == NULL) {
-			log_warn("%s: strdup", __func__);
-			free(c);
-			return (-1);
-		}
-
-		/* only demote if this group already is demoted */
-		if ((level = carp_demote_get(group)) == -1) {
-			free(c->group);
-			free(c);
-			return (-1);
-		}
-		if (level > 0 || force)
-			c->do_demote = 1;
-
-		TAILQ_INSERT_TAIL(&carpgroups, c, entry);
+	if (carp_group_find(group) != NULL)
+		return (0);
+
+	if ((c = calloc(1, sizeof(struct carpgroup))) == NULL) {
+		log_warn("%s: calloc", __func__);
+		return (-1);
+	}
+	if ((c->group = strdup(group)) == NULL) {
+		log_warn("%s: strdup", __func__);
+		free(c);
+		return (-1);
+	}
+	/* only demote if this group already is demoted */
+	if ((level = carp_demote_get(group)) == -1) {
+		free(c->group);
+		free(c);
+		return (-1);
 	}
+	if (level > 0 || force)
+		c->do_demote = 1;
 
+	TAILQ_INSERT_TAIL(&carpgroups, c, entry);
 	return (0);
 }