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

Re: [libvirt] [PATCH 3/5] openvz: Add openvzVEGetStringParam



On 07/10/2012 02:46 PM, Guido Günther wrote:

> +char*
> +openvzVEGetStringParam(virDomainPtr domain, const char* param)
> +{
> +    int status, len;
> +    char *output = NULL;
> +
> +    virCommandPtr cmd = virCommandNewArgList(VZLIST,
> +                                             "-o",
> +                                             param,
> +                                             domain->name,
> +                                             "-H" , NULL);
> +
> +    virCommandSetOutputBuffer(cmd, &output);
> +    if (virCommandRun(cmd, &status)) {

This should check for virCommandRun() < 0.  Also, virCommandRun() does
not guarantee that 'status' is populated unless it returns 0; if it
returns -1, you may be left with outputting uninitilized data (maybe we
could argue that it is a bug in virCommandRun that should be fixed...).

Outputting the raw exit status isn't very handy; I think it would be
simpler to just pass NULL for the second argument (which lets
virCommandRun issue a reasonable error message about any failure to run
the command, including the entire command line attempted, which already
includes param and domain->name).

> +        openvzError(VIR_ERR_OPERATION_FAILED,
> +                    _("Failed to get %s for %s: %d"), param, domain->name,
> +                    status);
> +        VIR_FREE(output);
> +    }
> +    len = strlen(output);

Crash-tastic!  If the command fails, then you end up calling
strlen(NULL).  Are you missing a goto or early return?

> +    /* delete trailing newline */
> +    if (len)
> +        output[len-1] = '\0';

Shouldn't you at least check that output[len-1] is '\n' before nuking
it?  Also, our style prefers spaces on either side of '-'.

-- 
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]