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

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120710/748500d7/attachment-0001.sig>