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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)