[libvirt] [PATCHv4 3/4]vbox: Use vboxUniformedAPI to write common code
Taowei Luo
uaedante at gmail.com
Wed Jul 2 13:45:12 UTC 2014
2014-07-02 17:12 GMT+08:00 Michal Privoznik <mprivozn at 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 at 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 at 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 */
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140702/5792be59/attachment-0001.htm>
More information about the libvir-list
mailing list