8:45 a.m.
2014-07-02 17:12 GMT+08:00 Michal Privoznik <mprivozn(a)redhat.com>:
On 26.06.2014 15:51, Taowei wrote:
> In vbox_common.c:
> vboxInitialize and vboxDomainSave are rewrited with vboxUniformedAPI.
>
> In vbox_common.h
> Some common definitions in vbox_CAPI_v*.h are directly extracted to
> this file. Some other incompatible defintions are simplified here. So we
> can write common code with it.
>
> ---
> po/POTFILES.in | 1 +
> src/Makefile.am | 1 +
> src/vbox/vbox_common.c | 150 ++++++++++++++++++++++++++++++
> +++++++++++++++++
> src/vbox/vbox_common.h | 151 ++++++++++++++++++++++++++++++
> ++++++++++++++++++
> 4 files changed, 303 insertions(+)
> create mode 100644 src/vbox/vbox_common.c
> create mode 100644 src/vbox/vbox_common.h
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 31a8381..8c1b712 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -213,6 +213,7 @@ src/util/virxml.c
> src/vbox/vbox_MSCOMGlue.c
> src/vbox/vbox_XPCOMCGlue.c
> src/vbox/vbox_driver.c
> +src/vbox/vbox_common.c
> src/vbox/vbox_snapshot_conf.c
> src/vbox/vbox_tmpl.c
> src/vmware/vmware_conf.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index c1e3f45..7a935e5 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -674,6 +674,7 @@ VBOX_DRIVER_SOURCES =
> \
> vbox/vbox_V4_2_20.c vbox/vbox_CAPI_v4_2_20.h \
> vbox/vbox_V4_3.c vbox/vbox_CAPI_v4_3.h \
> vbox/vbox_V4_3_4.c vbox/vbox_CAPI_v4_3_4.h \
> + vbox/vbox_common.c vbox/vbox_common.h \
> vbox/vbox_uniformed_api.h
>
> VBOX_DRIVER_EXTRA_DIST = \
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> new file mode 100644
> index 0000000..27211a0
> --- /dev/null
> +++ b/src/vbox/vbox_common.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright 2014, Taowei Luo (uaedante(a)gmail.com)
> + *
> + * 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.1 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, see
> + * <http://www.gnu.org/licenses/>;.
> + */
> +
> +#include <config.h>
> +
> +#include <unistd.h>
> +
> +#include "internal.h"
> +#include "datatypes.h"
> +#include "domain_conf.h"
> +#include "domain_event.h"
> +#include "virlog.h"
> +
> +#include "vbox_common.h"
> +#include "vbox_uniformed_api.h"
> +
> +/* Common codes for vbox driver. With the definitions in vbox_common.h,
> + * it treats vbox structs as a void*. Though vboxUniformedAPI
> + * it call vbox functions. This file is a high level implement about
> + * the vbox driver.
> + */
> +
> +#define VIR_FROM_THIS VIR_FROM_VBOX
> +
> +VIR_LOG_INIT("vbox.vbox_common");
> +
> +#define RC_SUCCEEDED(rc) NS_SUCCEEDED(rc.resultCode)
> +#define RC_FAILED(rc) NS_FAILED(rc.resultCode)
> +
> +#define VBOX_RELEASE(arg)
> \
> + do {
> \
> + if (arg) {
> \
> + pVBoxAPI->nsisupportsRelease((void *)arg);
> \
> + (arg) = NULL;
> \
> + }
> \
> + } while (0)
> +
> +#define VBOX_OBJECT_CHECK(conn, type, value) \
> +vboxGlobalData *data = conn->privateData;\
> +type ret = value;\
> +if (!data->vboxObj) {\
> + return ret;\
> +}
> +
> +static vboxUniformedAPI *pVBoxAPI;
> +
> +void vboxRegisterUniformedAPI(vboxUniformedAPI *vboxAPI)
> +{
> + VIR_DEBUG("VirtualBox Uniformed API has been registered");
> + pVBoxAPI = vboxAPI;
> +}
> +
> +int vboxInitialize(vboxGlobalData *data)
> +{
> + if (pVBoxAPI->pfnInitialize(data) != 0)
> + goto cleanup;
> +
> + if (pVBoxAPI->fWatchNeedInitialize &&
pVBoxAPI->initializeFWatch(data)
> != 0)
> + goto cleanup;
> +
> + if (data->vboxObj == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("IVirtualBox object is null"));
> + goto cleanup;
> + }
> +
> + if (data->vboxSession == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("ISession object is null"));
> + goto cleanup;
> + }
> +
> + return 0;
> +
> + cleanup:
> + return -1;
> +}
> +
> +int vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
> +{
> + VBOX_OBJECT_CHECK(dom->conn, int, -1);
> + IConsole *console = NULL;
> + vboxIIDUnion iid;
> + IMachine *machine = NULL;
> + nsresult rc;
> +
> + pVBoxAPI->initializeVboxIID(&iid);
> + /* VirtualBox currently doesn't support saving to a file
> + * at a location other then the machine folder and thus
> + * setting path to ATTRIBUTE_UNUSED for now, will change
> + * this behaviour once get the VirtualBox API in right
> + * shape to do this
> + */
> +
>
This down here ..
+ /* Open a Session for the machine */
> + pVBoxAPI->vboxIIDFromUUID(data, &iid, dom->uuid);
> + if (pVBoxAPI->getMachineForSession) {
> + /* Get machine for the call to VBOX_SESSION_OPEN_EXISTING */
> + rc = pVBoxAPI->objectGetMachine(data, &iid, &machine);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_NO_DOMAIN, "%s",
> + _("no domain with matching uuid"));
> + return -1;
> + }
> + }
>
.. I guess is going to be used fairly frequently, so maybe it can be
turned into a separate function.
pVBoxAPI->vboxIIDFromUUID(data, &iid, dom->uuid);
rc =
pVBoxAPI->objectGetMachine(data, &iid, &machine);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_NO_DOMAIN, "%s",
_("no domain with matching uuid"));
return -1;
}
This part indeed be used frequently, but some places check the flag
getMachineForSession while other places don't.
So the prototype would be this:
int openSessionForMachine(vboxGlobaldata *data, vboxIID *iid, unsigned
char* dom_uuid, IMachine *machine, bool checkflag);
Is this okay (or too complex)?
Meanwhile, When NS_FAILED(rc), some functions returned -1 (like this one),
but some else goto the cleanup and unalloc the
vboxIID, I perfer to make all NS_FAILED(rc) goto cleanup, what's your
opinion?
+
> + rc = pVBoxAPI->sessionOpenExisting(data, &iid, machine);
> + if (NS_SUCCEEDED(rc)) {
> + rc = pVBoxAPI->sessionGetConsole(data->vboxSession, &console);
> + if (NS_SUCCEEDED(rc) && console) {
> + IProgress *progress = NULL;
> +
> + pVBoxAPI->consoleSaveState(console, &progress);
> +
> + if (progress) {
> + resultCodeUnion resultCode;
> +
> + pVBoxAPI->progressWaitForCompletion(progress, -1);
> + pVBoxAPI->progressGetResultCode(progress, &resultCode);
> + if (RC_SUCCEEDED(resultCode)) {
> + ret = 0;
> + }
> + VBOX_RELEASE(progress);
> + }
> + VBOX_RELEASE(console);
> + }
> + pVBoxAPI->sessionClose(data->vboxSession);
> + }
>
I find this still distant to the rest of our code. I mean, we use this
pattern:
if (func() < 0)
goto cleanup;
rc = func2();
if (rc < 0)
goto cleanup;
So maybe we can use the same here:
rc = pVBoxAPI->sessionOpenExisting(data, &iid, machine);
if (NS_FAILED(rc) < 0)
goto cleanup;
rc = pVBoxAPI->sessionGetConsole(data->vboxSession, &console);
if (NS_FAILED(rc) < 0)
goto cleanup;
...
ret = 0;
cleanup:
VBOX_RELEASE(progress);
VBOX_RELEASE(console);
pVBoxAPI->sessionClose(data->vboxSession);
...
return ret;
+
> + pVBoxAPI->DEBUGIID("UUID of machine being saved:", &iid);
> +
> + VBOX_RELEASE(machine);
> + pVBoxAPI->vboxIIDUnalloc(data, &iid);
> + return ret;
> +}
> diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> new file mode 100644
> index 0000000..bf0d106
> --- /dev/null
> +++ b/src/vbox/vbox_common.h
> @@ -0,0 +1,151 @@
> +/*
> + * Copyright 2014, Taowei Luo (uaedante(a)gmail.com)
> + *
> + * 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.1 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, see
> + * <http://www.gnu.org/licenses/>;.
> + */
> +
> +#ifndef VBOX_COMMON_H
> +# define VBOX_COMMON_H
> +
> +# ifdef ___VirtualBox_CXPCOM_h
> +# error this file should not be included after vbox_CAPI_v*.h
> +# endif
> +
> +# include "internal.h"
> +# include <stddef.h>
> +# include "wchar.h"
> +
> +/* This file extracts some symbols defined in
> + * vbox_CAPI_v*.h. It tells the vbox_common.c
> + * how to treat with this symbols. This file
> + * can't be included with files such as
> + * vbox_CAPI_v*.h, or it would casue multiple
> + * definitions.
> + *
> + * You can see the more informations in vbox_api.h
> + */
> +
> +/* Copied definitions from vbox_CAPI_*.h.
> + * We must MAKE SURE these codes are compatible. */
> +
> +typedef unsigned char PRUint8;
> +# if (defined(HPUX) && defined(__cplusplus) \
> + && !defined(__GNUC__) && __cplusplus < 199707L) \
> + || (defined(SCO) && defined(__cplusplus) \
> + && !defined(__GNUC__) && __cplusplus == 1L)
> +typedef char PRInt8;
> +# else
> +typedef signed char PRInt8;
> +# endif
> +
> +# define PR_INT8_MAX 127
> +# define PR_INT8_MIN (-128)
> +# define PR_UINT8_MAX 255U
> +
> +typedef unsigned short PRUint16;
> +typedef short PRInt16;
> +
> +# define PR_INT16_MAX 32767
> +# define PR_INT16_MIN (-32768)
> +# define PR_UINT16_MAX 65535U
>
Are these really necessary? I know we already have them, I'm just asking.
+
> +typedef unsigned int PRUint32;
> +typedef int PRInt32;
> +# define PR_INT32(x) x
> +# define PR_UINT32(x) x ## U
> +
> +# define PR_INT32_MAX PR_INT32(2147483647)
> +# define PR_INT32_MIN (-PR_INT32_MAX - 1)
> +# define PR_UINT32_MAX PR_UINT32(4294967295)
> +
> +typedef long PRInt64;
> +typedef unsigned long PRUint64;
> +typedef int PRIntn;
> +typedef unsigned int PRUintn;
> +
> +typedef double PRFloat64;
> +typedef size_t PRSize;
> +
> +typedef ptrdiff_t PRPtrdiff;
> +
> +typedef unsigned long PRUptrdiff;
> +
> +typedef PRIntn PRBool;
> +
> +# define PR_TRUE 1
> +# define PR_FALSE 0
> +
> +typedef PRUint8 PRPackedBool;
> +
> +/*
> +** Status code used by some routines that have a single point of failure
> or
> +** special status return.
> +*/
> +typedef enum { PR_FAILURE = -1, PR_SUCCESS = 0 } PRStatus;
> +
> +# ifndef __PRUNICHAR__
> +# define __PRUNICHAR__
> +# if defined(WIN32) || defined(XP_MAC)
> +typedef wchar_t PRUnichar;
> +# else
> +typedef PRUint16 PRUnichar;
> +# endif
> +# endif
> +
> +typedef long PRWord;
> +typedef unsigned long PRUword;
> +
> +# define nsnull 0
> +typedef PRUint32 nsresult;
> +
> +# if defined(__GNUC__) && (__GNUC__ > 2)
> +# define NS_LIKELY(x) (__builtin_expect((x), 1))
> +# define NS_UNLIKELY(x) (__builtin_expect((x), 0))
> +# else
> +# define NS_LIKELY(x) (x)
> +# define NS_UNLIKELY(x) (x)
> +# endif
> +
> +# define NS_FAILED(_nsresult) (NS_UNLIKELY((_nsresult) & 0x80000000))
> +# define NS_SUCCEEDED(_nsresult) (NS_LIKELY(!((_nsresult) & 0x80000000)))
>
Wow, I didn't know we are using likely and unlikely in libvirt.
+
> +/**
> + * An "interface id" which can be used to uniquely identify a given
> + * interface.
> + * A "unique identifier". This is modeled after OSF DCE UUIDs.
> + */
> +
> +struct nsID {
> + PRUint32 m0;
> + PRUint16 m1;
> + PRUint16 m2;
> + PRUint8 m3[8];
> +};
> +
> +typedef struct nsID nsID;
> +typedef nsID nsIID;
> +
> +/* Simplied definitions in vbox_CAPI_*.h */
> +
> +typedef void const *PCVBOXXPCOM;
> +typedef void IVirtualBox;
> +typedef void ISession;
> +typedef void IVirtualBoxCallback;
> +typedef void nsIEventQueue;
> +typedef void IConsole;
> +typedef void IMachine;
> +typedef void IProgress;
> +
> +#endif /* VBOX_COMMON_H */
>
>