Hi everyone,
lately I've been getting familiar with library and working on slight
re-layering of the library. It's about having locking layer in public API
and
tracing one layer below that (let's call it __t_ layer. I'm not very
good at
making up names, so this is temporary:) ). Then making sure that all generated
public stuff call __t_ layer and all other internal stuff doesn't use public
API since it would deadlock otherwise.
Now the problem - an example:
Generator creates guestfs_copy_device_to_device_argv, but not
guestfs_copy_device_to_device_argv version.
Other issue:
generated declaration for guestfs__internal_test in guestfs-internal-actions.h
looks like this:
extern int guestfs__internal_test (guestfs_h *g, const char *str,
const char *optstr, char *const *strlist, int b, int integer,
int64_t integer64, const char *filein, const char *fileout,
const char *bufferin, size_t bufferin_size, ...);
but, it's type in bindtests.c looks different:
int
guestfs__internal_test (guestfs_h *g,
const char *str,
const char *optstr,
char *const *strlist,
int b,
int integer,
int64_t integer64,
const char *filein,
const char *fileout,
const char *bufferin,
size_t bufferin_size,
const struct guestfs_internal_test_argv *optargs)
Any ideas, please? I'd very grateful for any :)
- maros
n.b.: please ignore some rubbish in sources, it's still WIP
Maros Zatko (1):
WIP locking/tracing layer split
generator/c.ml | 165 +++++++++++++++++++++++++++++++++++++++++++------
src/fuse.c | 14 +++++
src/guestfs-internal.h | 3 +
src/handle.c | 29 +++++----
src/inspect-fs.c | 10 +--
src/inspect-icon.c | 7 +++
src/tmpdirs.c | 2 +-
7 files changed, 193 insertions(+), 37 deletions(-)
--
1.8.5.3
---
generator/c.ml | 165 +++++++++++++++++++++++++++++++++++++++++++------
src/fuse.c | 14 +++++
src/guestfs-internal.h | 3 +
src/handle.c | 29 +++++----
src/inspect-fs.c | 10 +--
src/inspect-icon.c | 7 +++
src/tmpdirs.c | 2 +-
7 files changed, 193 insertions(+), 37 deletions(-)
diff --git a/generator/c.ml b/generator/c.ml
index ee276dc..113b582 100644
--- a/generator/c.ml
+++ b/generator/c.ml
@@ -66,6 +66,7 @@ let rec generate_prototype ?(extern = true) ?(static = false)
?(prefix = "") ?(suffix = "")
?handle
?(optarg_proto = Dots)
+ ?(optarg_name = "")
name (ret, args, optargs) pr "%s" indent;
if extern then pr "extern ";
@@ -156,7 +157,10 @@ let rec generate_prototype ?(extern = true) ?(static =
false)
match optarg_proto with
| Dots -> pr "..."
| VA -> pr "va_list args"
- | Argv -> pr "const struct guestfs_%s_argv *optargs" name
+ | Argv -> pr "const struct guestfs_%s_argv *optargs"
+ (match optarg_name with
+ | "" -> name
+ | _ -> optarg_name)
);
(* Was anything output between ()? If not, it's a
'function(void)'. *)
@@ -671,13 +675,22 @@ extern GUESTFS_DLL_PUBLIC void *guestfs_next_private
(guestfs_h *g, const char *
| Some fn -> pr "\n GUESTFS_DEPRECATED_BY (%S);\n" fn
| None -> pr ";\n"
);
+(* generate_prototype ~single_line:true ~semicolon:false
+ ~handle:"g" ~prefix:"guestfs__t_" shortname style;
+ (match deprecated_by with
+ | Some fn -> pr "\n GUESTFS_DEPRECATED_BY (%S);\n" fn
+ | None -> pr ";\n"
+ ); *)
if optargs <> [] then (
generate_prototype ~single_line:true ~newline:true ~handle:"g"
~prefix:"guestfs_" ~suffix:"_va" ~optarg_proto:VA
~dll_public:true
shortname style;
-
+(* generate_prototype ~single_line:true ~newline:true
~handle:"g"
+ ~prefix:"guestfs__t_" ~suffix:"_va"
~optarg_proto:VA
+ shortname style;
+ *)
pr "\n";
pr "struct guestfs_%s_argv {\n" shortname;
pr " uint64_t bitmask;\n";
@@ -703,6 +716,9 @@ extern GUESTFS_DLL_PUBLIC void *guestfs_next_private
(guestfs_h *g, const char *
~prefix:"guestfs_" ~suffix:"_argv"
~optarg_proto:Argv
~dll_public:true
shortname style;
+(* generate_prototype ~single_line:true ~newline:true
~handle:"g"
+ ~prefix:"guestfs__t_" ~suffix:"_argv"
~optarg_proto:Argv
+ shortname style; *)
);
pr "\n"
@@ -791,7 +807,10 @@ and generate_internal_actions_h () List.iter (
fun { c_name = c_name; style = style } ->
generate_prototype ~single_line:true ~newline:true ~handle:"g"
- ~prefix:"guestfs__" ~optarg_proto:Argv
+ ~prefix:"guestfs__t_" ~optarg_proto:Dots
+ c_name style;
+ generate_prototype ~single_line:true ~newline:true ~handle:"g"
+ ~prefix:"guestfs__" ~optarg_proto:Dots
c_name style
) non_daemon_functions;
@@ -1551,20 +1570,19 @@ and generate_client_actions hash () )
in
- (* For non-daemon functions, generate a wrapper around each function. *)
- let generate_non_daemon_wrapper { name = name; c_name = c_name;
+ let generate_tracing_wrapper { name = name; c_name = c_name;
style = ret, _, optargs as style;
config_only = config_only } if optargs
= [] then
generate_prototype ~extern:false ~semicolon:false ~newline:true
~handle:"g" ~prefix:"guestfs_"
- ~dll_public:true
- c_name style
+(* ~optarg_name:c_name *)
+ ("_t_" ^ c_name) style
else
generate_prototype ~extern:false ~semicolon:false ~newline:true
~handle:"g" ~prefix:"guestfs_"
~suffix:"_argv" ~optarg_proto:Argv
- ~dll_public:true
- c_name style;
+ ~optarg_name:c_name
+ ("_t_" ^ c_name) style;
pr "{\n";
handle_null_optargs optargs c_name;
@@ -1617,11 +1635,101 @@ and generate_client_actions hash () trace_return
name style "r";
);
pr "\n";
+
pr " return r;\n";
pr "}\n";
pr "\n"
in
+ (* For non-daemon functions, generate a wrapper around each function. *)
+ let generate_non_daemon_wrapper ({ name = name; c_name = c_name;
+ style = ret, _, optargs as style;
+ config_only = config_only } as arg) + (*
Add tracing, but non-locking layer below public api, we'll use it
internally. *)
+ (* generate_tracing_wrapper { arg with name = ("_t_" ^ c_name) };
*)
+ generate_tracing_wrapper { arg with name = ("_t_" ^ name) };
+ pr "\n";
+
+ if optargs = [] then
+ generate_prototype ~extern:false ~semicolon:false ~newline:true
+ ~handle:"g" ~prefix:"guestfs_"
+ ~dll_public:true
+ c_name style
+ else (
+ generate_prototype ~extern:false ~semicolon:false ~newline:true
+ ~handle:"g" ~prefix:"guestfs_"
~suffix:"_argv" ~optarg_proto:Argv
+ ~dll_public:true
+ c_name style
+ );
+
+ pr "{\n";
+
+ handle_null_optargs optargs c_name;
+
+ if optargs = [] then
+ pr " printf(\"locking in %s\\n\");\n" c_name
+ else
+ pr " printf(\"locking in %s_argv\\n\");\n" c_name;
+
+(* pr " guestfs___per_handle_lock_lock (g);\n"; *)
+ pr " gl_lock_lock (g->global_lock);\n";
+ pr "\n";
+
+ (match ret with
+ | RErr | RInt _ | RBool _ ->
+ pr " int r;\n"
+ | RInt64 _ ->
+ pr " int64_t r;\n"
+ | RConstString _ ->
+ pr " const char *r;\n"
+ | RConstOptString _ ->
+ pr " const char *r;\n"
+ | RString _ | RBufferOut _ ->
+ pr " char *r;\n"
+ | RStringList _ | RHashtable _ ->
+ pr " char **r;\n"
+ | RStruct (_, typ) ->
+ pr " struct guestfs_%s *r;\n" typ
+ | RStructList (_, typ) ->
+ pr " struct guestfs_%s_list *r;\n" typ
+ );
+ pr "\n";
+ if config_only then (
+ pr " if (g->state != CONFIG) {\n";
+ pr " error (g, \"%%s: this function can only be called in
the config state\",\n";
+ pr " \"%s\");\n" c_name;
+ pr " return -1;\n";
+ pr " }\n";
+ );
+ if optargs = [] then
+ pr " r = guestfs__t_%s " c_name
+ else
+ pr " r = guestfs__t_%s " (c_name ^ "_argv");
+
+ generate_c_call_args ~handle:"g"
~implicit_size_ptr:"size_r" style;
+ pr ";\n";
+ pr "\n";
+
+ (* pr " guestfs___per_handle_lock_unlock (g);\n"; *)
+ pr " gl_lock_unlock (g->global_lock);\n";
+ if optargs = [] then
+ pr " printf(\"unlocked in %s\\n\");\n" c_name
+ else
+ pr " printf(\"unlocked in %s_argv\\n\");\n" c_name;
+
+ pr "\n";
+
+ pr " return r;\n";
+ pr "}\n";
+ pr "\n"
+ in
+
+(* let generate_non_daemon_trace_wrapper x + pr "WRAPPED!!1";
+ generate_non_daemon_wrapper x;
+ pr "POST WRAP!"
+ in *)
+
List.iter (
function
| { wrapper = true } as f ->
@@ -1949,7 +2057,7 @@ and generate_client_actions_variants () ";
let generate_va_variants { name = name; c_name = c_name;
- style = ret, args, optargs as style } +
style = ret, args, optargs as style } locking assert (optargs <> []);
(* checked by caller *)
(* Get the name of the last regular argument. *)
@@ -1979,15 +2087,23 @@ and generate_client_actions_variants () sprintf
"struct guestfs_%s_list *" typ in
(* The regular variable args function, just calls the _va variant. *)
- generate_prototype ~extern:false ~semicolon:false ~newline:true
- ~handle:"g" ~prefix:"guestfs_" c_name style;
+ if locking then
+ generate_prototype ~extern:false ~semicolon:false ~newline:true
+ ~handle:"g" ~prefix:"guestfs_" c_name style
+ else
+ generate_prototype ~extern:false ~semicolon:false ~newline:true
+ ~handle:"g" ~prefix:"guestfs__t_" c_name style;
+
pr "{\n";
pr " va_list optargs;\n";
pr "\n";
pr " %sr;\n" rtype;
pr "\n";
pr " va_start (optargs, %s);\n" last_arg;
- pr " r = guestfs_%s_va " c_name;
+ if locking then
+ pr " r = guestfs__t_%s_va " c_name
+ else
+ pr " r = guestfs__%s_va " c_name;
generate_c_call_args ~handle:"g"
~implicit_size_ptr:"size_r" style;
pr ";\n";
pr " va_end (optargs);\n";
@@ -1995,9 +2111,15 @@ and generate_client_actions_variants () pr "
return r;\n";
pr "}\n\n";
- generate_prototype ~extern:false ~semicolon:false ~newline:true
- ~handle:"g" ~prefix:"guestfs_"
~suffix:"_va" ~optarg_proto:VA
- c_name style;
+ if locking then
+ generate_prototype ~extern:false ~semicolon:false ~newline:true
+ ~handle:"g" ~prefix:"guestfs_"
~suffix:"_va" ~optarg_proto:VA
+ c_name style
+ else
+ generate_prototype ~extern:false ~semicolon:false ~newline:true
+ ~handle:"g" ~prefix:"guestfs__t_"
~suffix:"_va" ~optarg_proto:VA
+ c_name style;
+
pr "{\n";
pr " struct guestfs_%s_argv optargs_s;\n" c_name;
pr " struct guestfs_%s_argv *optargs = &optargs_s;\n"
c_name;
@@ -2045,7 +2167,10 @@ and generate_client_actions_variants () pr "
optargs_s.bitmask |= i_mask;\n";
pr " }\n";
pr "\n";
- pr " return guestfs_%s_argv " c_name;
+ if locking then
+ pr " return guestfs_%s_argv " c_name
+ else
+ pr " return guestfs__t_%s_argv " c_name;
generate_c_call_args ~handle:"g"
~implicit_size_ptr:"size_r" style;
pr ";\n";
pr "}\n\n"
@@ -2070,9 +2195,11 @@ and generate_client_actions_variants () function
| { style = _, _, [] } -> ()
| ({ style = _, _, (_::_); once_had_no_optargs = false } as f) ->
- generate_va_variants f
+ generate_va_variants f false;
+ generate_va_variants f true
| ({ style = _, _, (_::_); once_had_no_optargs = true } as f) ->
- generate_va_variants f;
+ generate_va_variants f false;
+ generate_va_variants f true;
generate_back_compat_wrapper f
) all_functions_sorted
diff --git a/src/fuse.c b/src/fuse.c
index dd63729..020a865 100644
--- a/src/fuse.c
+++ b/src/fuse.c
@@ -1004,6 +1004,13 @@ guestfs__mount_local (guestfs_h *g, const char
*localmountpoint,
}
int
+guestfs__t_mount_local (guestfs_h *g, const char *localmountpoint,
+ const struct guestfs_mount_local_argv *optargs)
+{
+ return guestfs__mount_local (g, localmountpoint, optargs);
+}
+
+int
guestfs__mount_local_run (guestfs_h *g)
{
int r, mounted;
@@ -1105,6 +1112,13 @@ guestfs__umount_local (guestfs_h *g,
return -1;
}
+int
+guestfs__t_umount_local (guestfs_h *g,
+ const struct guestfs_umount_local_argv *optargs)
+{
+ return guestfs__umount_local(g, optargs);
+}
+
/* Functions handling the directory cache.
*
* Note on attribute caching: FUSE can cache filesystem attributes for
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index b99335b..9254d4a 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -32,6 +32,7 @@
#include <libvirt/libvirt.h>
#endif
+#include "glthread/lock.h"
#include "hash.h"
#include "guestfs-internal-frontend.h"
@@ -493,6 +494,8 @@ struct guestfs_h
unsigned int nr_requested_credentials;
virConnectCredentialPtr requested_credentials;
#endif
+
+ gl_recursive_lock_t global_lock;
};
/* Per-filesystem data stored for inspect_os. */
diff --git a/src/handle.c b/src/handle.c
index 719bbcd..b68444d 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -84,6 +84,9 @@ guestfs_create_flags (unsigned flags, ...)
g = calloc (1, sizeof (*g));
if (!g) return NULL;
+// guestfs___per_handle_lock_add (g);
+ gl_lock_init (g->global_lock);
+
g->state = CONFIG;
g->conn = NULL;
@@ -167,6 +170,7 @@ guestfs_create_flags (unsigned flags, ...)
free (g->path);
free (g->hv);
free (g->append);
+ gl_lock_destroy (g->global_lock);
free (g);
return NULL;
}
@@ -185,21 +189,21 @@ parse_environment (guestfs_h *g,
str = do_getenv (data, "LIBGUESTFS_TRACE");
if (str != NULL && STREQ (str, "1"))
- guestfs_set_trace (g, 1);
+ guestfs__t_set_trace (g, 1);
str = do_getenv (data, "LIBGUESTFS_DEBUG");
if (str != NULL && STREQ (str, "1"))
- guestfs_set_verbose (g, 1);
+ guestfs__t_set_verbose (g, 1);
str = do_getenv (data, "LIBGUESTFS_TMPDIR");
if (str) {
- if (guestfs_set_tmpdir (g, str) == -1)
+ if (guestfs__t_set_tmpdir (g, str) == -1)
return -1;
}
str = do_getenv (data, "LIBGUESTFS_CACHEDIR");
if (str) {
- if (guestfs_set_cachedir (g, str) == -1)
+ if (guestfs__t_set_cachedir (g, str) == -1)
return -1;
}
@@ -209,20 +213,20 @@ parse_environment (guestfs_h *g,
str = do_getenv (data, "LIBGUESTFS_PATH");
if (str)
- guestfs_set_path (g, str);
+ guestfs__t_set_path (g, str);
str = do_getenv (data, "LIBGUESTFS_HV");
if (str)
- guestfs_set_hv (g, str);
+ guestfs__t_set_hv (g, str);
else {
str = do_getenv (data, "LIBGUESTFS_QEMU");
if (str)
- guestfs_set_hv (g, str);
+ guestfs__t_set_hv (g, str);
}
str = do_getenv (data, "LIBGUESTFS_APPEND");
if (str)
- guestfs_set_append (g, str);
+ guestfs__t_set_append (g, str);
str = do_getenv (data, "LIBGUESTFS_MEMSIZE");
if (str) {
@@ -230,18 +234,18 @@ parse_environment (guestfs_h *g,
error (g, _("non-numeric or too small value for
LIBGUESTFS_MEMSIZE"));
return -1;
}
- guestfs_set_memsize (g, memsize);
+ guestfs__t_set_memsize (g, memsize);
}
str = do_getenv (data, "LIBGUESTFS_BACKEND");
if (str) {
- if (guestfs_set_backend (g, str) == -1)
+ if (guestfs__t_set_backend (g, str) == -1)
return -1;
}
else {
str = do_getenv (data, "LIBGUESTFS_ATTACH_METHOD");
if (str) {
- if (guestfs_set_backend (g, str) == -1)
+ if (guestfs__t_set_backend (g, str) == -1)
return -1;
}
}
@@ -255,7 +259,7 @@ parse_environment (guestfs_h *g,
return -1;
}
- if (guestfs_set_backend_settings (g, settings) == -1)
+ if (guestfs__t_set_backend_settings (g, settings) == -1)
return -1;
}
@@ -374,6 +378,7 @@ guestfs_close (guestfs_h *g)
free (g->backend_data);
guestfs___free_string_list (g->backend_settings);
free (g->append);
+ gl_lock_destroy (g->global_lock);
free (g);
}
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index c011b5a..03fd0bf 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -149,12 +149,12 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const
char *mountable)
guestfs_push_error_handler (g, NULL, NULL);
if (vfs_type && STREQ (vfs_type, "ufs")) { /* Hack for the
*BSDs. */
/* FreeBSD fs is a variant of ufs called ufs2 ... */
- r = guestfs_mount_vfs (g, "ro,ufstype=ufs2", "ufs",
mountable, "/");
+ r = guestfs__t_mount_vfs (g, "ro,ufstype=ufs2", "ufs",
mountable, "/");
if (r == -1)
/* while NetBSD and OpenBSD use another variant labeled 44bsd */
- r = guestfs_mount_vfs (g, "ro,ufstype=44bsd", "ufs",
mountable, "/");
+ r = guestfs__t_mount_vfs (g, "ro,ufstype=44bsd",
"ufs", mountable, "/");
} else {
- r = guestfs_mount_ro (g, mountable, "/");
+ r = guestfs__t_mount_ro (g, mountable, "/");
}
guestfs_pop_error_handler (g);
if (r == -1)
@@ -366,7 +366,7 @@ guestfs___is_file_nocase (guestfs_h *g, const char *path)
p = guestfs___case_sensitive_path_silently (g, path);
if (!p)
return 0;
- r = guestfs_is_file (g, p);
+ r = guestfs__t_is_file (g, p);
return r > 0;
}
@@ -379,7 +379,7 @@ guestfs___is_dir_nocase (guestfs_h *g, const char *path)
p = guestfs___case_sensitive_path_silently (g, path);
if (!p)
return 0;
- r = guestfs_is_dir (g, p);
+ r = guestfs__t_is_dir (g, p);
return r > 0;
}
diff --git a/src/inspect-icon.c b/src/inspect-icon.c
index 94b63a2..f17149b 100644
--- a/src/inspect-icon.c
+++ b/src/inspect-icon.c
@@ -214,6 +214,13 @@ guestfs__inspect_get_icon (guestfs_h *g, const char *root,
size_t *size_r,
return r;
}
+char *
+guestfs__t_inspect_get_icon (guestfs_h *g, const char *root, size_t *size_r,
+ const struct guestfs_inspect_get_icon_argv *optargs)
+{
+ return guestfs__inspect_get_icon(g, root, size_r, optargs);
+}
+
/* Check that the named file 'filename' is a PNG file and is
reasonable.
* If it is, download and return it.
*/
diff --git a/src/tmpdirs.c b/src/tmpdirs.c
index 56523aa..63d235e 100644
--- a/src/tmpdirs.c
+++ b/src/tmpdirs.c
@@ -129,7 +129,7 @@ int
guestfs___lazy_make_tmpdir (guestfs_h *g)
{
if (!g->tmpdir) {
- CLEANUP_FREE char *tmpdir = guestfs_get_tmpdir (g);
+ CLEANUP_FREE char *tmpdir = guestfs__t_get_tmpdir (g);
g->tmpdir = safe_asprintf (g, "%s/libguestfsXXXXXX", tmpdir);
if (mkdtemp (g->tmpdir) == NULL) {
perrorf (g, _("%s: cannot create temporary directory"),
g->tmpdir);
--
1.8.5.3
Richard W.M. Jones
2014-Jun-27 16:00 UTC
Re: [Libguestfs] [PATCH WIP] Can't generate argv variant
On Fri, Jun 27, 2014 at 04:55:33PM +0200, Maros Zatko wrote:> Hi everyone, > lately I've been getting familiar with library and working on slight > re-layering of the library. It's about having locking layer in public API and > tracing one layer below that (let's call it __t_ layer. I'm not very good at > making up names, so this is temporary:) ). Then making sure that all generated > public stuff call __t_ layer and all other internal stuff doesn't use public > API since it would deadlock otherwise. > > Now the problem - an example: > Generator creates guestfs_copy_device_to_device_argv, but not > guestfs_copy_device_to_device_argv version.Is there a typo in this sentence? The generator makes: - guestfs_copy_device_to_device in src/actions-variants.c - guestfs_copy_device_to_device_va in src/actions-variants.c - guestfs_copy_device_to_device_argv in src/actions-1.c guestfs_copy_device_to_device --> calls guestfs_copy_device_to_device_argv guestfs_copy_device_to_device_va --> calls guestfs_copy_device_to_device_argv The "real" public API here is guestfs_copy_device_to_device_argv, and I think that's where the locking should go. If there is some code in libguestfs which recursively calls guestfs_copy_device_to_device now (there happens not to be), I think it should be changed to call _guestfs_unlocked_copy_device_to_device, or whatever we're calling the internal unlocked version.> Other issue: > generated declaration for guestfs__internal_test in guestfs-internal-actions.h > looks like this: > > extern int guestfs__internal_test (guestfs_h *g, const char *str, > const char *optstr, char *const *strlist, int b, int integer, > int64_t integer64, const char *filein, const char *fileout, > const char *bufferin, size_t bufferin_size, ...);In my copy it is declared as: extern int guestfs__internal_test (guestfs_h *g, const char *str, const char *optstr, char *const *strlist, int b, int integer, int64_t integer64, const char *filein, const char *fileout, const char *bufferin, size_t bufferin_size, const struct guestfs_internal_test_argv *optargs); so that would be correct I think. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Richard W.M. Jones
2014-Jun-27 17:01 UTC
Re: [Libguestfs] [PATCH WIP] Can't generate argv variant
On Fri, Jun 27, 2014 at 05:00:50PM +0100, Richard W.M. Jones wrote:> If there is some code in libguestfs which recursively calls > guestfs_copy_device_to_device now (there happens not to be), I think > it should be changed to call _guestfs_unlocked_copy_device_to_device,I mean ... _guestfs_unlocked_copy_device_to_device_argv> or whatever we're calling the internal unlocked version.Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/