Libguestfs - Oct 2012 - [PATCH 0/2 NOT WORKING] Symbol versioning

John,

This was my attempt to add symbol versioning to the library,
letting us break ABI without breaking any existing callers.

Unfortunately it doesn't work:

 - the new versioned symbols are marked local in libguestfs.so

 - the existing symbols should now have @GUESTFS_0.0 versions,
   but don't

The documentation for this stuff is extremely thin, and I've
got a bad case of influenza at the moment.

Rich.

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 generator/actions.ml |  1 +
 generator/c.ml       | 96 ++++++++++++++++++++++++++++++++++++++++++++++------
 generator/checks.ml  | 10 ++++++
 generator/types.ml   |  2 ++
 src/Makefile.am      |  1 +
 src/compat.c         | 25 ++++++++++++++
 src/guestfs.pod      | 26 ++++++++++++++
 7 files changed, 151 insertions(+), 10 deletions(-)
 create mode 100644 src/compat.c

diff --git a/generator/actions.ml b/generator/actions.ml
index 71aee37..e1db3db 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -32,6 +32,7 @@ let defaults = { name = ""; style = RErr, [], [];
proc_nr = None;
                  progress = false; camel_name = "";
                  cancellable = false; config_only = false;
                  once_had_no_optargs = false; blocking = true;
+                 symbol_version = None;
                  c_name = ""; c_function = "";
c_optarg_prefix = "";
                  non_c_aliases = [] }
 
diff --git a/generator/c.ml b/generator/c.ml
index ac8fd5e..68f5f46 100644
--- a/generator/c.ml
+++ b/generator/c.ml
@@ -1069,14 +1069,36 @@ trace_send_line (guestfs_h *g)
   (* 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 } +              
config_only = config_only;
+                                    symbol_version = symbol_version } +    (*
GCC symver won't let us just use the external name of the
+     * function if the function has multiple versions.  Instead we
+     * have to give the function a unique, non-exported name
+     * ('latest_guestfs_...').
+     *)
+    let prefix = match symbol_version with
+      | None -> "guestfs_"
+      | Some _ -> "latest_guestfs_" in
+    if prefix <> "guestfs_" then (
+      if optargs = [] then
+        generate_prototype ~extern:true ~single_line:true ~newline:true
+          ~handle:"g" ~prefix
+          c_name style
+      else
+        generate_prototype ~extern:true ~single_line:true ~newline:true
+          ~handle:"g" ~prefix
+          ~suffix:"_argv" ~optarg_proto:Argv
+          c_name style
+    );
+
     if optargs = [] then
       generate_prototype ~extern:false ~semicolon:false ~newline:true
-        ~handle:"g" ~prefix:"guestfs_"
+        ~handle:"g" ~prefix
         c_name style
     else
       generate_prototype ~extern:false ~semicolon:false ~newline:true
-        ~handle:"g" ~prefix:"guestfs_"
~suffix:"_argv" ~optarg_proto:Argv
+        ~handle:"g" ~prefix
+        ~suffix:"_argv" ~optarg_proto:Argv
         c_name style;
     pr "{\n";
 
@@ -1140,20 +1162,43 @@ trace_send_line (guestfs_h *g)
 
   (* Client-side stubs for each function. *)
   let generate_daemon_stub { name = name; c_name = c_name;
-                             style = ret, args, optargs as style } +           
style = ret, args, optargs as style;
+                             symbol_version = symbol_version }      let errcode
match errcode_of_ret ret with
       | `CannotReturnError -> assert false
       | (`ErrorIsMinusOne | `ErrorIsNULL) as e -> e in
 
+    (* GCC symver won't let us just use the external name of the
+     * function if the function has multiple versions.  Instead we
+     * have to give the function a unique, non-exported name
+     * ('latest_guestfs_...').
+     *)
+    let prefix = match symbol_version with
+      | None -> "guestfs_"
+      | Some _ -> "latest_guestfs_" in
+    if prefix <> "guestfs_" then (
+      if optargs = [] then
+        generate_prototype ~extern:true ~single_line:true ~newline:true
+          ~handle:"g" ~prefix
+          c_name style
+      else
+        generate_prototype ~extern:true ~single_line:true ~newline:true
+          ~handle:"g" ~prefix
+          ~suffix:"_argv" ~optarg_proto:Argv
+          c_name style
+    );
+
     (* Generate the action stub. *)
     if optargs = [] then
       generate_prototype ~extern:false ~semicolon:false ~newline:true
-        ~handle:"g" ~prefix:"guestfs_" c_name style
+        ~handle:"g" ~prefix
+        c_name style
     else
       generate_prototype ~extern:false ~semicolon:false ~newline:true
-        ~handle:"g" ~prefix:"guestfs_"
~suffix:"_argv"
-        ~optarg_proto:Argv c_name style;
+        ~handle:"g" ~prefix
+        ~suffix:"_argv" ~optarg_proto:Argv
+        c_name style;
 
     pr "{\n";
 
@@ -1578,7 +1623,12 @@ trace_send_line (guestfs_h *g)
     | ({ style = _, _, (_::_); once_had_no_optargs = true } as f) ->
       generate_va_variants f;
       generate_back_compat_wrapper f
-  ) all_functions_sorted
+  ) all_functions_sorted;
+
+  pr "/* Include backwards compatibility code for old ABIs. */\n";
+  pr "#include \"compat.c\"\n";
+  pr "\n";
+  pr "/* EOF */\n"
 
 (* Generate the linker script which controls the visibility of
  * symbols in the public ABI and ensures no other symbols get
@@ -1646,14 +1696,40 @@ and generate_linker_script ()      ) in
   let globals = List.sort compare (globals @ functions @ structs) in
 
-  pr "{\n";
+  pr "GUESTFS_0.0 {\n";
   pr "    global:\n";
   List.iter (pr "        %s;\n") globals;
   pr "\n";
 
   pr "    local:\n";
   pr "        *;\n";
-  pr "};\n"
+  pr "};\n";
+
+  (* Explicitly versioned symbols. *)
+  let h = Hashtbl.create 13 in
+  List.iter (
+    function
+    | { name = name; symbol_version = Some ver } ->
+      let names = try Hashtbl.find h ver with Not_found -> [] in
+      Hashtbl.replace h ver (name :: names)
+    | { symbol_version = None } -> ()
+  ) all_functions;
+  let vers = Hashtbl.fold (fun k _ ks -> k :: ks) h [] in
+  let vers = List.sort compare vers in
+  let prev = ref "GUESTFS_0.0" in
+  List.iter (
+    fun ver ->
+      let names = Hashtbl.find h ver in
+      let names = List.sort compare names in
+
+      pr "\n";
+      pr "%s {\n" ver;
+      pr "    global:\n";
+      List.iter (pr "        guestfs_%s;\n") names;
+      pr "} %s;\n" !prev;
+
+      prev := ver
+  ) vers
 
 and generate_max_proc_nr ()    pr "%d\n" max_proc_nr
diff --git a/generator/checks.ml b/generator/checks.ml
index 2cccf26..f813fbf 100644
--- a/generator/checks.ml
+++ b/generator/checks.ml
@@ -243,6 +243,16 @@ let ()      | { blocking = true } -> ()
   ) daemon_functions;
 
+  (* Check symbol version string is sane. *)
+  List.iter (
+    function
+    | { name = name; symbol_version = Some ver } ->
+      let len = String.length ver in
+      if len < 12 || String.sub ver 0 10 <> "GUESTFS_1."
then
+        failwithf "%s: invalid symbol_version (%s)" name ver
+    | { symbol_version = None } -> ()
+  ) all_functions;
+
   (* Non-fish functions must have correct camel_name. *)
   List.iter (
     fun { name = name; camel_name = camel_name } ->
diff --git a/generator/types.ml b/generator/types.ml
index cff3c6d..df019e8 100644
--- a/generator/types.ml
+++ b/generator/types.ml
@@ -399,6 +399,8 @@ type action = {
                                      set flags in the handle are marked
                                      non-blocking so that we don't add
                                      machinery in various bindings. *)
+  symbol_version : string option; (* C symbol version.
+                                     See guestfs(3)/SYMBOL VERSIONING *)
 
   (* "Internal" data attached by the generator at various stages. 
This
    * doesn't need to (and shouldn't) be set when defining actions.
diff --git a/src/Makefile.am b/src/Makefile.am
index 5f80bc1..ce2398f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -45,6 +45,7 @@ EXTRA_DIST = \
 	libguestfs.3 \
 	libguestfs.pc libguestfs.pc.in \
 	guestfs.pod \
+	compat.c \
 	api-support/added \
 	api-support/README \
 	api-support/update-from-tarballs.sh
diff --git a/src/compat.c b/src/compat.c
new file mode 100644
index 0000000..119eb7c
--- /dev/null
+++ b/src/compat.c
@@ -0,0 +1,25 @@
+/* libguestfs
+ * Copyright (C) 2012 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* This is used for symbol versioning.  See guestfs(3)/SYMBOL VERSIONING.
+ *
+ * XXX Symbol versioning only seems to work if all the related
+ * functions are compiled into a single file, so this file is
+ * #included directly into src/actions.c, instead of being compiled as
+ * a separate unit.
+ */
diff --git a/src/guestfs.pod b/src/guestfs.pod
index 6b119dc..8e3baea 100644
--- a/src/guestfs.pod
+++ b/src/guestfs.pod
@@ -3481,6 +3481,32 @@ into the appliance.
 Debugging messages are never translated, since they are intended for
 the programmers.
 
+=head2 SYMBOL VERSIONING
+
+The generator supports symbol versioning.  This is used
+B<as a last resort only> when we need to modify an API and we
+cannot possibly make the change ABI compatible.  Using symbol
+versioning allows us to get older applications to transparently use a
+compatibility function (preserving ABI) while newly compiled
+applications get the new API.
+
+First, familiarize yourself with symbol versioning by reading the
+relevant sections of the GNU ld documentation and this document by
+Ulrich Drepper: L<http://www.akkadia.org/drepper/dsohowto.pdf>
+
+The modified API should have a C<symbol_version> added.  This has the
+form C<GUESTFS_1.X> where C<1.X> is the first stable version of
+libguestfs where the new, incompatible API will appear.
+
+Next edit C<src/compat.c> and add the relevant C<.symver>
directives
+so that old libraries call a translation function that is backwards
+compatible with the old ABI, and to make the new symbol the default
+for newly compiled code.  There are examples in this file.
+
+Finally check that old binaries do not crash and still return the same
+data.  It's a good idea to add a C<debug> call to the translation
+function so you can be sure it is being called.
+
 =head2 SOURCE CODE SUBDIRECTORIES
 
 =over 4
-- 
1.7.11.4

From: "Richard W.M. Jones" <rjones at redhat.com>

This breaks the ABI, but by using symbol versioning existing callers
will not be affected.  Also this change is source compatible, so
existing code does not need to be modified.
---
 generator/actions.ml |  1 +
 generator/structs.ml |  5 ++++
 src/compat.c         | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/inspect-apps.c   |  5 ++++
 4 files changed, 80 insertions(+)

diff --git a/generator/actions.ml b/generator/actions.ml
index e1db3db..3b0806a 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -1522,6 +1522,7 @@ Please read L<guestfs(3)/INSPECTION> for more
details." };
   { defaults with
     name = "inspect_list_applications";
     style = RStructList ("applications", "application"),
[Device "root"], [];
+    symbol_version = Some "GUESTFS_1.20";
     shortdesc = "get list of applications installed in the operating
system";
     longdesc = "\
 Return the list of applications installed in the operating system.
diff --git a/generator/structs.ml b/generator/structs.ml
index d62fcc5..c6158af 100644
--- a/generator/structs.ml
+++ b/generator/structs.ml
@@ -183,6 +183,7 @@ let structs = [
     "app_epoch", FInt32;
     "app_version", FString;
     "app_release", FString;
+    "app_arch", FString;
     "app_install_path", FString;
     "app_trans_path", FString;
     "app_publisher", FString;
@@ -190,6 +191,10 @@ let structs = [
     "app_source_package", FString;
     "app_summary", FString;
     "app_description", FString;
+    "app_spare1", FString;
+    "app_spare2", FString;
+    "app_spare3", FString;
+    "app_spare4", FString;
   ];
 
   (* ISO primary volume descriptor. *)
diff --git a/src/compat.c b/src/compat.c
index 119eb7c..c9cde2b 100644
--- a/src/compat.c
+++ b/src/compat.c
@@ -23,3 +23,72 @@
  * #included directly into src/actions.c, instead of being compiled as
  * a separate unit.
  */
+
+/* guestfs_inspect_list_applications did not return the C<app_arch>
+ * field in libguestfs < 1.20.  We cannot add fields to structs
+ * without breaking compatibility.
+ */
+struct compat118_guestfs_application {
+  char *app_name;
+  char *app_display_name;
+  int32_t app_epoch;
+  char *app_version;
+  char *app_release;
+  char *app_install_path;
+  char *app_trans_path;
+  char *app_publisher;
+  char *app_url;
+  char *app_source_package;
+  char *app_summary;
+  char *app_description;
+};
+
+struct compat118_guestfs_application_list {
+  uint32_t len;
+  struct compat118_guestfs_application *val;
+};
+
+/* Declare a prototype to make GCC happy. */
+struct compat118_guestfs_application_list
*compat118_guestfs_inspect_list_applications (guestfs_h *g, const char *root);
+
+struct compat118_guestfs_application_list *
+compat118_guestfs_inspect_list_applications (guestfs_h *g, const char *root)
+{
+  struct compat118_guestfs_application_list *ret;
+  struct guestfs_application_list *r;
+  size_t i;
+
+  debug (g, "%s: compatibility wrapper invoked", __func__);
+
+  /* Call the new function. */
+  r = guestfs_inspect_list_applications (g, root);
+  if (!r)
+    return NULL;
+
+  /* Translate the structures from the new format to the old format. */
+  ret = safe_malloc (g, sizeof (struct compat118_guestfs_application_list));
+  ret->len = r->len;
+  ret->val +    safe_malloc (g, sizeof (struct
compat118_guestfs_application) * r->len);
+  for (i = 0; i < r->len; ++i) {
+    ret->val[i].app_name = r->val[i].app_name;
+    ret->val[i].app_display_name = r->val[i].app_display_name;
+    ret->val[i].app_epoch = r->val[i].app_epoch;
+    ret->val[i].app_version = r->val[i].app_version;
+    ret->val[i].app_release = r->val[i].app_release;
+    ret->val[i].app_install_path = r->val[i].app_install_path;
+    ret->val[i].app_trans_path = r->val[i].app_trans_path;
+    ret->val[i].app_publisher = r->val[i].app_publisher;
+    ret->val[i].app_url = r->val[i].app_url;
+    ret->val[i].app_source_package = r->val[i].app_source_package;
+    ret->val[i].app_summary = r->val[i].app_summary;
+    ret->val[i].app_description = r->val[i].app_description;
+  }
+  free (r->val);                /* Must not free the strings. */
+  free (r);
+
+  return ret;
+}
+
+__asm__(".symver
compat118_guestfs_inspect_list_applications,guestfs_inspect_list_applications@");
+__asm__(".symver
latest_guestfs_inspect_list_applications,guestfs_inspect_list_applications@@GUESTFS_1.20");
diff --git a/src/inspect-apps.c b/src/inspect-apps.c
index f65c70a..54c10f4 100644
--- a/src/inspect-apps.c
+++ b/src/inspect-apps.c
@@ -555,6 +555,7 @@ add_application (guestfs_h *g, struct
guestfs_application_list *apps,
   apps->val[apps->len-1].app_epoch = epoch;
   apps->val[apps->len-1].app_version = safe_strdup (g, version);
   apps->val[apps->len-1].app_release = safe_strdup (g, release);
+  apps->val[apps->len-1].app_arch = safe_strdup (g, "");
   apps->val[apps->len-1].app_install_path = safe_strdup (g,
install_path);
   /* XXX Translated path is not implemented yet. */
   apps->val[apps->len-1].app_trans_path = safe_strdup (g, "");
@@ -566,6 +567,10 @@ add_application (guestfs_h *g, struct
guestfs_application_list *apps,
   apps->val[apps->len-1].app_source_package = safe_strdup (g,
"");
   apps->val[apps->len-1].app_summary = safe_strdup (g, "");
   apps->val[apps->len-1].app_description = safe_strdup (g, description);
+  apps->val[apps->len-1].app_spare1 = safe_strdup (g, "");
+  apps->val[apps->len-1].app_spare2 = safe_strdup (g, "");
+  apps->val[apps->len-1].app_spare3 = safe_strdup (g, "");
+  apps->val[apps->len-1].app_spare4 = safe_strdup (g, "");
 }
 
 /* Sort applications by name before returning the list. */
-- 
1.7.11.4

"Richard W.M. Jones" <rjones at redhat.com>
writes:
> John,
>
> This was my attempt to add symbol versioning to the library,
> letting us break ABI without breaking any existing callers.
>
> Unfortunately it doesn't work:
>
>  - the new versioned symbols are marked local in libguestfs.so
>
>  - the existing symbols should now have @GUESTFS_0.0 versions,
>    but don't
>
> The documentation for this stuff is extremely thin, and I've
> got a bad case of influenza at the moment.
>
> Rich.
Thanks for posting this.  After skimming this I decided I needed a crash
course on OCaml, so I spent the majority of today doing just that.  I
think I've wrapped my head around it well enough (past elisp experience
helped!) and I'll be looking at this with a slightly more educated eye
in the morning.

Good luck fighting the influenza, and hope you feel better!