[libvirt] [PATCH] SetAutostart and GetAutostart in openvz driver

[Daniel P. Berrange]

On Tue, Jul 08, 2008 at 05:51:05PM +0400, Evgeniy Sokolov wrote:
> +int
> +openvzReadConfigParam(int vpsid ,const char * param, char *value, int maxlen)
> +{
> +    char conf_file[PATH_MAX] ;
> +    char line[PATH_MAX] ;
> +    int ret, found = 0;
> +    char * conf_dir;
> +    int fd ;
> +    char * sf, * token;
> +    char *saveptr = NULL;
> +
> +        
> +    conf_dir = openvzLocateConfDir();
> +    if (conf_dir == NULL)
> +        return -1;
> +
> +    sprintf(conf_file,"%s/%d.conf",conf_dir,vpsid);

Please use snprintf & check the return value, even if you think
it'll never overflow PATH_MAX. Or even use asprintf().

> +    VIR_FREE(conf_dir);
> +
> +    value[0] = 0;
> +
> +    fd = open(conf_file, O_RDWR);

You're only reading the config use  O_RDONLY. THe O_RDWR will generate
an abort() under FORTIFY_SOURCE too, because you're not supplying the
3rd mode arg which is mandatory when opening for write access.

> +    if (fd == -1)
> +        return -1;
> +
> +    while(1) {
> +        ret = openvz_readline(fd, line, sizeof(line));
> +        if(ret <= 0)
> +            break;
> +        saveptr = NULL;
> +        if (STREQLEN(line, param, strlen(param))) { 
> +            sf = line;
> +            sf += strlen(param);
> +            if (sf[0] == '=' && (token = strtok_r(sf,"\"\t=\n", &saveptr)) != NULL) {
> +                strncpy(value, token, maxlen) ;

Potentially non-terminated string there - if there is no null byte among the
first maxlen bytes  of token, the string placed in value will not be null
terminated.


> +openvzDomainSetAutostart(virDomainPtr dom, int autostart)
> +{
> +    char cmdbuf[CMDBUF_LEN], *cmdExec[OPENVZ_MAX_ARG];
> +    int ret, pid, outfd, errfd;
> +    virConnectPtr conn= dom->conn;
> +    struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
> +    struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> +
> +    if (!vm) {
> +        error(conn, VIR_ERR_INVALID_DOMAIN, _("no domain with matching uuid"));
> +        return -1;
> +    }
> +
> +    snprintf(cmdbuf, CMDBUF_LEN - 1, VZCTL " set %s --onboot %s --save", vm->vmdef->name, 
> +			autostart ? "yes" : "no");
> +
> +    if((ret = convCmdbufExec(cmdbuf, cmdExec)) == -1)
> +    {
> +        openvzLog(OPENVZ_ERR, "%s", _("Error in parsing Options to OPENVZ"));
> +        goto bail_out5;
> +    }
> +    ret = virExec(conn, (char **)cmdExec, &pid, -1, &outfd, &errfd);

I realize you are just following the existing pattern used in OpenVZ
driver, but this piece of code is horrible. 

sprintf'ing into a string, then parsing that string and turning it back 
into a list of argv[] strings, with no escaping of special characters,
or quoting. eg if the vm name had a space in it it'll mis-parse it.

Just declare the command argv straight into a char*[], eg

  const char *prog[] = {
     VZCTL,
     "set",
     vm->vmdef->name,
     "--onboot",
     autostart ? "yes" : "no",
     "--save"
  }
  ret = virExec(conn, prog, &pid, -1, &outfd, &errfd)

See the storage_backend_logical.c file for examples of this kind of approach

We should put other uses of convCmdbufExec() on the TODO list for removal
in the future.

> +static int
> +openvzDomainGetAutostart(virDomainPtr dom, int *autostart)
> +{
> +    virConnectPtr conn= dom->conn;
> +    struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
> +    struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> +    char value[1024];
> +
> +    if (!vm) {
> +        error(conn, VIR_ERR_INVALID_DOMAIN, _("no domain with matching uuid"));
> +        return -1;
> +    }
> +
> +    if (openvzReadConfigParam(vm->vpsid , "ONBOOT", value, sizeof(value)) < 0) {
> +        openvzLog(OPENVZ_ERR, "%s", _("Cound not read container config"));

This should raise a VIR_ERR_INTERNAL_ERROR otherwise the details are never
seen by the calling app.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|