[libvirt] [PATCH] openvz: convert popen to virCommand

[Matthias Bolte]

2010/12/3 Eric Blake <eblake at redhat.com>:
> popen must be matched with pclose (not fclose), or it will leak
> resources.  Furthermore, it is a lousy interface when it comes
> to signal handling.  We're much better off using our decent command
> wrapper.
>
> * src/openvz/openvz_conf.c (openvzLoadDomains, openvzGetVEID):
> Replace popen with virCommand usage.
> ---
>  src/openvz/openvz_conf.c |   54 +++++++++++++++++++++++----------------------
>  1 files changed, 28 insertions(+), 26 deletions(-)

>  int openvzLoadDomains(struct openvz_driver *driver) {
> -    FILE *fp;
>     int veid, ret;
>     char status[16];
>     char uuidstr[VIR_UUID_STRING_BUFLEN];
>     virDomainObjPtr dom = NULL;
>     char temp[50];
> +    char *outbuf = NULL;
> +    char *line;
> +    virCommandPtr cmd = NULL;
>
>     if (openvzAssignUUIDs() < 0)
>         return -1;
>
> -    if ((fp = popen(VZLIST " -a -ovpsid,status -H 2>/dev/null", "r")) == NULL) {
> -        openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed"));
> -        return -1;
> -    }
> -
> -    while (!feof(fp)) {
> -        if (fscanf(fp, "%d %s\n", &veid, status) != 2) {
> -            if (feof(fp))
> -                break;
> +    cmd = virCommandNewArgList(VZLIST, "-a", "-ovpsid,status", "-H", NULL);
> +    virCommandSetOutputBuffer(cmd, &outbuf);
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
>
> +    line = *outbuf ? outbuf : NULL;

Is outbuf guaranteed to be non-NULL when virCommandRun succeeds?
Otherwise we have a potential NULL dereference here.

> +    while (line) {
> +        if (sscanf(line, "%d %s\n", &veid, status) != 2) {
>             openvzError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Failed to parse vzlist output"));
>             goto cleanup;

> @@ -978,27 +985,22 @@ static int openvzAssignUUIDs(void)
>  */
>
>  int openvzGetVEID(const char *name) {
> -    char *cmd;
> +    virCommandPtr cmd;
> +    char *outbuf;
>     int veid;
> -    FILE *fp;
>     bool ok;
>
> -    if (virAsprintf(&cmd, "%s %s -ovpsid -H", VZLIST, name) < 0) {
> -        openvzError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                    _("virAsprintf failed"));
> +    cmd = virCommandNewArgList(VZLIST, name, "-ovpsid", "-H", NULL);
> +    virCommandSetOutputBuffer(cmd, &outbuf);
> +    if (virCommandRun(cmd, NULL) < 0) {
> +        virCommandFree(cmd);

Is outbuf guaranteed to be unallocated when virCommandRun fails?
Otherwise a VIR_FREE(outbuf) is missing here.

>         return -1;
>     }
>
> -    fp = popen(cmd, "r");
> -    VIR_FREE(cmd);
> -
> -    if (fp == NULL) {
> -        openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed"));
> -        return -1;
> -    }
> +    virCommandFree(cmd);
> +    ok = sscanf(outbuf, "%d\n", &veid) == 1;

Same question here about outbuf being guaranteed to be non-NULL when
virCommandRun succeeds as in openvzLoadDomains. If not then sscanf is
called with NULL and that'll probably segfault.

> +    VIR_FREE(outbuf);
>
> -    ok = fscanf(fp, "%d\n", &veid ) == 1;
> -    VIR_FORCE_FCLOSE(fp);
>     if (ok && veid >= 0)
>         return veid;

Matthias