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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list