8:51 a.m.
Implemented functions domainSetAutostart and domainGetAutostart.
other:
fixed closing file descriptor during reading uuid from config.
12:23 p.m.
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 :|
9:48 a.m.
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.
In fact,
names of VMs are always numbers in OpenVZ.
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.
Ok. Moreover, many code is needed to rewrite.
Daniel, thanks for review!
fixed patch is attached.
2:55 a.m.
On Wed, Jul 09, 2008 at 06:48:25PM +0400, Evgeniy Sokolov wrote:
>
>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.
In fact, names of VMs are always numbers in OpenVZ.
>
>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.
Ok. Moreover, many code is needed to rewrite.
Daniel, thanks for review!
fixed patch is attached.
Patch looks fine to me, applied and commited to CVS,
thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5979
days inactive
5981
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Evgeniy Sokolov