shithub: riscv

Download patch

ref: 9e8a7578f063f4caa5ceae9b997ff72a88dbaa58
parent: 0afd20e4c409ea8b05916654783150c87dfa0c33
author: cinap_lenrek <cinap_lenrek@felloff.net>
date: Sat Jun 3 12:58:13 EDT 2023 

devsd: Fix memory leaks, wstst check, cleanup

Free all the name and user strings of the partitions
and files of a unit when removing a device.

Reorder wstat check, so we only modify the state
once all checks are done (committed).

Cleanup the code, use explicit comparsions with nil
for pointers. Use (*) for indirect function calls...


--- a/sys/src/9/port/devsd.c
+++ b/sys/src/9/port/devsd.c
@@ -78,8 +78,38 @@
 #define QID(d,u, p, t)	(((d)<<DevSHIFT)|((u)<<UnitSHIFT)|\
 					 ((p)<<PartSHIFT)|((t)<<TypeSHIFT))
 
+static void
+freeperm(SDperm *perm)
+{
+	free(perm->name);
+	perm->name = nil;
+	free(perm->user);
+	perm->user = nil;
+}
 
 static void
+freeunit(SDunit *unit)
+{
+	int i;
+
+	if(unit == nil)
+		return;
+
+	for(i = 0; i < unit->nefile; i++)
+		freeperm(&unit->efile[i]);
+
+	for(i = 0; i < unit->npart; i++)
+		freeperm(&unit->part[i]);
+	free(unit->part);
+
+	freeperm(&unit->ctlperm);
+	freeperm(&unit->rawperm);
+	freeperm(unit);
+
+	free(unit);
+}
+
+static void
 sdaddpart(SDunit* unit, char* name, uvlong start, uvlong end)
 {
 	SDpart *pp;
@@ -204,8 +234,9 @@
 		return 0;
 	}
 
-	if(unit->dev->ifc->online)
-		unit->dev->ifc->online(unit);
+	if(unit->dev->ifc->online != nil)
+		(*unit->dev->ifc->online)(unit);
+
 	if(unit->sectors){
 		sdincvers(unit);
 		sdaddpart(unit, "data", 0, unit->sectors);
@@ -220,7 +251,7 @@
 		 */
 		snprint(buf, sizeof buf, "%spart", unit->name);
 		for(p = getconf(buf); p != nil; p = q){
-			if(q = strchr(p, '/'))
+			if((q = strchr(p, '/')) != nil)
 				*q++ = '\0';
 			nf = tokenize(p, f, nelem(f));
 			if(nf < 3)
@@ -259,7 +290,7 @@
 		return nil;
 
 	qlock(&devslock);
-	if(sdev = devs[i])
+	if((sdev = devs[i]) != nil)
 		incref(&sdev->r);
 	qunlock(&devslock);
 	return sdev;
@@ -303,8 +334,8 @@
 
 		if(waserror())
 			goto Error;
-		if(sdev->enabled == 0)
-			sdev->enabled = sdev->ifc->enable == nil || sdev->ifc->enable(sdev);
+		if(!sdev->enabled)
+			sdev->enabled = sdev->ifc->enable == nil || (*sdev->ifc->enable)(sdev);
 
 		/*
 		 * No need to lock anything here as this is only
@@ -311,13 +342,11 @@
 		 * called before the unit is made available in the
 		 * sdunit[] array.
 		 */
-		if(sdev->enabled == 0 || sdev->ifc->verify(unit) == 0){
+		if(!sdev->enabled || (*sdev->ifc->verify)(unit) == 0){
 			poperror();
 		Error:
 			qunlock(&sdev->unitlock);
-			free(unit->name);
-			free(unit->user);
-			free(unit);
+			freeunit(unit);
 			return nil;
 		}
 		poperror();
@@ -351,15 +380,15 @@
 	for(; sdev; sdev=next){
 		next = sdev->next;
 
-		sdev->unit = malloc(sdev->nunit * sizeof(SDunit*));
-		sdev->unitflg = malloc(sdev->nunit * sizeof(int));
+		sdev->unit = malloc(sdev->nunit * sizeof(sdev->unit[0]));
+		sdev->unitflg = malloc(sdev->nunit * sizeof(sdev->unitflg[0]));
 		if(sdev->unit == nil || sdev->unitflg == nil){
 			print("sdadddevs: out of memory\n");
 		giveup:
-			if(sdev->ifc->clear)
-				sdev->ifc->clear(sdev);
-			free(sdev->unit);
+			if(sdev->ifc->clear != nil)
+				(*sdev->ifc->clear)(sdev);
 			free(sdev->unitflg);
+			free(sdev->unit);
 			free(sdev);
 			continue;
 		}
@@ -385,15 +414,6 @@
 	}
 }
 
-// void
-// sdrmdevs(SDev *sdev)
-// {
-// 	char buf[2];
-//
-// 	snprint(buf, sizeof buf, "%c", sdev->idno);
-// 	unconfigure(buf);
-// }
-
 static int
 sd2gen(Chan* c, SDunit *unit, int i, Dir* dp)
 {
@@ -464,7 +484,7 @@
 		mkqid(&q, QID(0, 0, 0, Qtopctl), 0, QTFILE);
 		qlock(&topctlunit.ctl);
 		p = &topctlunit.ctlperm;
-		if(p->user == nil || p->user[0] == 0){
+		if(emptystr(p->user)){
 			kstrdup(&p->name, "sdctl");
 			kstrdup(&p->user, eve);
 			p->perm = 0640;
@@ -579,7 +599,7 @@
 		 * Online is a bit of a large hammer but does the job.
 		 */
 		if(unit->sectors == 0
-		|| (unit->dev->ifc->online && unit->dev->ifc->online(unit) > 1))
+		|| (unit->dev->ifc->online != nil && (*unit->dev->ifc->online)(unit) > 1))
 			sdinitpart(unit);
 
 		i = s+Qunitbase;
@@ -657,7 +677,7 @@
 	if(subno < 0 || p == &spec[3])
 		error(Ebadspec);
 
-	if((sdev=sdgetdev(idno)) == nil)
+	if((sdev = sdgetdev(idno)) == nil)
 		error(Enonexist);
 	if(waserror()){
 		decref(&sdev->r);
@@ -838,7 +858,7 @@
 
 	if(write){
 		if(hard){
-			l = unit->dev->ifc->bio(unit, 0, 0, b, nb, bno);
+			l = (*unit->dev->ifc->bio)(unit, 0, 0, b, nb, bno);
 			if(l < 0)
 				error(Eio);
 			if(l < (nb*unit->secsize)){
@@ -850,7 +870,7 @@
 		}
 		if(allocd)
 			memmove(b+offset, a, len);
-		l = unit->dev->ifc->bio(unit, 0, 1, b, nb, bno);
+		l = (*unit->dev->ifc->bio)(unit, 0, 1, b, nb, bno);
 		if(l < 0)
 			error(Eio);
 		if(l < offset)
@@ -859,7 +879,7 @@
 			len = l - offset;
 	}
 	else{
-		l = unit->dev->ifc->bio(unit, 0, 0, b, nb, bno);
+		l = (*unit->dev->ifc->bio)(unit, 0, 0, b, nb, bno);
 		if(l < 0)
 			error(Eio);
 		if(l < offset)
@@ -928,7 +948,7 @@
 	}
 	if(f == nil)
 		error(errstr);
-	rv = f(r);
+	rv = (*f)(r);
 	if(r->flags & SDvalidsense){
 		memmove(u->rsense, r->sense, sizeof u->rsense);
 		u->haversense = 1;
@@ -1215,8 +1235,8 @@
 			sdev = devs[i];
 			if(sdev == nil)
 				continue;
-			if(sdev->ifc->rtopctl)
-				p = sdev->ifc->rtopctl(sdev, p, e);
+			if(sdev->ifc->rtopctl != nil)
+				p = (*sdev->ifc->rtopctl)(sdev, p, e);
 			else
 				p = deftopctl(sdev, p, e);
 		}
@@ -1251,8 +1271,8 @@
 		 * provide all information pertaining to night geometry
 		 * and the garscadden trains.
 		 */
-		if(unit->dev->ifc->rctl)
-			p += unit->dev->ifc->rctl(unit, p, e - p);
+		if(unit->dev->ifc->rctl != nil)
+			p += (*unit->dev->ifc->rctl)(unit, p, e - p);
 		if(unit->sectors == 0)
 			sdinitpart(unit);
 		if(unit->sectors){
@@ -1259,12 +1279,11 @@
 			if(unit->dev->ifc->rctl == nil)
 				p = seprint(p, e, "geometry %llud %lud\n",
 					unit->sectors, unit->secsize);
-			pp = unit->part;
 			for(i = 0; i < unit->npart; i++){
+				pp = &unit->part[i];
 				if(pp->valid)
 					p = seprint(p, e, "part %s %llud %llud\n",
 						pp->name, pp->start, pp->end);
-				pp++;
 			}
 		}
 		qunlock(&unit->ctl);
@@ -1358,7 +1377,7 @@
 		 */
 		ifc = nil;
 		sdev = nil;
-		for(i=0; sdifc[i]; i++){
+		for(i=0; sdifc[i] != nil; i++){
 			if(strcmp(sdifc[i]->name, f0) == 0){
 				ifc = sdifc[i];
 				sdev = nil;
@@ -1384,8 +1403,8 @@
 				decref(&sdev->r);
 			nexterror();
 		}
-		if(ifc->wtopctl)
-			ifc->wtopctl(sdev, cb);
+		if(ifc->wtopctl != nil)
+			(*ifc->wtopctl)(sdev, cb);
 		else
 			error(Ebadctl);
 		if(sdev != nil)
@@ -1423,8 +1442,8 @@
 				error(Ebadctl);
 			sddelpart(unit, cb->f[1]);
 		}
-		else if(unit->dev->ifc->wctl)
-			unit->dev->ifc->wctl(unit, cb);
+		else if(unit->dev->ifc->wctl != nil)
+			(*unit->dev->ifc->wctl)(unit, cb);
 		else
 			error(Ebadctl);
 		qunlock(&unit->ctl);
@@ -1558,10 +1577,11 @@
 		error(Eperm);
 	if(!emptystr(d[0].muid) || !emptystr(d[0].name))
 		error(Eperm);
-	if(!emptystr(d[0].uid))
-		kstrdup(&perm->user, d[0].uid);
 	if(!emptystr(d[0].gid) && strcmp(d[0].gid, eve) != 0)
 		error(Eperm);
+	/* committed */
+	if(!emptystr(d[0].uid))
+		kstrdup(&perm->user, d[0].uid);
 	if(d[0].mode != ~0UL)
 		perm->perm = (perm->perm & ~0777) | (d[0].mode & 0777);
 	qunlock(&unit->ctl);
@@ -1591,7 +1611,7 @@
 			break;
 	if(sdifc[i] == nil)
 		error("sd type not found");
-	if(p)
+	if(p != nil)
 		*(p-1) = '/';
 
 	if(sdifc[i]->probe == nil)
@@ -1598,7 +1618,7 @@
 		error("sd type cannot probe");
 
 	sdev = sdifc[i]->probe(cf);
-	for(s=sdev; s; s=s->next)
+	for(s = sdev; s != nil; s = s->next)
 		s->idno = *spec;
 	sdadddevs(sdev);
 	return 0;
@@ -1609,7 +1629,6 @@
 {
 	int i;
 	SDev *sdev;
-	SDunit *unit;
 
 	if((i = sdindex(*spec)) < 0)
 		error(Enonexist);
@@ -1627,23 +1646,21 @@
 	qunlock(&devslock);
 
 	/* make sure no interrupts arrive anymore before removing resources */
-	if(sdev->enabled && sdev->ifc->disable)
-		sdev->ifc->disable(sdev);
+	if(sdev->enabled && sdev->ifc->disable != nil)
+		(*sdev->ifc->disable)(sdev);
 
 	/* free controller specific info */
-	if(sdev->ifc->clear)
-		sdev->ifc->clear(sdev);
+	if(sdev->ifc->clear != nil)
+		(*sdev->ifc->clear)(sdev);
 
-	for(i = 0; i < sdev->nunit; i++){
-		if((unit = sdev->unit[i]) != nil){
-			free(unit->name);
-			free(unit->user);
-			free(unit);
-		}
-	}
-	free(sdev->unit);
+	/* free units and their files */
+	for(i = 0; i < sdev->nunit; i++)
+		freeunit(sdev->unit[i]);
+
 	free(sdev->unitflg);
+	free(sdev->unit);
 	free(sdev);
+
 	return 0;
 }
 
@@ -1675,10 +1692,8 @@
 	if(i >= unit->nefile)
 		unit->nefile = i + 1;
 	e = unit->efile + i;
-	if(e->name == nil)
-		kstrdup(&e->name, s);
-	if(e->user == nil)
-		kstrdup(&e->user, u);
+	kstrdup(&e->name, s);
+	kstrdup(&e->user, u);
 	e->perm = perm;
 	e->r = r;
 	e->w = w;
@@ -1690,17 +1705,17 @@
 sdshutdown(void)
 {
 	int i;
-	SDev *sd;
+	SDev *sdev;
 
 	for(i = 0; i < nelem(devs); i++){
-		sd = devs[i];
-		if(sd == nil)
+		sdev = devs[i];
+		if(sdev == nil)
 			continue;
-		if(sd->ifc->disable == nil){
+		if(sdev->ifc->disable == nil){
 			print("#S/sd%c: no disable function\n", devletters[i]);
 			continue;
 		}
-		sd->ifc->disable(sd);
+		(*sdev->ifc->disable)(sdev);
 	}
 }
 
--- a/sys/src/9/port/sd.h
+++ b/sys/src/9/port/sd.h
@@ -73,7 +73,7 @@
 	int	enabled;
 	int	nunit;			/* Number of units */
 	QLock	unitlock;		/* `Loading' of units */
-	int*	unitflg;		/* Unit flags */
+	char*	unitflg;		/* Unit flags */
 	SDunit**unit;
 };