[libvirt] [PATCH v8 1/8] parallels: add driver skeleton

[Dmitry Guryanov]

On 07/10/2012 06:53 PM, Peter Krempa wrote:
> On 07/05/12 12:58, Dmitry Guryanov wrote:
>
>> Parallels Virtuozzo Server is a cloud-ready virtualization
>> solution that allows users to simultaneously run multiple virtual
>> machines and containers on the same physical server.
>>
>> Current name of this product is Parallels Server Bare Metal and
....
>> +static int
>> +parallelsGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned 
>> long *hvVer)
>> +{
>> +    /* TODO */
>> +    *hvVer = 6;
>
> I hope this hack gets updated in a patch later on. Also this produces 
> following output:
>
> virsh # version
> Compiled against library: libvir 0.9.13
> Using library: libvir 0.9.13
> Using API: PARALLELS 0.9.13
> Running hypervisor: PARALLELS 0.0.6
>
>
Yes, I'll fix this soon.


>> +    return 0;
>> +}
>> +
>> +static virDriver parallelsDriver = {
>> +    .no = VIR_DRV_PARALLELS,
>> +    .name = "PARALLELS",
>> +    .open = parallelsOpen,            /* 0.10.0 */
>> +    .close = parallelsClose,          /* 0.10.0 */
>> +    .version = parallelsGetVersion,   /* 0.10.0 */
>> +    .getHostname = virGetHostname,      /* 0.10.0 */
>> +    .nodeGetInfo = nodeGetInfo,      /* 0.10.0 */
>> +    .getCapabilities = parallelsGetCapabilities,      /* 0.10.0 */
>> +};
>> +
>> +/**
>> + * parallelsRegister:
>> + *
>> + * Registers the parallels driver
>> + */
>> +int
>> +parallelsRegister(void)
>> +{
>> +    char *prlctl_path;
>> +
>> +    prlctl_path = virFindFileInPath(PRLCTL);
>> +    if (!prlctl_path) {
>> +        parallelsError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                 _("Can't find prlctl command in the PATH env"));
>> +        return VIR_DRV_OPEN_ERROR;
>> +    }
>
> Memory leak: virFindFileInPath states:
> /*
>  * Finds a requested executable file in the PATH env. e.g.:
>  * "kvm-img" will return "/usr/bin/kvm-img"
>  *
>  * You must free the result
>  */
> char *virFindFileInPath(const char *file)
>
> VIR_FREE(prctl_path)
>
> Also this piece of code is somewhat user-unfriendly. If you don't have 
> the prlctl command, the driver is not registered and for some reason 
> the error message is not present in the logs.

Eric Blake suggested moving this check from parallelsOpen to 
parallelsRegister, 
http://www.redhat.com/archives/libvir-list/2012-May/msg00026.html.

>
>> +
>> +    if (virRegisterDriver(&parallelsDriver) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> diff --git a/src/parallels/parallels_driver.h 
>> b/src/parallels/parallels_driver.h
>> new file mode 100644
>> index 0000000..c04db35
>> --- /dev/null
>> +++ b/src/parallels/parallels_driver.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * parallels_driver.c: core driver functions for managing
>> + * Parallels Virtuozzo Server hosts
>> + *
>> + * Copyright (C) 2012 Parallels, Inc.
>> + *
>> + * 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, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 
>> 02111-1307  USA
>> + *
>> + */
>> +
>> +#ifndef PARALLELS_DRIVER_H
>> +# define PARALLELS_DRIVER_H
>> +
>> +
>> +# include "domain_conf.h"
>> +# include "storage_conf.h"
>> +# include "domain_event.h"
>> +
>> +# define parallelsError(code, 
>> ...)                                         \
>> +        virReportErrorHelper(VIR_FROM_TEST, code, __FILE__,         \
>
> s/VIR_FROM_TEST/VIR_FROM_THIS/
>
>> + __FUNCTION__, __LINE__, __VA_ARGS__)
>> +# define PRLCTL      "prlctl"
>> +
>> +
>> +struct _parallelsConn {
>> +    virMutex lock;
>> +    virDomainObjList domains;
>> +    virStoragePoolObjList pools;
>> +    virCapsPtr caps;
>> +    virDomainEventStatePtr domainEventState;
>> +};
>> +
>> +typedef struct _parallelsConn parallelsConn;
>> +
>> +typedef struct _parallelsConn *parallelsConnPtr;
>
> All of the above definitions belong to the driver .c file as we don't 
> want to expose the internals.
>
>> +
>> +int parallelsRegister(void);
>> +
>> +#endif
>> diff --git a/src/util/virterror.c b/src/util/virterror.c
>> index cb37be0..7c0119f 100644
>> --- a/src/util/virterror.c
>> +++ b/src/util/virterror.c
>> @@ -99,7 +99,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
>>
>>                 "URI Utils", /* 45 */
>>                 "Authentication Utils",
>> -              "DBus Utils"
>> +              "DBus Utils",
>> +              "Parallels Cloud Server"
>>       )
>
> I'm attaching a patch that contains my findings. I'm inclining to 
> giving an ACK to this patch with the changes I suggested, but I'd be 
> glad if somebody else could have a quick look. Let's see how the rest 
> of the series works.
>
> Peter
>

Thanks, I agree with all your comments and fixes.

-- 
Dmitry Guryanov