[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 3/9] pvs: add functions to list domains and get info



On 04/20/2012 10:01 AM, Dmitry Guryanov wrote:
> PVS driver is 'stateless', like vmware or openvz drivers.
> It collects information about domains during startup using
> command-line utility prlctl. VMs in PVS identified by UUIDs

s/identified/are identified/

> or unique names, which can be used as respective fields in
> virDomainDef structure. Currently only basic info, like
> description, virtual cpus number and memory amount implemented.

s/amount implemented/amount, is implemented/

> Quering devices information will be added in the next patches.

s/Quering/Querying/

> 
> PVS does't support non-persistent domains - you can't run

s/does't/doesn't/

> a domain having only disk image, it must always be registered
> in system.
> 
> Functions for quering domain info have been just copied from

s/quering/querying/

> test driver with some changes - they extract needed data from
> previouly created list of virDomainObj objects.

s/previouly/previously/

> +++ b/po/POTFILES.in
> @@ -165,6 +165,7 @@ src/xenapi/xenapi_driver.c
>  src/xenapi/xenapi_utils.c
>  src/xenxs/xen_sxpr.c
>  src/xenxs/xen_xm.c
> +src/pvs/pvs_driver.c
>  tools/console.c
>  tools/libvirt-guests.init.sh
>  tools/virsh.c

I think this hunk belongs in 1/9.

> @@ -77,6 +78,14 @@ pvsDefaultConsoleType(const char *ostype ATTRIBUTE_UNUSED)
>      return VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
>  }
>  
> +void
> +pvsFreeDomObj(void *p)
> +{
> +    pvsDomObjPtr pdom = (pvsDomObjPtr) p;
> +
> +    VIR_FREE(pdom);

Simpler to just write VIR_FREE(p) and avoid the intermediate variable.
Also, this function can safely be marked static.


> +/*
> + * Must be called with privconn->lock held
> + */
> +static virDomainObjPtr
> +pvsLoadDomain(pvsConnPtr privconn, virJSONValuePtr jobj)
> +{

Long, but looks okay.

> +/*
> + * Must be called with privconn->lock held
> + */
> +static int
> +pvsLoadDomains(pvsConnPtr privconn, const char *domain_name)
> +{
> +    int count, i;
> +    virJSONValuePtr jobj;
> +    virJSONValuePtr jobj2;
> +    virDomainObjPtr dom = NULL;
> +    int ret = -1;
> +
> +    jobj = pvsParseOutput(PRLCTL, "list", "-j", "-a",
> +                          "-i", "-H", domain_name, NULL);

I guess you can call this command with a domain name, to limit to one
output, or with no argument, to list all domains, given...

> @@ -150,6 +371,9 @@ pvsOpenDefault(virConnectPtr conn)
>      if (virDomainObjListInit(&privconn->domains) < 0)
>          goto error;
>  
> +    if (pvsLoadDomains(privconn, NULL))
> +        goto error;

this usage.


> +static int
> +pvsDomainIsPersistent(virDomainPtr dom ATTRIBUTE_UNUSED)
> +{
> +    return 1;
> +}

I'm wondering if we should at least validate that dom still exists in
our hash table.  But I can live with this implementation.

> +
> +static int
> +pvsDomainGetState(virDomainPtr domain,
> +                  int *state, int *reason, unsigned int flags)
> +{
> +    pvsConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr privdom;
> +    int ret = -1;
> +    virCheckFlags(0, -1);
> +
> +    pvsDriverLock(privconn);
> +    privdom = virDomainFindByName(&privconn->domains, domain->name);
> +    pvsDriverUnlock(privconn);
> +
> +    if (privdom == NULL) {
> +        pvsError(VIR_ERR_INVALID_ARG, __FUNCTION__);

Use of __FUNCTION__ as the error message is not a good habit, since
pvsError _already_ adds the function name automatically.  You should
instead provide a real message, such as _("Domain '%s' not found"),
domain->name.


> +static char *
> +pvsDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
> +{
> +    pvsConnPtr privconn = domain->conn->privateData;
> +    virDomainDefPtr def;
> +    virDomainObjPtr privdom;
> +    char *ret = NULL;
> +
> +    /* Flags checked by virDomainDefFormat */
> +
> +    pvsDriverLock(privconn);
> +    privdom = virDomainFindByName(&privconn->domains, domain->name);
> +    pvsDriverUnlock(privconn);
> +
> +    if (privdom == NULL) {
> +        pvsError(VIR_ERR_INVALID_ARG, __FUNCTION__);

Likewise (and probably throughout the series, so I'll quit pointing it out).

>  # define pvsError(code, ...)                                         \
>          virReportErrorHelper(VIR_FROM_TEST, code, __FILE__,         \
>                               __FUNCTION__, __LINE__, __VA_ARGS__)
>  # define PRLCTL      "prlctl"
> +# define pvsParseError()                                                          \
> +        virReportErrorHelper(VIR_FROM_TEST, VIR_ERR_OPERATION_FAILED, __FILE__,  \
> +                             __FUNCTION__, __LINE__, "Can't parse prlctl output")

Mark this string for translation: _("Can't parse prlctl output")

> +
> +struct pvsDomObj {
> +    int id;
> +    char *uuid;
> +    char *os;
> +};
>  
> +typedef struct pvsDomObj *pvsDomObjPtr;
>  
>  struct _pvsConn {
>      virMutex lock;
> @@ -48,4 +60,6 @@ typedef struct _pvsConn *pvsConnPtr;
>  
>  int pvsRegister(void);
>  
> +virJSONValuePtr pvsParseOutput(const char *binary, ...);

Mark this with ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL.

> +
> +static int
> +pvsDoCmdRun(char **outbuf, const char *binary, va_list list)
> +{
> +    virCommandPtr cmd = virCommandNew(binary);
> +    const char *arg;
> +    int exitstatus;
> +    char *scmd = NULL;
> +    char *sstatus = NULL;
> +    int ret = -1;
> +
> +    while ((arg = va_arg(list, const char *)) != NULL)
> +        virCommandAddArg(cmd, arg);
> +
> +    if (outbuf)
> +        virCommandSetOutputBuffer(cmd, outbuf);
> +
> +    scmd = virCommandToString(cmd);
> +    if (!scmd)
> +        goto cleanup;
> +
> +    if (virCommandRun(cmd, &exitstatus)) {

If you pass NULL as the second argument here,

> +        pvsError(VIR_ERR_INTERNAL_ERROR,
> +                 _("Failed to execute command '%s'"), scmd);
> +        goto cleanup;
> +    }
> +
> +    if (exitstatus) {
> +        sstatus = virCommandTranslateStatus(exitstatus);
> +        pvsError(VIR_ERR_INTERNAL_ERROR,
> +                 _("Command '%s' finished with errors: %s"), scmd, sstatus);

then virCommand will take care of ensuring a 0 exit status, and with
more uniform error reporting.  No need to reinvent the work that someone
else will do for you :)  The only reason to ever pass an &exitstatus
parameter is if you validly expect non-zero exit status.


> +/*
> + * Run command and parse its JSON output, return
> + * pointer to virJSONValue or NULL in case of error.
> + */
> +virJSONValuePtr
> +pvsParseOutput(const char *binary, ...)
> +{
> +    char *outbuf;
> +    virJSONValuePtr jobj = NULL;
> +    va_list list;
> +    int ret;
> +
> +    va_start(list, binary);
> +    ret = pvsDoCmdRun(&outbuf, binary, list);
> +    va_end(list);
> +    if (ret)
> +        return NULL;
> +
> +    jobj = virJSONValueFromString(outbuf);
> +    if (!jobj)
> +        pvsError(VIR_ERR_INTERNAL_ERROR, "%s: %s",
> +                 _("invalid output from prlctl"), outbuf);
> +
> +    return jobj;

You leak outbuf if you reach the virJSONValueFromString() call.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]