The generated code had: ... done_no_free: return; } There was no possible code between the label and the return statement, so both the label and the return statement are redundant. The instances of ‘goto done_no_free’ can simply be replaced by ‘return’. The only small complication is that there is a label just before this which contains some optional code. So we might have ended up with generated code looking like: done: // if there was no optional code, this would be empty } which provokes a parse error in C. Therefore I had to add a semicolon after the ‘done:’ label. This will be removed in a subsequent commit, so it's just to make the current commit bisectable. --- generator/daemon.ml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/generator/daemon.ml b/generator/daemon.ml index 9453d1256..83c0ad24c 100644 --- a/generator/daemon.ml +++ b/generator/daemon.ml @@ -284,7 +284,7 @@ let generate_daemon_stubs actions () = if is_filein then pr " cancel_receive ();\n"; pr " reply_with_unavailable_feature (\"%s\");\n" group; - pr " goto done_no_free;\n"; + pr " return;\n"; pr " }\n"; pr "\n" | None -> () @@ -302,14 +302,14 @@ let generate_daemon_stubs actions () = if is_filein then pr " cancel_receive ();\n"; pr " reply_with_error (\"unknown option in optional arguments bitmask (this can happen if a program is compiled against a newer version of libguestfs, then run against an older version of the daemon)\");\n"; - pr " goto done_no_free;\n"; + pr " return;\n"; pr " }\n"; ) else ( pr " if (optargs_bitmask != 0) {\n"; if is_filein then pr " cancel_receive ();\n"; pr " reply_with_error (\"header optargs_bitmask field must be passed as 0 for calls that don't take optional arguments\");\n"; - pr " goto done_no_free;\n"; + pr " return;\n"; pr " }\n"; ); pr "\n"; @@ -499,15 +499,13 @@ let generate_daemon_stubs actions () = ); (* Free the args. *) - pr "done:\n"; + pr "done: ;\n"; (match args_passed_to_daemon with | [] -> () | _ -> pr " xdr_free ((xdrproc_t) xdr_guestfs_%s_args, (char *) &args);\n" name ); - pr "done_no_free:\n"; - pr " return;\n"; pr "}\n\n"; ) (actions |> daemon_functions |> sort) -- 2.12.0

As a simple consequence of the previous commit, the ‘done’ label can be removed, and any instance of ‘goto done’ can be replaced by ‘return’. --- generator/daemon.ml | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/generator/daemon.ml b/generator/daemon.ml index 535f99458..a469831c1 100644 --- a/generator/daemon.ml +++ b/generator/daemon.ml @@ -345,7 +345,7 @@ let generate_daemon_stubs actions () = if is_filein then pr " cancel_receive ();\n"; pr " reply_with_error (\"daemon failed to decode procedure arguments\");\n"; - pr " goto done;\n"; + pr " return;\n"; pr " }\n"; let pr_args n = pr " %s = args.%s;\n" n n @@ -354,19 +354,19 @@ let generate_daemon_stubs actions () = function | Pathname n -> pr_args n; - pr " ABS_PATH (%s, %s, goto done);\n" + pr " ABS_PATH (%s, %s, return);\n" n (if is_filein then "cancel_receive ()" else ""); | Device n -> - pr " RESOLVE_DEVICE (args.%s, %s, %s, goto done);\n" + pr " RESOLVE_DEVICE (args.%s, %s, %s, return);\n" n n (if is_filein then "cancel_receive ()" else ""); | Mountable n -> - pr " RESOLVE_MOUNTABLE (args.%s, %s, %s, goto done);\n" + pr " RESOLVE_MOUNTABLE (args.%s, %s, %s, return);\n" n n (if is_filein then "cancel_receive ()" else ""); | Dev_or_Path n -> - pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (args.%s, %s, %s, goto done);\n" + pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (args.%s, %s, %s, return);\n" n n (if is_filein then "cancel_receive ()" else ""); | Mountable_or_Path n -> - pr " REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE (args.%s, %s, %s, goto done);\n" + pr " REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE (args.%s, %s, %s, return);\n" n n (if is_filein then "cancel_receive ()" else ""); | String n | Key n | GUID n -> pr_args n | OptString n -> pr " %s = args.%s ? *args.%s : NULL;\n" n n n @@ -381,7 +381,7 @@ let generate_daemon_stubs actions () = pr " cancel_receive ();\n"; pr " reply_with_error (\"%%s: '%%s' is not a file name\", __func__, args.%s.%s_val[i]);\n" n n; - pr " goto done;\n"; + pr " return;\n"; pr " }\n"; pr " }\n"; pr " }\n" @@ -394,7 +394,7 @@ let generate_daemon_stubs actions () = if is_filein then pr " cancel_receive ();\n"; pr " reply_with_perror (\"realloc\");\n"; - pr " goto done;\n"; + pr " return;\n"; pr " }\n"; pr " %s[args.%s.%s_len] = NULL;\n" n n n; pr " args.%s.%s_val = %s;\n" n n n @@ -407,7 +407,7 @@ let generate_daemon_stubs actions () = pr " size_t i;\n"; pr " for (i = 0; i < args.%s.%s_len; ++i)\n" n n; pr " RESOLVE_DEVICE (args.%s.%s_val[i], %s[i],\n" n n n; - pr " %s, goto done);\n" + pr " %s, return);\n" (if is_filein then "cancel_receive ()" else ""); pr " %s[i] = NULL;\n" n; pr " }\n" @@ -426,7 +426,7 @@ let generate_daemon_stubs actions () = if List.exists (function Pathname _ -> true | _ -> false) args then ( (* Emit NEED_ROOT just once, even when there are two or more Pathname args *) - pr " NEED_ROOT (%s, goto done);\n" + pr " NEED_ROOT (%s, return);\n" (if is_filein then "cancel_receive ()" else ""); ); @@ -454,7 +454,7 @@ let generate_daemon_stubs actions () = | (`ErrorIsMinusOne | `ErrorIsNULL) as e -> e in pr " if (r == %s)\n" (string_of_errcode errcode); pr " /* do_%s has already called reply_with_error */\n" name; - pr " goto done;\n"; + pr " return;\n"; pr "\n" | RBufferOut _ -> pr " /* size == 0 && r == NULL could be a non-error case (just\n"; @@ -462,7 +462,7 @@ let generate_daemon_stubs actions () = pr " */\n"; pr " if (size == 1 && r == NULL)\n"; pr " /* do_%s has already called reply_with_error */\n" name; - pr " goto done;\n"; + pr " return;\n"; pr "\n" ); @@ -520,8 +520,6 @@ let generate_daemon_stubs actions () = name; pr " free (r);\n" ); - - pr "done: ;\n"; pr "}\n\n"; ) (actions |> daemon_functions |> sort) -- 2.12.0

In every instance where we used the ‘cancel_stmt’ parameter of these macros: ABS_PATH NEED_ROOT the value was only ever ‘cancel_receive ()’ or empty. We only use ‘cancel_receive’ for FileIn functions, so replace it with a simple flag for whether the current function is a FileIn function. --- daemon/9p.c | 2 +- daemon/daemon.h | 16 ++++++-------- daemon/df.c | 4 ++-- daemon/hivex.c | 4 ++-- daemon/inotify.c | 2 +- daemon/mount.c | 10 ++++----- daemon/sh.c | 2 +- generator/daemon.ml | 60 +++++++++++++++++++++++++---------------------------- 8 files changed, 46 insertions(+), 54 deletions(-) diff --git a/daemon/9p.c b/daemon/9p.c index bd564a5be..fc5b01736 100644 --- a/daemon/9p.c +++ b/daemon/9p.c @@ -180,7 +180,7 @@ do_mount_9p (const char *mount_tag, const char *mountpoint, const char *options) struct stat statbuf; int r; - ABS_PATH (mountpoint, , return -1); + ABS_PATH (mountpoint, 0, return -1); mp = sysroot_path (mountpoint); if (!mp) { diff --git a/daemon/daemon.h b/daemon/daemon.h index a317139a5..5137e2c2a 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -327,26 +327,22 @@ extern void pulse_mode_cancel (void); */ extern void notify_progress_no_ratelimit (uint64_t position, uint64_t total, const struct timeval *now); -/* Helper for functions that need a root filesystem mounted. - * NB. Cannot be used for FileIn functions. - */ -#define NEED_ROOT(cancel_stmt,fail_stmt) \ +/* Helper for functions that need a root filesystem mounted. */ +#define NEED_ROOT(is_filein,fail_stmt) \ do { \ if (!is_root_mounted ()) { \ - cancel_stmt; \ + if (is_filein) cancel_receive (); \ reply_with_error ("%s: you must call 'mount' first to mount the root filesystem", __func__); \ fail_stmt; \ } \ } \ while (0) -/* Helper for functions that need an argument ("path") that is absolute. - * NB. Cannot be used for FileIn functions. - */ -#define ABS_PATH(path,cancel_stmt,fail_stmt) \ +/* Helper for functions that need an argument ("path") that is absolute. */ +#define ABS_PATH(path,is_filein,fail_stmt) \ do { \ if ((path)[0] != '/') { \ - cancel_stmt; \ + if (is_filein) cancel_receive (); \ reply_with_error ("%s: path must start with a / character", __func__); \ fail_stmt; \ } \ diff --git a/daemon/df.c b/daemon/df.c index f17708453..80b765f30 100644 --- a/daemon/df.c +++ b/daemon/df.c @@ -36,7 +36,7 @@ do_df (void) char *out; CLEANUP_FREE char *err = NULL; - NEED_ROOT (, return NULL); + NEED_ROOT (0, return NULL); r = command (&out, &err, str_df, NULL); if (r == -1) { @@ -55,7 +55,7 @@ do_df_h (void) char *out; CLEANUP_FREE char *err = NULL; - NEED_ROOT (, return NULL); + NEED_ROOT (0, return NULL); r = command (&out, &err, str_df, "-h", NULL); if (r == -1) { diff --git a/daemon/hivex.c b/daemon/hivex.c index 95ed7dd28..1cbfb3458 100644 --- a/daemon/hivex.c +++ b/daemon/hivex.c @@ -349,8 +349,8 @@ do_hivex_commit (const char *filename) /* There is no "OptPathname" in the generator, so we have * to do the pathname checks explicitly here. RHBZ#981683 */ - ABS_PATH (filename, , return -1); - NEED_ROOT (, return -1); + ABS_PATH (filename, 0, return -1); + NEED_ROOT (0, return -1); buf = sysroot_path (filename); if (!buf) { diff --git a/daemon/inotify.c b/daemon/inotify.c index 38a1c92fb..b9bfed713 100644 --- a/daemon/inotify.c +++ b/daemon/inotify.c @@ -82,7 +82,7 @@ do_inotify_init (int max_events) { FILE *fp; - NEED_ROOT (, return -1); + NEED_ROOT (0, return -1); if (max_events < 0) { reply_with_error ("max_events < 0"); diff --git a/daemon/mount.c b/daemon/mount.c index f2aedfd11..0ad9626a7 100644 --- a/daemon/mount.c +++ b/daemon/mount.c @@ -127,7 +127,7 @@ do_mount_vfs (const char *options, const char *vfstype, CLEANUP_FREE char *mp = NULL; struct stat statbuf; - ABS_PATH (mountpoint, , return -1); + ABS_PATH (mountpoint, 0, return -1); mp = sysroot_path (mountpoint); if (!mp) { @@ -498,8 +498,8 @@ do_mkmountpoint (const char *path) { int r; - /* NEED_ROOT (return -1); - we don't want this test for this call. */ - ABS_PATH (path, , return -1); + /* NEED_ROOT (0, return -1); - we don't want this test for this call. */ + ABS_PATH (path, 0, return -1); CHROOT_IN; r = mkdir (path, 0777); @@ -518,8 +518,8 @@ do_rmmountpoint (const char *path) { int r; - /* NEED_ROOT (return -1); - we don't want this test for this call. */ - ABS_PATH (path, , return -1); + /* NEED_ROOT (0, return -1); - we don't want this test for this call. */ + ABS_PATH (path, 0, return -1); CHROOT_IN; r = rmdir (path); diff --git a/daemon/sh.c b/daemon/sh.c index 94f348b31..baebd3960 100644 --- a/daemon/sh.c +++ b/daemon/sh.c @@ -251,7 +251,7 @@ do_command (char *const *argv) { .mounted = false }; /* We need a root filesystem mounted to do this. */ - NEED_ROOT (, return NULL); + NEED_ROOT (0, return NULL); /* Conveniently, argv is already a NULL-terminated argv-style array * of parameters, so we can pass it straight in to our internal diff --git a/generator/daemon.ml b/generator/daemon.ml index 2816cb847..3f30e0940 100644 --- a/generator/daemon.ml +++ b/generator/daemon.ml @@ -91,22 +91,22 @@ let generate_daemon_stubs_h () = * * NB. Cannot be used for FileIn functions. */ -#define RESOLVE_DEVICE(path,path_out,cancel_stmt) \\ +#define RESOLVE_DEVICE(path,path_out,is_filein) \\ do { \\ if (STRNEQLEN ((path), \"/dev/\", 5)) { \\ - cancel_stmt; \\ + if (is_filein) cancel_receive (); \\ reply_with_error (\"%%s: %%s: expecting a device name\", __func__, (path)); \\ return; \\ } \\ if (is_root_device (path)) { \\ - cancel_stmt; \\ + if (is_filein) cancel_receive (); \\ reply_with_error (\"%%s: %%s: device not found\", __func__, path); \\ return; \\ } \\ (path_out) = device_name_translation ((path)); \\ if ((path_out) == NULL) { \\ const int err = errno; \\ - cancel_stmt; \\ + if (is_filein) cancel_receive (); \\ errno = err; \\ reply_with_perror (\"%%s: %%s\", __func__, path); \\ return; \\ @@ -120,12 +120,12 @@ let generate_daemon_stubs_h () = * * Note that the \"string\" argument may be modified. */ -#define RESOLVE_MOUNTABLE(string,mountable,cancel_stmt) \\ +#define RESOLVE_MOUNTABLE(string,mountable,is_filein) \\ do { \\ if (STRPREFIX ((string), \"btrfsvol:\")) { \\ if (parse_btrfsvol ((string) + strlen (\"btrfsvol:\"), &(mountable)) == -1)\\ { \\ - cancel_stmt; \\ + if (is_filein) cancel_receive (); \\ reply_with_error (\"%%s: %%s: expecting a btrfs volume\", \\ __func__, (string)); \\ return; \\ @@ -135,7 +135,7 @@ let generate_daemon_stubs_h () = (mountable).type = MOUNTABLE_DEVICE; \\ (mountable).device = NULL; \\ (mountable).volume = NULL; \\ - RESOLVE_DEVICE ((string), (mountable).device, cancel_stmt); \\ + RESOLVE_DEVICE ((string), (mountable).device, (is_filein)); \\ } \\ } while (0) @@ -149,16 +149,16 @@ let generate_daemon_stubs_h () = * because we intend in future to make device parameters a distinct * type from filenames. */ -#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,path_out,cancel_stmt) \\ +#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,path_out,is_filein) \\ do { \\ if (STREQLEN ((path), \"/dev/\", 5)) \\ - RESOLVE_DEVICE ((path), (path_out), cancel_stmt); \\ + RESOLVE_DEVICE ((path), (path_out), (is_filein)); \\ else { \\ - NEED_ROOT (cancel_stmt, return); \\ - ABS_PATH ((path), cancel_stmt, return); \\ + NEED_ROOT ((is_filein), return); \\ + ABS_PATH ((path), (is_filein), return); \\ (path_out) = strdup ((path)); \\ if ((path_out) == NULL) { \\ - cancel_stmt; \\ + if (is_filein) cancel_receive (); \\ reply_with_perror (\"strdup\"); \\ return; \\ } \\ @@ -168,19 +168,19 @@ let generate_daemon_stubs_h () = /* Helper for functions which need either an absolute path in the * mounted filesystem, OR a valid mountable description. */ -#define REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE(string, mountable, cancel_stmt) \\ +#define REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE(string, mountable, is_filein) \\ do { \\ - if (STRPREFIX ((string), \"/dev/\") || (string)[0] != '/') {\\ - RESOLVE_MOUNTABLE (string, mountable, cancel_stmt); \\ + if (STRPREFIX ((string), \"/dev/\") || (string)[0] != '/') { \\ + RESOLVE_MOUNTABLE ((string), (mountable), (is_filein)); \\ } \\ else { \\ - NEED_ROOT (cancel_stmt, return); \\ + NEED_ROOT ((is_filein), return); \\ /* NB: It's a path, not a device. */ \\ (mountable).type = MOUNTABLE_PATH; \\ (mountable).device = strdup ((string)); \\ (mountable).volume = NULL; \\ if ((mountable).device == NULL) { \\ - cancel_stmt; \\ + if (is_filein) cancel_receive (); \\ reply_with_perror (\"strdup\"); \\ return; \\ } \\ @@ -206,6 +206,7 @@ let generate_daemon_stubs actions () = #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <inttypes.h> #include <errno.h> @@ -353,20 +354,17 @@ let generate_daemon_stubs actions () = function | Pathname n -> pr_args n; - pr " ABS_PATH (%s, %s, return);\n" - n (if is_filein then "cancel_receive ()" else ""); + pr " ABS_PATH (%s, %b, return);\n" n is_filein; | Device n -> - pr " RESOLVE_DEVICE (args.%s, %s, %s);\n" - n n (if is_filein then "cancel_receive ()" else ""); + pr " RESOLVE_DEVICE (args.%s, %s, %b);\n" n n is_filein; | Mountable n -> - pr " RESOLVE_MOUNTABLE (args.%s, %s, %s);\n" - n n (if is_filein then "cancel_receive ()" else ""); + pr " RESOLVE_MOUNTABLE (args.%s, %s, %b);\n" n n is_filein; | Dev_or_Path n -> - pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (args.%s, %s, %s);\n" - n n (if is_filein then "cancel_receive ()" else ""); + pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (args.%s, %s, %b);\n" + n n is_filein; | Mountable_or_Path n -> - pr " REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE (args.%s, %s, %s);\n" - n n (if is_filein then "cancel_receive ()" else ""); + pr " REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE (args.%s, %s, %b);\n" + n n is_filein; | String n | Key n | GUID n -> pr_args n | OptString n -> pr " %s = args.%s ? *args.%s : NULL;\n" n n n | StringList n | FilenameList n as arg -> @@ -405,9 +403,8 @@ let generate_daemon_stubs actions () = pr " {\n"; pr " size_t i;\n"; pr " for (i = 0; i < args.%s.%s_len; ++i)\n" n n; - pr " RESOLVE_DEVICE (args.%s.%s_val[i], %s[i],\n" n n n; - pr " %s);\n" - (if is_filein then "cancel_receive ()" else ""); + pr " RESOLVE_DEVICE (args.%s.%s_val[i], %s[i], %b);\n" + n n n is_filein; pr " %s[i] = NULL;\n" n; pr " }\n" | Bool n -> pr " %s = args.%s;\n" n n @@ -425,8 +422,7 @@ let generate_daemon_stubs actions () = if List.exists (function Pathname _ -> true | _ -> false) args then ( (* Emit NEED_ROOT just once, even when there are two or more Pathname args *) - pr " NEED_ROOT (%s, return);\n" - (if is_filein then "cancel_receive ()" else ""); + pr " NEED_ROOT (%b, return);\n" is_filein ); (* Don't want to call the impl with any FileIn or FileOut -- 2.12.0