diff mbox series

Simplify alias storage.

Message ID c3903caf-06a7-e321-4fa9-5abe0a0f2ae9@gigawatt.nl (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series Simplify alias storage. | expand

Commit Message

Harald van Dijk Jan. 11, 2023, 6:23 p.m. UTC 
Rather than storing the alias name and value separately, we can reduce
simplify code and reduce code size by storing them in name=value form.
This allows us to re-use some code from var.c to handle hashing and
comparisons, so long as we update that to account for aliases' special
handling of a leading = character. This is okay to do for variables as
well, as for variables the leading character is guaranteed to not be =.
---
  src/alias.c | 32 ++++++++++----------------------
  src/var.c   | 27 +++++++++++----------------
  src/var.h   | 15 +++++++++++++++
  3 files changed, 36 insertions(+), 38 deletions(-)

Comments

Herbert Xu April 6, 2024, 7:19 a.m. UTC | #1 
On Wed, Jan 11, 2023 at 06:23:41PM +0000, Harald van Dijk wrote:
>
>  	if (ap) {
>  		if (!(ap->flag & ALIASINUSE)) {
> -			ckfree(ap->val);
> +			ckfree(ap->name);
>  		}

I think this breaks ALIASINUSE.  When an alias is marked as INUSE,
it will be freed by input.c (popstring).  However, your patch doesn't
touch input.c at all.

Thanks,
Harald van Dijk April 25, 2024, 8:46 p.m. UTC | #2 
On 06/04/2024 08:19, Herbert Xu wrote:
> On Wed, Jan 11, 2023 at 06:23:41PM +0000, Harald van Dijk wrote:
>>
>>   	if (ap) {
>>   		if (!(ap->flag & ALIASINUSE)) {
>> -			ckfree(ap->val);
>> +			ckfree(ap->name);
>>   		}
> 
> I think this breaks ALIASINUSE.  When an alias is marked as INUSE,
> it will be freed by input.c (popstring).  However, your patch doesn't
> touch input.c at all.

You're quite right, thanks for spotting that. A test case that covers 
that is

   alias foo='
   alias foo="echo Hello"
   '
   foo
   foo

Here, the storage of the original 'foo' alias needs to be kept while the 
expansion is ongoing, even when the second 'alias' overwrites the 
original definition. This bug is fixed by below additional patch.

However, the patch conflicts with c8db655b because scanning 'name' will 
now find the '=' character and continue on from that to check the value. 
That should be relatively easy to avoid, but I am not sure it is worth it.

Cheers,
Harald van Dijk

diff --git a/src/input.c b/src/input.c
index 38969a7..4604945 100644
--- a/src/input.c
+++ b/src/input.c
@@ -386,7 +386,7 @@ pushstring(char *s, void *ap)
  	sp->ap = (struct alias *)ap;
  	if (ap) {
  		((struct alias *)ap)->flag |= ALIASINUSE;
-		sp->string = s;
+		sp->string = ((struct alias *)ap)->name;
  	}
  	parsefile->nextc = s;
  	parsefile->nleft = len;
@@ -405,7 +405,7 @@ static void popstring(void)
  		    parsefile->nextc[-1] == '\t') {
  			checkkwd |= CHKALIAS;
  		}
-		if (sp->string != sp->ap->val) {
+		if (sp->string != sp->ap->name) {
  			ckfree(sp->string);
  		}
  	}
Herbert Xu April 26, 2024, 2:49 a.m. UTC | #3 
On Thu, Apr 25, 2024 at 09:46:45PM +0100, Harald van Dijk wrote:
>
> You're quite right, thanks for spotting that. A test case that covers that
> is
> 
>   alias foo='
>   alias foo="echo Hello"
>   '
>   foo
>   foo
> 
> Here, the storage of the original 'foo' alias needs to be kept while the
> expansion is ongoing, even when the second 'alias' overwrites the original
> definition. This bug is fixed by below additional patch.

Could you please post a combined patch? Thanks!

> However, the patch conflicts with c8db655b because scanning 'name' will now
> find the '=' character and continue on from that to check the value. That
> should be relatively easy to avoid, but I am not sure it is worth it.

Don't worry about that one.  I can back it out if necessary.
In fact, I'll probably fix it in a different way as I'm adding
multi-byte support.

Cheers,
diff mbox series

Patch

diff --git a/src/alias.c b/src/alias.c
index fcad43b..bb963a6 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -41,6 +41,7 @@ 
  #include "mystring.h"
  #include "alias.h"
  #include "options.h"	/* XXX for argptr (should remove?) */
+#include "var.h"
   #define ATABSIZE 39
  @@ -55,25 +56,26 @@ void
  setalias(const char *name, const char *val)
  {
  	struct alias *ap, **app;
+	size_t namelen;
   	app = __lookupalias(name);
  	ap = *app;
  	INTOFF;
  	if (ap) {
  		if (!(ap->flag & ALIASINUSE)) {
-			ckfree(ap->val);
+			ckfree(ap->name);
  		}
-		ap->val	= savestr(val);
  		ap->flag &= ~ALIASDEAD;
  	} else {
  		/* not found */
  		ap = ckmalloc(sizeof (struct alias));
-		ap->name = savestr(name);
-		ap->val = savestr(val);
  		ap->flag = 0;
  		ap->next = 0;
  		*app = ap;
  	}
+	namelen = val - name;
+	ap->name = savestr(name);
+	ap->val = ap->name + namelen;
  	INTON;
  }
  @@ -150,8 +152,7 @@ aliascmd(int argc, char **argv)
  			} else
  				printalias(ap);
  		} else {
-			*v++ = '\0';
-			setalias(n, v);
+			setalias(n, v + 1);
  		}
  	}
  @@ -190,36 +191,23 @@ freealias(struct alias *ap) {
   	next = ap->next;
  	ckfree(ap->name);
-	ckfree(ap->val);
  	ckfree(ap);
  	return next;
  }
   void
  printalias(const struct alias *ap) {
-	out1str(single_quote(ap->name));
-	out1fmt("=%s\n", single_quote(ap->val));
+	out1fmt("%s\n", single_quote(ap->name));
  }
   STATIC struct alias **
  __lookupalias(const char *name) {
-	unsigned int hashval;
  	struct alias **app;
-	const char *p;
-	unsigned int ch;
  -	p = name;
-
-	ch = (unsigned char)*p;
-	hashval = ch << 4;
-	while (ch) {
-		hashval += ch;
-		ch = (unsigned char)*++p;
-	}
-	app = &atab[hashval % ATABSIZE];
+	app = &atab[hashval(name) % ATABSIZE];
   	for (; *app; app = &(*app)->next) {
-		if (equal(name, (*app)->name)) {
+		if (varequal(name, (*app)->name)) {
  			break;
  		}
  	}
diff --git a/src/var.c b/src/var.c
index b70d72c..64d93aa 100644
--- a/src/var.c
+++ b/src/var.c
@@ -625,12 +625,7 @@  void unsetvar(const char *s)
  STATIC struct var **
  hashvar(const char *p)
  {
-	unsigned int hashval;
-
-	hashval = ((unsigned char) *p) << 4;
-	while (*p && *p != '=')
-		hashval += (unsigned char) *p++;
-	return &vartab[hashval % VTABSIZE];
+	return &vartab[hashval(p) % VTABSIZE];
  }
   @@ -644,19 +639,19 @@ hashvar(const char *p)
  int
  varcmp(const char *p, const char *q)
  {
-	int c, d;
-
-	while ((c = *p) == (d = *q)) {
-		if (!c || c == '=')
-			goto out;
+	int c = *p, d = *q;
+	while (c == d) {
+		if (!c)
+			break;
  		p++;
  		q++;
+		c = *p;
+		d = *q;
+		if (c == '=')
+			c = '\0';
+		if (d == '=')
+			d = '\0';
  	}
-	if (c == '=')
-		c = 0;
-	if (d == '=')
-		d = 0;
-out:
  	return c - d;
  }
  diff --git a/src/var.h b/src/var.h
index aa7575a..2aece9d 100644
--- a/src/var.h
+++ b/src/var.h
@@ -153,6 +153,21 @@  int unsetcmd(int, char **);
  void unsetvar(const char *);
  int varcmp(const char *, const char *);
  +static inline unsigned int hashval(const char *p)
+{
+	unsigned int hashval;
+
+	hashval = ((unsigned char) *p) << 4;
+	while (*p) {
+		hashval += (unsigned char) *p++;
+		if (*p == '=')
+			break;
+	}
+
+	return hashval;
+}
+
+
  static inline int varequal(const char *a, const char *b) {
  	return !varcmp(a, b);
  }
-- 
2.39.0