Re: [PATCH] outline of writing vbox driver

[Taowei Luo <uaedante@xxxxxxxxx>]

2014-06-20 17:09 GMT+08:00 Michal Privoznik <mprivozn@xxxxxxxxxx>:

On 17.06.2014 13:06, Taowei wrote:

Define the vboxUniformedAPI struct to handle version conflicts.
The vboxInitialize is rewrited with the new mechanism. Other
functions would be rewriting in the same way.

s/rewriting/rewritten/

Here, I still use template to generate functions in vboxUniformedAPI.
Though, these functions may change between different versions, but
not for every version. Using template could decrease the duplicated code.

For every new feature added by vbox, a flag would indicate whether this
feature is supported in current version. Calling for an unsupported
feature would lead to a VIR_WARN.

---
  src/vbox/vbox_tmpl.c |   84 ++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 61 insertions(+), 23 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 1ed2729..4625805 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -826,6 +826,65 @@ static PRUnichar *PRUnicharFromInt(int n) {

  #endif /* !(VBOX_API_VERSION == 2002000) */

+/* Begin of vboxUniformedAPI */
+
+#define UNUSED(expr) do { (void)(expr); } while (0)

We have ATTRIBUTE_UNUSED which can serve this purpose. Or ignore_value() depending on the use case.

+
+static void _pfnComInitialize(vboxGlobalData *data)
+{
+#if VBOX_XPCOMC_VERSION == 0x00010000U
+    data->pFuncs->pfnComInitialize(&data->vboxObj, &data->vboxSession);
+#else  /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
+    data->pFuncs->pfnComInitialize(IVIRTUALBOX_IID_STR, &data->vboxObj, ISESSION_IID_STR, &data->vboxSession);
+#endif /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
+}
+
+#if (VBOX_XPCOMC_VERSION == 0x00010000U) || (VBOX_API_VERSION == 2002000)
+    #define FWATCH_NEED_INITICAL 0
+#else /* (VBOX_XPCOMC_VERSION != 0x00010000U && VBOX_API_VERSION != 2002000) */
+    #define FWATCH_NEED_INITICAL 1
+#endif /* (VBOX_XPCOMC_VERSION != 0x00010000U && VBOX_API_VERSION != 2002000) */
+
+static int _initicalFWatch(vboxGlobalData *data)

so this would be:
static int
_initicalFWatch(vboxGlobalData *data ATTRIBUTE_UNUSED)
{ ... }

even though the @data might be used later (depending on VBOX version).

+{
+#if FWATCH_NEED_INITICAL == 0

What's the reason for not having:

#ifdef VBOX_XPCOMC_VERSION .. || VBOX_API_VERSION ..

#else

#endinf

I don't think I see why you need to invent this new preprocessor symbol.

 

The fWatchNeedInitial is a flag that indicates whether we need to initial fdWatch and vboxQueue in vboxGlobalData. VBox2.2 has no event call back, so we don't need to call it.

I expect that others would know whether they need to initialize the fdWatch, so the flag and the fWatchNeedInitial is introduced.

Do you mean just use VBOX_XPCOMC_VERSION and VBOX_API_VERSION to check directly?

I will modify the next codes with your advise. 

+    /* No event queue functionality in 2.2.* as of now */
+    UNUSED(data);
+    VIR_WARN("There is no fWatch initical in current version");
+#else /* FWATCH_NEED_INITICAL != 0 */
+    /* Initial the fWatch needed for Event Callbacks */
+    data->fdWatch = -1;
+    data->pFuncs->pfnGetEventQueue(&data->vboxQueue);
+    if (data->vboxQueue == NULL) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("nsIEventQueue object is null"));
+        return -1;
+    }
+#endif /* FWATCH_NEED_INITICAL != 0 */
+    return 0;
+}
+
+typedef struct {
+    /* vbox API version */
+    uint32_t uVersion;
+    /* vbox APIs */
+    void  (*pfnComInitialize)(vboxGlobalData *data);
+    int (*initicalFWatch)(vboxGlobalData *data);
+    /* vbox API features */
+    unsigned fWatchNeedInitical : 1;

s /unsigned .. :1/bool/

And what does Initical stands for anyway?

+} vboxUniformedAPI;
+
+static vboxUniformedAPI vboxAPI = {
+    .uVersion = VBOX_API_VERSION,
+    .pfnComInitialize = _pfnComInitialize,
+    .initicalFWatch = _initicalFWatch,
+    .fWatchNeedInitical = FWATCH_NEED_INITICAL,
+};
+
+static vboxUniformedAPI *pVboxAPI = &vboxAPI;
+
+/* End of vboxUniformedAPI and Begin of common codes */
+
  static PRUnichar *
  vboxSocketFormatAddrUtf16(vboxGlobalData *data, virSocketAddrPtr addr)
  {
@@ -923,31 +982,10 @@ vboxInitialize(vboxGlobalData *data)
      if (data->pFuncs == NULL)
          goto cleanup;

-#if VBOX_XPCOMC_VERSION == 0x00010000U
-    data->pFuncs->pfnComInitialize(&data->vboxObj, &data->vboxSession);
-#else  /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
-    data->pFuncs->pfnComInitialize(IVIRTUALBOX_IID_STR, &data->vboxObj,
-                               ISESSION_IID_STR, &data->vboxSession);
-
-# if VBOX_API_VERSION == 2002000
-
-    /* No event queue functionality in 2.2.* as of now */
-
-# else  /* !(VBOX_API_VERSION == 2002000) */
+    pVboxAPI->pfnComInitialize(data);

-    /* Initial the fWatch needed for Event Callbacks */
-    data->fdWatch = -1;
-
-    data->pFuncs->pfnGetEventQueue(&data->vboxQueue);
-
-    if (data->vboxQueue == NULL) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("nsIEventQueue object is null"));
+    if (pVboxAPI->fWatchNeedInitical && pVboxAPI->initicalFWatch(data) != 0)
          goto cleanup;
-    }
-
-# endif /* !(VBOX_API_VERSION == 2002000) */
-#endif /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */

      if (data->vboxObj == NULL) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list