[libvirt] [PATCH] outline of writing vbox driver

[Taowei Luo]

2014-06-20 17:09 GMT+08:00 Michal Privoznik <mprivozn at redhat.com>:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140620/1bf2fc4c/attachment-0001.htm>