Richard W.M. Jones
2017-Apr-20 12:56 UTC
[Libguestfs] [PATCH 0/5] generator: daemon: Various simplifications to stubs code.
This is a series of simplifications to the stubs code. It's all refactoring, there is no functional change. Rich.
Richard W.M. Jones
2017-Apr-20 12:56 UTC
[Libguestfs] [PATCH 1/5] generator: daemon: Get rid of useless done_no_free label.
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
Richard W.M. Jones
2017-Apr-20 12:56 UTC
[Libguestfs] [PATCH 2/5] generator: daemon: Use a cleanup function to free XDR args struct.
Previously stubs were generated like this:
void
fn_stub (XDR *xdr_in)
{
struct guestfs_fn_args args;
...
// on error paths, ‘goto done’
...
done:
xdr_free ((xdrproc_t) xdr_guestfs_fn_args, (char *) &args);
}
This replaces the call to xdr_free with a generated cleanup function.
---
generator/daemon.ml | 44 ++++++++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/generator/daemon.ml b/generator/daemon.ml
index 83c0ad24c..535f99458 100644
--- a/generator/daemon.ml
+++ b/generator/daemon.ml
@@ -225,6 +225,33 @@ let generate_daemon_stubs actions () List.iter (
fun { name = name; style = ret, args, optargs; optional = optional } ->
(* Generate server-side stubs. *)
+ let uc_name = String.uppercase_ascii name in
+
+ let args_passed_to_daemon = args @ args_of_optargs optargs in
+ let args_passed_to_daemon + List.filter (function FileIn _ |
FileOut _ -> false | _ -> true)
+ args_passed_to_daemon in
+
+ if args_passed_to_daemon <> [] then (
+ pr "#ifdef HAVE_ATTRIBUTE_CLEANUP\n";
+ pr "\n";
+ pr "#define CLEANUP_XDR_FREE_%s_ARGS \\\n" uc_name;
+ pr " __attribute__((cleanup(cleanup_xdr_free_%s_args)))\n"
name;
+ pr "\n";
+ pr "static void\n";
+ pr "cleanup_xdr_free_%s_args (struct guestfs_%s_args
*argsp)\n"
+ name name;
+ pr "{\n";
+ pr " xdr_free ((xdrproc_t) xdr_guestfs_%s_args, (char *)
argsp);\n"
+ name;
+ pr "}\n";
+ pr "\n";
+ pr "#else /* !HAVE_ATTRIBUTE_CLEANUP */\n";
+ pr "#define CLEANUP_XDR_FREE_%s_ARGS\n" uc_name;
+ pr "#endif /* !HAVE_ATTRIBUTE_CLEANUP */\n";
+ pr "\n"
+ );
+
pr "void\n";
pr "%s_stub (XDR *xdr_in)\n" name;
pr "{\n";
@@ -243,12 +270,10 @@ let generate_daemon_stubs actions () pr "
char *r;\n"
);
- let args_passed_to_daemon = args @ args_of_optargs optargs in
- let args_passed_to_daemon - List.filter (function FileIn _ |
FileOut _ -> false | _ -> true)
- args_passed_to_daemon in
if args_passed_to_daemon <> [] then (
- pr " struct guestfs_%s_args args;\n" name;
+ pr " CLEANUP_XDR_FREE_%s_ARGS struct guestfs_%s_args
args;\n"
+ uc_name name;
+ pr " memset (&args, 0, sizeof args);\n";
List.iter (
function
| Device n | Dev_or_Path n ->
@@ -316,8 +341,6 @@ let generate_daemon_stubs actions ()
(* Decode arguments. *)
if args_passed_to_daemon <> [] then (
- pr " memset (&args, 0, sizeof args);\n";
- pr "\n";
pr " if (!xdr_guestfs_%s_args (xdr_in, &args)) {\n"
name;
if is_filein then
pr " cancel_receive ();\n";
@@ -498,14 +521,7 @@ let generate_daemon_stubs actions () pr "
free (r);\n"
);
- (* Free the args. *)
pr "done: ;\n";
- (match args_passed_to_daemon with
- | [] -> ()
- | _ ->
- pr " xdr_free ((xdrproc_t) xdr_guestfs_%s_args, (char *)
&args);\n"
- name
- );
pr "}\n\n";
) (actions |> daemon_functions |> sort)
--
2.12.0
Richard W.M. Jones
2017-Apr-20 12:56 UTC
[Libguestfs] [PATCH 3/5] generator: daemon: Remove done label.
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
Richard W.M. Jones
2017-Apr-20 12:56 UTC
[Libguestfs] [PATCH 4/5] generator: daemon: Remove fail_stmt parameter from *RESOLVE* macros.
As a simple consequence of the previous commit, every instance of the
‘fail_stmt’ parameter to one of the following macros:
RESOLVE_DEVICE
RESOLVE_MOUNTABLE
REQUIRE_ROOT_OR_RESOLVE_DEVICE
REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE
is now ‘return’ and therefore we can replace it in the macro and drop
the parameter completely.
---
generator/daemon.ml | 43 +++++++++++++++++++++----------------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/generator/daemon.ml b/generator/daemon.ml
index a469831c1..2816cb847 100644
--- a/generator/daemon.ml
+++ b/generator/daemon.ml
@@ -91,17 +91,17 @@ let generate_daemon_stubs_h () *
* NB. Cannot be used for FileIn functions.
*/
-#define RESOLVE_DEVICE(path,path_out,cancel_stmt,fail_stmt) \\
+#define RESOLVE_DEVICE(path,path_out,cancel_stmt) \\
do { \\
if (STRNEQLEN ((path), \"/dev/\", 5)) { \\
cancel_stmt; \\
reply_with_error (\"%%s: %%s: expecting a device name\",
__func__, (path)); \\
- fail_stmt; \\
+ return; \\
} \\
if (is_root_device (path)) { \\
cancel_stmt; \\
reply_with_error (\"%%s: %%s: device not found\", __func__,
path); \\
- fail_stmt; \\
+ return; \\
} \\
(path_out) = device_name_translation ((path)); \\
if ((path_out) == NULL) { \\
@@ -109,7 +109,7 @@ let generate_daemon_stubs_h () cancel_stmt;
\\
errno = err; \\
reply_with_perror (\"%%s: %%s\", __func__, path);
\\
- fail_stmt; \\
+ return; \\
} \\
} while (0)
@@ -120,7 +120,7 @@ let generate_daemon_stubs_h () *
* Note that the \"string\" argument may be modified.
*/
-#define RESOLVE_MOUNTABLE(string,mountable,cancel_stmt,fail_stmt) \\
+#define RESOLVE_MOUNTABLE(string,mountable,cancel_stmt) \\
do { \\
if (STRPREFIX ((string), \"btrfsvol:\")) {
\\
if (parse_btrfsvol ((string) + strlen (\"btrfsvol:\"),
&(mountable)) == -1)\\
@@ -128,14 +128,14 @@ let generate_daemon_stubs_h () cancel_stmt;
\\
reply_with_error (\"%%s: %%s: expecting a btrfs volume\",
\\
__func__, (string)); \\
- fail_stmt; \\
+ return; \\
} \\
} \\
else { \\
(mountable).type = MOUNTABLE_DEVICE; \\
(mountable).device = NULL; \\
(mountable).volume = NULL; \\
- RESOLVE_DEVICE ((string), (mountable).device, cancel_stmt, fail_stmt); \\
+ RESOLVE_DEVICE ((string), (mountable).device, cancel_stmt); \\
} \\
} while (0)
@@ -149,18 +149,18 @@ 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,fail_stmt) \\
+#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,path_out,cancel_stmt) \\
do { \\
if (STREQLEN ((path), \"/dev/\", 5))
\\
- RESOLVE_DEVICE ((path), (path_out), cancel_stmt, fail_stmt); \\
+ RESOLVE_DEVICE ((path), (path_out), cancel_stmt); \\
else { \\
- NEED_ROOT (cancel_stmt, fail_stmt); \\
- ABS_PATH ((path), cancel_stmt, fail_stmt); \\
+ NEED_ROOT (cancel_stmt, return); \\
+ ABS_PATH ((path), cancel_stmt, return); \\
(path_out) = strdup ((path)); \\
if ((path_out) == NULL) { \\
cancel_stmt; \\
reply_with_perror (\"strdup\");
\\
- fail_stmt; \\
+ return; \\
} \\
} \\
} while (0)
@@ -168,14 +168,13 @@ 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, fail_stmt) \\
+#define REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE(string, mountable, cancel_stmt) \\
do { \\
if (STRPREFIX ((string), \"/dev/\") || (string)[0] !=
'/') {\\
- RESOLVE_MOUNTABLE (string, mountable, cancel_stmt, fail_stmt); \\
+ RESOLVE_MOUNTABLE (string, mountable, cancel_stmt); \\
} \\
else { \\
- NEED_ROOT (cancel_stmt, fail_stmt); \\
+ NEED_ROOT (cancel_stmt, return); \\
/* NB: It's a path, not a device. */ \\
(mountable).type = MOUNTABLE_PATH; \\
(mountable).device = strdup ((string)); \\
@@ -183,7 +182,7 @@ let generate_daemon_stubs_h () if ((mountable).device
== NULL) { \\
cancel_stmt; \\
reply_with_perror (\"strdup\");
\\
- fail_stmt; \\
+ return; \\
} \\
} \\
} while (0) \\
@@ -357,16 +356,16 @@ let generate_daemon_stubs actions () pr
" ABS_PATH (%s, %s, return);\n"
n (if is_filein then "cancel_receive ()" else
"");
| Device n ->
- pr " RESOLVE_DEVICE (args.%s, %s, %s, return);\n"
+ pr " RESOLVE_DEVICE (args.%s, %s, %s);\n"
n n (if is_filein then "cancel_receive ()" else
"");
| Mountable n ->
- pr " RESOLVE_MOUNTABLE (args.%s, %s, %s, return);\n"
+ pr " RESOLVE_MOUNTABLE (args.%s, %s, %s);\n"
n n (if is_filein then "cancel_receive ()" else
"");
| Dev_or_Path n ->
- pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (args.%s, %s, %s,
return);\n"
+ pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (args.%s, %s,
%s);\n"
n n (if is_filein then "cancel_receive ()" else
"");
| Mountable_or_Path n ->
- pr " REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE (args.%s, %s, %s,
return);\n"
+ pr " REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE (args.%s, %s,
%s);\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
@@ -407,7 +406,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, return);\n"
+ pr " %s);\n"
(if is_filein then "cancel_receive ()" else
"");
pr " %s[i] = NULL;\n" n;
pr " }\n"
--
2.12.0
Richard W.M. Jones
2017-Apr-20 12:56 UTC
[Libguestfs] [PATCH 5/5] generator: daemon: Replace ‘cancel_stmt’ with ‘is_filein’ flag.
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
Pino Toscano
2017-Apr-21 11:36 UTC
Re: [Libguestfs] [PATCH 0/5] generator: daemon: Various simplifications to stubs code.
On Thursday, 20 April 2017 14:56:28 CEST Richard W.M. Jones wrote:> This is a series of simplifications to the stubs code. > > It's all refactoring, there is no functional change.Refactoring indeed, LGTM. Thanks, -- Pino Toscano
Possibly Parallel Threads
- [REVIEW ONLY] Mountable patches
- [PATCH 01/12] generator: Add new Mountable argument type
- [PATCH 0/23] factor and const-correctness
- [PATCH 00/67] Proposed patches for libguestfs 1.22.6.
- [PATCH v2 0/4] daemon: Translate device names if Linux device is unstable (RHBZ#1804207).