uuidd cleanups and clarifications #1196

kerolasa · 2020-11-21T22:04:36Z

As mentioned in #1186 (comment) the uuidd changes were needed in smaller chunks that are easier to review. So here we are, and as I started to look uuidd more there was more small things that could be done. So the change set grew since the last time. As a whole this is still pretty large change, but when viewed each commit one at a time they hopefully make sense.

karelzak · 2020-11-23T09:41:31Z

misc-utils/uuidd.c

@@ -662,14 +673,14 @@ int main(int argc, char **argv)
 		char buf[1024];
 		char str[UUID_STR_LEN];

-		ret = call_daemon(uuidd_opts.socket_path, uuidd_opts.do_type + 2, buf,
+		ret = call_daemon(uuidd_opts.socket_path, uuidd_opts.do_type, buf,


Thanks, it's really dirty to rely on +2 (and without any somment in uuidd.h).

karelzak · 2020-11-23T09:52:02Z

misc-utils/uuidd.c

 			memcpy(reply_buf + reply_len, &num, sizeof(num));
-			reply_len += sizeof(num);


This does not seem correct. The second memcpy() uses old offset from previous daemon call (or 0) to write to reply_buf.

You are absolutely right. Fixed in f84b3cd.

karelzak · 2020-11-23T09:59:17Z

misc-utils/uuidd.c

 			__uuid_generate_random((unsigned char *) reply_buf +
 					      sizeof(num), &num);
+			reply_len = sizeof(num) + (UUID_LEN * num);


It does not seem more readable and robust. It seems (for reader) __uuid_generate_random() can modify num and it seems better to use it in memcpy() after __uuid_generate_random().

The memcpy() is after __uuid_generate_random() in ea94169 but before debug loop. I think it is better to write the number first to output buffer before payload that follows so that writes and data are in same order.

karelzak · 2020-11-23T10:02:41Z

misc-utils/uuidd.c

-				     i++, cp += UUID_LEN) {
+
+				cp = reply_buf + sizeof(num)
+				for (i = 0; i < num; i++) {
 					uuid_unparse((unsigned char *)cp, str);


This is only part of the patch which makes the code more readable. It would be bettrer to remove the rest.

karelzak · 2020-11-23T10:17:09Z

misc-utils/uuidd.c

+	UUID_NUM_LEN = sizeof(int),		/* number of requested uuids */
+	UUID_DATA_LEN = sizeof(uuid_t),		/* binary representation of UUID */
+	UUID_BUF_LEN = 1024			/* client - server buffer size */
+};


I don't think use UUID_ prefix is good idea in this case, it seems like someting releated to the uuid_t (or so).
What about:

UUIDD_PROT_OPER_LEN UUIDD_PROT_NUM_LEN UUIDD_PROT_DATA_LEN UUIDD_PROT_BUFSZ

Maybe to be sure about sizes

typedef int32_t uuidd_prot_num_t typedef uint_t8 uuidd_prot_op_t

and in code use

uuidd_prot_num_t num; ... memcpy(buf, num, sizeof(num));

and _OPER_LEN and _NUM_LEN will be almost unnecessary.

Really good proposal. Thank you @karelzak The change is close to completely rewrote, see b491161.

karelzak · 2020-11-23T10:22:06Z

misc-utils/uuidd.c

-		memcpy(op_buf + 1, num, sizeof(int));
-		op_len += sizeof(int);
+		memcpy(op_buf + UUID_OP_LEN, num, UUID_NUM_LEN);
+		op_len += UUID_NUM_LEN;
 	}


IMHO it's always better and robust to follow variables when use sizeof() for memcpy().

memcpy(op_buf + UUID_OP_LEN, num, sizeof(num)); op_len += sizeof(num);

karelzak · 2020-11-23T10:24:22Z

misc-utils/uuidd.c

-		if (reply_len >= (int) (UUID_LEN + sizeof(int)))
-			memcpy(buf + UUID_LEN, num, sizeof(int));
+		if ((UUID_DATA_LEN + UUID_NUM_LEN) <= reply_len)
+			memcpy(buf + UUID_DATA_LEN, num, UUID_NUM_LEN);


if ((UUID_DATA_LEN + sizeof(num)) <= reply_len) memcpy(buf + UUID_DATA_LEN, num, sizeof(num));

karelzak · 2020-11-23T10:25:00Z

misc-utils/uuidd.c

-		if (reply_len >= (int) sizeof(int))
-			memcpy(buf, num, sizeof(int));
+		if (UUID_NUM_LEN <= reply_len)
+			memcpy(buf, num, UUID_NUM_LEN);
 		else


and again sizeof(num)

karelzak · 2020-11-23T10:32:57Z

misc-utils/uuidd.c

@@ -527,8 +527,6 @@ static void server_loop(const char *socket_path, const char *pidfile_path,
 		case UUIDD_OP_BULK_RANDOM_UUID:
 			if (num < 0)
 				num = 1;
-			if (num > 1000)
-				num = 1000;


Good catch. Maybe the buffer size should be defined as number of uuids

#define UUIDD_PROT_BUFSZ (63 * sizeof(uuid_t))

or so.

karelzak · 2020-11-23T10:34:47Z

misc-utils/uuidd.c

@@ -448,7 +448,7 @@ static void server_loop(const char *socket_path, const char *pidfile_path,
 		}
 		if (ret == 0) {		/* true when poll() times out */
 			if (uuidd_cxt->debug)
-				fprintf(stderr, _("timeout [%d sec]\n"), uuidd_cxt->timeout),
+				fprintf(stderr, _("timeout [%d sec]\n"), uuidd_cxt->timeout);


Oh, so stupid typo. Thanks!

Earlier use of a variable that holds switch enabling boolean to hold process id was a little bit strange, and not exactly correct. An int should be good enough to hold any pid, but it is better to be precise and use the type that is meant for the job. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

The struct will help seeing where the variable values are coming from. This will also help upcoming refactoring. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Moving the option parsing to a separate function will allow moving some variables from main() heap to be in scope that free them later. That should make the uuidd to have a little bit smaller runtime memory allocation. The static long options is changed to be local variable. That should make it to be part of heap for a bit, until removed. Earlier the variable was in data segment and permanently in runtime memory. Whether this makes any impact either way is not entirely clear, but hope is the runtime memory allocation is tiny bit smaller. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Relying to single vs bulk requests to be exactly two steps away from each other was an unnecessary dirty trick. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Seeing what can be expected should make it easier to understand what the code does. Notice that simply writing down the resposes one can start wondering deeper questions, such as, why does the time bulk response reply only one uuid followed by number how many were requested? Was that a some type of TODO note? Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Use a typedef for types that are sensitive what comes to uuidd protocol field widths, read write sizes, and related checks. Proposed-by: Karel Zak <kzak@redhat.com> Reference: util-linux#1196 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Write the data and response length values in same close to order they appear in protocol. This should make code a little easier to read. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Use a typedef for types that are sensitive what comes to uuidd protocol field widths, read write sizes, and related checks. Proposed-by: Karel Zak <kzak@redhat.com> Reference: util-linux#1196 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Limiting random query to 1000 uuid's was pointless when the next line will further restrict upper limit to 63 entries. The 63 entries is what fits to the uuidd communication buffer with the header. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

The trailing comma made code to compile without issues, but meant all_done() was called only when --debug was in defined in command-line. Fixes: 3d6250e Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

karelzak · 2020-11-24T12:13:41Z

It's cool and easy to review now. Thanks!

karelzak reviewed Nov 23, 2020

View reviewed changes

kerolasa added 5 commits November 23, 2020 21:34

uuidd: add command-line option values struct

b03ffec

The struct will help seeing where the variable values are coming from. This will also help upcoming refactoring. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

uuidd: override operation type when performing bulk request

77a5465

Relying to single vs bulk requests to be exactly two steps away from each other was an unnecessary dirty trick. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa force-pushed the uuidd-work branch from ccaab01 to e497adc Compare November 23, 2020 22:43

kerolasa added 5 commits November 23, 2020 22:47

uuidd: reorder bulk time and random generation code

ea94169

Write the data and response length values in same close to order they appear in protocol. This should make code a little easier to read. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

uuidd: make timeout to take effect when debug is not defined

f187035

The trailing comma made code to compile without issues, but meant all_done() was called only when --debug was in defined in command-line. Fixes: 3d6250e Signed-off-by: Sami Kerola <kerolasa@iki.fi>

uuidd: fix misleading indentation

ef6ac59

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa force-pushed the uuidd-work branch from e497adc to ef6ac59 Compare November 23, 2020 22:49

karelzak merged commit 665d79e into util-linux:master Nov 24, 2020

kerolasa deleted the uuidd-work branch November 24, 2020 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uuidd cleanups and clarifications #1196

uuidd cleanups and clarifications #1196

kerolasa commented Nov 21, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020 •

edited

Loading

kerolasa Nov 23, 2020

karelzak Nov 23, 2020

kerolasa Nov 23, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020

kerolasa Nov 23, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020

karelzak Nov 23, 2020

karelzak commented Nov 24, 2020

		memcpy(reply_buf + reply_len, &num, sizeof(num));
		reply_len += sizeof(num);

uuidd cleanups and clarifications #1196

uuidd cleanups and clarifications #1196

Conversation

kerolasa commented Nov 21, 2020

Choose a reason for hiding this comment

karelzak Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelzak commented Nov 24, 2020

karelzak Nov 23, 2020 •

edited

Loading