2014-07-02 17:12 GMT+08:00 Michal Privoznik <mpriv...@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 (uaeda...@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 (uaeda...@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 */ >> >> >
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list