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

Peter Krempa pkrempa at redhat.com
Tue Jul 10 20:28:19 UTC 2012


On 07/10/12 20:50, Dmitry Guryanov wrote:
> 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.

Ok. In this case, I'm fine with a temporary solution.

>
>
>>> +    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.

I see. Yes I agree with Eric on this, but we'll have to find out why the 
error message doesn't get logged. I wanted to test the driver overall 
and I just got rejected without any sign of what is wrong and it seemed 
as if the driver wasn't even compiled.

I'll have a look at it while reviewing the rest of the series (that will 
hopefully go faster as this first patch. I'm not as skilled as Eric in 
reviews and I just don't want to make mistakes :) )

>
>>
>>> +
>>> +    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.
>

Let's see how the rest of the series works out. If there won't be 
anything serious by design I'll fix the nits and amend your commits.

Peter




More information about the libvir-list mailing list