Libguestfs - Jan 2012 - [PATCH 1/2] gobject: Allow RConstOptString to return an error

RConstOptString cannot return an error in the C api. This makes it a special
case for the GObject api, as all other return types have a corresponding GError
**err argument to return an error. This change removes this special case, and
includes the possibility of an error return in the API. An error is indicated by
setting *err to a non-NULL value.

This change is in preparation for adding a close api. An attempt to call any
api, even RConstOptString, on a closed handle must return an error.
---
 generator/generator_gobject.ml |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/generator/generator_gobject.ml b/generator/generator_gobject.ml
index 4779f23..2b88274 100644
--- a/generator/generator_gobject.ml
+++ b/generator/generator_gobject.ml
@@ -37,10 +37,6 @@ let camel_of_name flags name          a ^ String.uppercase
(Str.first_chars b 1) ^ Str.string_after b 1
     ) "" (Str.split (regexp "_") name)
 
-let returns_error = function
-  | RConstOptString _ -> false
-  | _ -> true
-
 let generate_gobject_proto name ?(single_line = true)
                                 (ret, args, optargs) flags    let spacer = if
single_line then " " else "\n" in
@@ -109,7 +105,7 @@ let generate_gobject_proto name ?(single_line = true)
   | _ -> ());
   if List.exists (function Cancellable -> true | _ -> false) flags then
     pr ", GCancellable *cancellable";
-  if returns_error ret then pr ", GError **err";
+  pr ", GError **err";
   pr ")"
 
 let generate_gobject_header_static () @@ -780,7 +776,7 @@ let
generate_gobject_c_methods ()
       (* Check return, throw error if necessary, marshall return value *)
 
-      if returns_error ret then (
+      if match ret with RConstOptString _ -> false | _ -> true then (
         pr "  if (ret == %s) {\n"
           (match ret with
           | RErr | RInt _ | RInt64 _ | RBool _ ->
-- 
1.7.7.5

This change binds guestfs_close(). It consequently results in RConstOptString
being able to throw an error.
---
 generator/generator_gobject.ml |   79 ++++++++++++++++++++++++++++------------
 gobject/run-bindtests          |    1 +
 gobject/tests-misc.js          |   41 +++++++++++++++++++++
 3 files changed, 97 insertions(+), 24 deletions(-)
 create mode 100644 gobject/tests-misc.js

diff --git a/generator/generator_gobject.ml b/generator/generator_gobject.ml
index 2b88274..b005dfe 100644
--- a/generator/generator_gobject.ml
+++ b/generator/generator_gobject.ml
@@ -156,6 +156,7 @@ struct _GuestfsSessionClass
 
 GType guestfs_session_get_type(void);
 GuestfsSession *guestfs_session_new(void);
+gboolean guestfs_session_close(GuestfsSession *session, GError **err);
 
 /* Guestfs::Tristate */
 typedef enum
@@ -304,6 +305,16 @@ let generate_gobject_c_static ()   * images.
  */
 
+/* Error quark */
+
+#define GUESTFS_ERROR guestfs_error_quark()
+
+static GQuark
+guestfs_error_quark(void)
+{
+  return g_quark_from_static_string(\"guestfs\");
+}
+
 #define GUESTFS_SESSION_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE ( \
                                             (obj), \
                                             GUESTFS_TYPE_SESSION, \
@@ -357,6 +368,29 @@ guestfs_session_new(void)
   return GUESTFS_SESSION(g_object_new(GUESTFS_TYPE_SESSION, NULL));
 }
 
+/**
+ * guestfs_session_close:
+ *
+ * Close a libguestfs session.
+ *
+ * Returns: true on success, false on error
+ */
+gboolean
+guestfs_session_close(GuestfsSession *session, GError **err)
+{
+  guestfs_h *g = session->priv->g;
+
+  if (g == NULL) {
+    g_set_error_literal(err, GUESTFS_ERROR, 0, \"Session is already
closed\");
+    return FALSE;
+  }
+
+  guestfs_close(g);
+  session->priv->g = NULL;
+
+  return TRUE;
+}
+
 /* Guestfs::Tristate */
 GType
 guestfs_tristate_get_type(void)
@@ -374,16 +408,6 @@ guestfs_tristate_get_type(void)
   return etype;
 }
 
-/* Error quark */
-
-#define GUESTFS_ERROR guestfs_error_quark()
-
-static GQuark
-guestfs_error_quark(void)
-{
-  return g_quark_from_static_string(\"guestfs\");
-}
-
 /* Cancellation handler */
 static void
 cancelled_handler(gpointer data)
@@ -598,7 +622,15 @@ let generate_gobject_c_methods ()        let doc =
String.concat "\n * " doc in
       let camel_name = camel_of_name flags name in
       let is_RBufferOut = match ret with RBufferOut _ -> true | _ ->
false in
-      let error_return = match ret with
+      let guestfs_error_return = match ret with
+      | RErr | RInt _ | RInt64 _ | RBool _ ->
+        "-1"
+      | RConstString _ | RString _ | RStringList _ | RHashtable _
+      | RBufferOut _ | RStruct _ | RStructList _ ->
+        "NULL"
+      | RConstOptString _ -> "" (* We don't check
RConstOptString for error *)
+      in
+      let gobject_error_return = match ret with
       | RErr ->
         "FALSE"
       | RInt _ | RInt64 _ | RBool _ ->
@@ -606,7 +638,9 @@ let generate_gobject_c_methods ()        | RConstString _ |
RString _ | RStringList _ | RHashtable _
       | RBufferOut _ | RStruct _ | RStructList _ ->
         "NULL"
-      | RConstOptString _ -> ""
+      | RConstOptString _ ->
+        "NULL" (* NULL is a valid return for RConstOptString. Error
is
+                  indicated by also setting *err to a non-NULL value *)
       in
 
       (* The comment header, including GI annotations for arguments and the
@@ -684,10 +718,16 @@ let generate_gobject_c_methods ()        if cancellable
then (
         pr "  /* Check we haven't already been cancelled */\n";
         pr "  if (g_cancellable_set_error_if_cancelled (cancellable,
err))\n";
-        pr "    return %s;\n\n" error_return;
+        pr "    return %s;\n\n" gobject_error_return;
       );
 
+      (* Get the guestfs handle, and ensure it isn't closed *)
+
       pr "  guestfs_h *g = session->priv->g;\n";
+      pr "  if (g == NULL) {\n";
+      pr "    g_set_error_literal(err, GUESTFS_ERROR, 0, \"Attempt to
call %s after the session has been closed\");\n" name;
+      pr "    return %s;\n" gobject_error_return;
+      pr "  }\n\n";
 
       (* Optargs *)
 
@@ -777,18 +817,9 @@ let generate_gobject_c_methods ()        (* Check return,
throw error if necessary, marshall return value *)
 
       if match ret with RConstOptString _ -> false | _ -> true then (
-        pr "  if (ret == %s) {\n"
-          (match ret with
-          | RErr | RInt _ | RInt64 _ | RBool _ ->
-            "-1"
-          | RConstString _ | RString _ | RStringList _ | RHashtable _
-          | RBufferOut _ | RStruct _ | RStructList _ ->
-            "NULL"
-          | RConstOptString _ ->
-            assert false;
-          );
+        pr "  if (ret == %s) {\n" guestfs_error_return;
         pr "    g_set_error_literal(err, GUESTFS_ERROR, 0,
guestfs_last_error(g));\n";
-        pr "    return %s;\n" error_return;
+        pr "    return %s;\n" gobject_error_return;
         pr "  }\n";
       );
       pr "\n";
diff --git a/gobject/run-bindtests b/gobject/run-bindtests
index 55c489c..d2ce0a6 100755
--- a/gobject/run-bindtests
+++ b/gobject/run-bindtests
@@ -26,3 +26,4 @@ fi
 ../run $GJS $srcdir/bindtests.js > bindtests.tmp
 diff -u ${srcdir}/../bindtests bindtests.tmp
 ../run $GJS $srcdir/bindtests-manual.js 2>/dev/null
+../run $GJS $srcdir/tests-misc.js 2>/dev/null
diff --git a/gobject/tests-misc.js b/gobject/tests-misc.js
new file mode 100644
index 0000000..aadc2f3
--- /dev/null
+++ b/gobject/tests-misc.js
@@ -0,0 +1,41 @@
+// libguestfs miscellaneous gobject binding tests
+// Copyright (C) 2012 Red Hat Inc.
+//
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 2 of the License, or
+// (at your option) any later version.
+//
+// This program 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 General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program; if not, write to the Free Software Foundation, Inc.,
+// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+const Guestfs = imports.gi.Guestfs;
+
+var fail = false;
+
+var g = new Guestfs.Session();
+
+// Test close()
+g.close();
+var threw = false;
+try {
+  var v = g.test0rconstoptstring('1');
+} catch (error) {
+  threw = true;
+  if (!error.message.match(/closed/)) {
+    print("call after close threw unexpected error: " +
error.message);
+    fail = true;
+  }
+}
+if (!threw) {
+  print("call after closed failed to throw an error");
+  fail = true;
+}
+
+fail ? 1 : 0;
-- 
1.7.7.5

On Wed, Jan 25, 2012 at 05:19:41PM +0000, Matthew Booth
wrote:
> @@ -780,7 +776,7 @@ let generate_gobject_c_methods () >  
>        (* Check return, throw error if necessary, marshall return value *)
>  
> -      if returns_error ret then (
> +      if match ret with RConstOptString _ -> false | _ -> true then
(
>          pr "  if (ret == %s) {\n"
>            (match ret with
>            | RErr | RInt _ | RInt64 _ | RBool _ ->
This is a bit of a mouthful.  You can just write:

  match ret with
  | RConstOptString _ -> ()
  | _ ->
    pr (* etc *)

However the rest of the patch is fine, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora