Re: [libvirt] [PATCH] SetAutostart and GetAutostart in openvz driver
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 :|