Re: [PATCHv3 iproute2 1/2] lib/libnetlink: re malloc buff if size is not enough

Hi Stephen,
On Wed, Sep 20, 2017 at 09:56:05AM -0700, Stephen Hemminger wrote:

quoted

+realloc:
+	bufp = realloc(buf, buf_len);
+
+	if (bufp == NULL) {

Minor personal style issue:
To me, blank lines are like paragraphs in writing.
Code reads better assignment and condition check are next to
each other.

OK, I will remove the blank lines.

quoted

+recv:
+	len = recvmsg(fd, msg, flag);
+
+	if (len < 0) {
+		if (errno == EINTR || errno == EAGAIN)
+			goto recv;
+		fprintf(stderr, "netlink receive error %s (%d)\n",
+			strerror(errno), errno);
+		free(buf);
+		return -errno;
+	}
+
+	if (len == 0) {
+		fprintf(stderr, "EOF on netlink\n");
+		free(buf);
+		return -ENODATA;
+	}
+
+	if (len > buf_len) {
+		buf_len = len;
+		flag = 0;
+		goto realloc;
+	}
+
+	if (flag != 0) {
+		flag = 0;
+		goto recv;

Although I programmed in BASIC years ago. I never liked code
with loops via goto. To me it indicates the logic is not well thought
through.  Not sure exactly how to rearrange the control flow, but it
should be possible to rewrite this so that it reads cleaner.

Hmm, if we remove goto. Then the logic should look like

	bufp = realloc(buf, buf_len);
	/* check bufp and set msg */

	len = recvmsg(fd, msg, flag);
	/* check len */

	if (len > buf_len) {
		buf_len = len;
		bufp = realloc(buf, buf_len);
		/* check bufp and set msg */

		len = recvmsg(fd, msg, flag);
		/* check len */
	}

	len = recvmsg(fd, msg, flag);
	/* check len */

Or maybe we can set buf_len very small first. Then it will force to realloc at
the second time. And the code would like

	int buf_len = 16;
	bufp = realloc(buf, buf_len);
	/* check bufp and set msg */

	len = recvmsg(fd, msg, flag);
	/* check len */

	buf_len = len;
	bufp = realloc(buf, buf_len);
	/* check bufp and set msg */

	len = recvmsg(fd, msg, flag);
	/* check len */

What do you think?

Thanks
Hangbin