3:28 p.m.
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(¶llelsDriver) < 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