Index | Thread | Search

From:
Dante Catalfamo <dante@lambda.cx>
Subject:
Re: mg: prevet crash with invalid compile buffer
To:
Omar Polo <op@omarpolo.com>
Cc:
tech@openbsd.org
Date:
Wed, 22 Jan 2025 21:51:00 -0500

Download raw body.

Thread
Totally forgot about this.

Here's the same diff, but with a break in the loop instead of a goto.

diff --git a/usr.bin/mg/buffer.c b/usr.bin/mg/buffer.c
index 8c8aab40a..628d485b4 100644
--- a/usr.bin/mg/buffer.c
+++ b/usr.bin/mg/buffer.c
@@ -1091,3 +1091,18 @@ findbuffer(char *fn)
  	bp = bfind(bname, TRUE);
  	return (bp);
  }
+
+/*
+ * Checks if a buffer has a given mode as one of its major or minor
+ * modes.
+ */
+int
+checkbuffermode(struct buffer *bp, char *mode)
+{
+	int i;
+
+	for (i = 0; i <= bp->b_nmodes; ++i)
+		if (!strcmp(bp->b_modes[i]->p_name, mode))
+			return (TRUE);
+	return (FALSE);
+}
diff --git a/usr.bin/mg/def.h b/usr.bin/mg/def.h
index 403635036..56a39c6d3 100644
--- a/usr.bin/mg/def.h
+++ b/usr.bin/mg/def.h
@@ -441,6 +441,7 @@ int		 killbuffer_cmd(int, int);
  int		 savebuffers(int, int);
  int		 listbuffers(int, int);
  int		 addlinef(struct buffer *, char *, ...);
+int		 checkbuffermode(struct buffer *, char *);
  #define	 addline(bp, text)	addlinef(bp, "%s", text)
  int		 anycb(int);
  int		 bclear(struct buffer *);
diff --git a/usr.bin/mg/grep.c b/usr.bin/mg/grep.c
index aa7f9dfd8..6c8b56fc1 100644
--- a/usr.bin/mg/grep.c
+++ b/usr.bin/mg/grep.c
@@ -29,14 +29,6 @@ void grep_init(void);

  static char compile_last_command[NFILEN] = "make ";

-/*
- * Hints for next-error
- *
- * XXX - need some kind of callback to find out when those get killed.
- */
-struct mgwin	*compile_win;
-struct buffer	*compile_buffer;
-
  static PF compile_pf[] = {
  	compile_goto_error
  };
@@ -82,7 +74,7 @@ grep(int f, int n)
  	if ((wp = popbuf(bp, WNONE)) == NULL)
  		return (FALSE);
  	curbp = bp;
-	compile_win = curwp = wp;
+	curwp = wp;
  	return (TRUE);
  }

@@ -108,7 +100,7 @@ compile(int f, int n)
  	if ((wp = popbuf(bp, WNONE)) == NULL)
  		return (FALSE);
  	curbp = bp;
-	compile_win = curwp = wp;
+	curwp = wp;
  	gotoline(FFARG, 0);
  	return (TRUE);
  }
@@ -166,7 +158,7 @@ gid(int f, int n)
  	if ((wp = popbuf(bp, WNONE)) == NULL)
  		return (FALSE);
  	curbp = bp;
-	compile_win = curwp = wp;
+	curwp = wp;
  	return (TRUE);
  }

@@ -240,8 +232,6 @@ compile_mode(const char *name, const char *command)
  	bp->b_modes[1] = name_mode("compile");
  	bp->b_nmodes = 1;

-	compile_buffer = bp;
-
  	if (chdir(cwd) == -1) {
  		dobeep();
  		ewprintf("Can't change dir back to %s", cwd);
@@ -261,9 +251,7 @@ compile_goto_error(int f, int n)
  	const char *errstr;
  	struct line	*last;

-	compile_win = curwp;
-	compile_buffer = curbp;
-	last = blastlp(compile_buffer);
+	last = blastlp(curbp);

   retry:
  	/* last line is compilation result */
@@ -317,16 +305,46 @@ fail:
  	return (FALSE);
  }

+static int
+show_compile_buffer(void)
+{
+	struct mgwin	*wp;
+	struct buffer	*bp;
+
+	if (checkbuffermode(curbp, "compile")) {
+		return (TRUE);
+	}
+
+	for (bp = bheadp; bp != NULL; bp = bp->b_bufp) {
+		if (checkbuffermode(bp, "compile"))
+			break;
+	}
+
+	if (bp == NULL)
+		return (FALSE);
+
+	for (wp = wheadp; wp != NULL; wp = wp->w_wndp) {
+		if (wp->w_bufp == bp) {
+			curwp = wp;
+			return (TRUE);
+		}
+	}
+
+	if (splitwind(0, 1) == FALSE ||
+	    showbuffer(bp, curwp, 0) == FALSE)
+		return (FALSE);
+
+	return (TRUE);
+}
+
  int
  next_error(int f, int n)
  {
-	if (compile_win == NULL || compile_buffer == NULL) {
+	if (show_compile_buffer() == FALSE) {
  		dobeep();
  		ewprintf("No compilation active");
  		return (FALSE);
  	}
-	curwp = compile_win;
-	curbp = compile_buffer;
  	if (curwp->w_dotp == blastlp(curbp)) {
  		dobeep();
  		ewprintf("No more hits");


On 8/1/24 6:09 PM, Omar Polo wrote:
> On 2024/08/01 15:19:23 -0400, "Dante Catalfamo" <dante@lambda.cx> wrote:
>> Found the issuse. If the window is already displaying but not focused,
>> we return true instead of making it the current window, so when
>> next-error checks if we're at the bottom of the buffer, it's looking at
>> whatever we're focused on.
> 
> oh right, good catch!  I forgot to test that case.
> 
> more below.
> 
>> [...]
>> +static int
>> +show_compile_buffer(void)
>> +{
>> +	struct mgwin	*wp;
>> +	struct buffer	*bp;
>> +
>> +	if (checkbuffermode(curbp, "compile") == TRUE) {
>> +		return (TRUE);
>> +	}
>> +
>> +	for (bp = bheadp; bp != NULL; bp = bp->b_bufp) {
>> +		if (checkbuffermode(bp, "compile") == TRUE)
>> +			goto done;
> 
> since now there is checkbuffermode() (which i think it's a good thing!),
> this goto can become just a break.
> 
>> +	}
>> + done:
>> [...]
> 
>> On Thu, Aug 1, 2024, at 2:43 PM, Dante Catalfamo wrote:
>>> I think this approach makes sense, although there can be two compile
>>> buffers, *grep* and *compile*.
> 
> You're right, here we pick the first buffer found while before we used
> the last one created by M-x grep or compile.  I came up with this
> because i don't like how we keep a pointer to a buffer that can be
> killed at any moment.
> 
> as an alternative we could register a callback and make killbuffer()
> invoke it so that grep.c can manage its state better.