[libvirt] [PATCH 4/4] lxc: store tty pid
This patch will use a file in the lxc configuration directory to store the tty forwarding process pid. The pid is stored after the process is fork()'d. It's loaded during startup when the config for a running container is loaded. The file is deleted when the domain is undefined. This should avoid "losing" the tty pid over a libvirtd restart. -- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization
er, this time with the patch.... Dave Leskovec wrote:
This patch will use a file in the lxc configuration directory to store the tty forwarding process pid. The pid is stored after the process is fork()'d. It's loaded during startup when the config for a running container is loaded. The file is deleted when the domain is undefined. This should avoid "losing" the tty pid over a libvirtd restart.
-- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization
On Thu, May 29, 2008 at 03:44:24PM -0700, Dave Leskovec wrote:
er, this time with the patch....
Dave Leskovec wrote:
This patch will use a file in the lxc configuration directory to store the tty forwarding process pid. The pid is stored after the process is fork()'d. It's loaded during startup when the config for a running container is loaded. The file is deleted when the domain is undefined. This should avoid "losing" the tty pid over a libvirtd restart.
the idea sounds fine,
Index: b/src/lxc_driver.c =================================================================== --- a/src/lxc_driver.c 2008-05-29 14:34:45.000000000 -0700 +++ b/src/lxc_driver.c 2008-05-29 14:34:51.000000000 -0700 @@ -328,6 +328,8 @@
vm->configFile[0] = '\0';
+ lxcDeleteTtyPid(vm); + lxcRemoveInactiveVM(driver, vm);
return 0; @@ -798,6 +800,10 @@ lxcTtyForward(vm->parentTty, vm->containerTtyFd); }
+ if (lxcStoreTtyPid(driver, vm)) { + DEBUG0("unable to store tty pid"); + } + close(vm->parentTty); close(vm->containerTtyFd);
Hum, I'm surprized the PID file is removed only in lxcDomainUndefine. The PID file need to be re-created each time the domain is started. But a domain could be started and stopped many time while being defined, what happen in a (define/start/stop/start) sequence ? Looks to me the O_CREAT flag in the second start would break because the PID file is still here ... Maybe I'm just wrong but if you could just double check that scenario before the commit, that would be nice :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Thu, May 29, 2008 at 03:44:24PM -0700, Dave Leskovec wrote: ...
Hum, I'm surprized the PID file is removed only in lxcDomainUndefine. The PID file need to be re-created each time the domain is started. But a domain could be started and stopped many time while being defined, what happen in a (define/start/stop/start) sequence ? Looks to me the O_CREAT flag in the second start would break because the PID file is still here ...
Maybe I'm just wrong but if you could just double check that scenario before the commit, that would be nice :-)
Daniel
O_CREAT just causes the file to be created if it doesn't exist. O_EXCL will trigger an error if the file already exists. I'd considered removing it after killing the tty process but don't recall now why I didn't do that. I think it makes sense to do that so I'll make the change with the ones from Jim and Dan. Thanks! -- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization
On Fri, May 30, 2008 at 04:31:14PM -0700, Dave Leskovec wrote:
Daniel Veillard wrote:
On Thu, May 29, 2008 at 03:44:24PM -0700, Dave Leskovec wrote: ...
Hum, I'm surprized the PID file is removed only in lxcDomainUndefine. The PID file need to be re-created each time the domain is started. But a domain could be started and stopped many time while being defined, what happen in a (define/start/stop/start) sequence ? Looks to me the O_CREAT flag in the second start would break because the PID file is still here ...
Maybe I'm just wrong but if you could just double check that scenario before the commit, that would be nice :-)
Daniel
O_CREAT just causes the file to be created if it doesn't exist. O_EXCL will trigger an error if the file already exists.
Oops, right :-), wrong flag !
I'd considered removing it after killing the tty process but don't recall now why I didn't do that. I think it makes sense to do that so I'll make the change with the ones from Jim and Dan.
Cool, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi Dave, All four of your patches look good. I've included a few comments on the fourth below. Dave Leskovec <dlesko@linux.vnet.ibm.com> wrote:
Index: b/src/lxc_conf.h =================================================================== ... +int lxcStoreTtyPid(lxc_driver_t *driver, lxc_vm_t *vm); +int lxcLoadTtyPid(lxc_driver_t *driver, lxc_vm_t *vm); +int lxcDeleteTtyPid(lxc_vm_t *vm);
It looks like each of the above pointer parameters may/should be "const". Renaming lxcDeleteTtyPid to lxcDeleteTtyPidFile would make it clear that it deletes the file, not the PID.
Index: b/src/lxc_conf.c =================================================================== ... +int lxcStoreTtyPid(lxc_driver_t *driver, lxc_vm_t *vm) +{ + int rc = -1; + int fd = -1;
There is no need to initialize fd here.
+ FILE *file = NULL; + + if (vm->ttyPidFile[0] == 0x00) { + if (virFileBuildPath(driver->configDir, vm->def->name, ".pid", + vm->ttyPidFile, PATH_MAX) < 0) { + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, ...
+int lxcDeleteTtyPid(lxc_vm_t *vm) +{ + if (vm->ttyPidFile[0] == 0x00) { + goto no_file; + } + + if (unlink(vm->ttyPidFile) < 0) { + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot remove ttyPidFile %s"), vm->ttyPidFile);
Please include strerror(errno) in the diagnostic, as you've done for other failed sys/lib calls. Do you want to give a diagnostic even for ENOENT?
On Thu, May 29, 2008 at 03:44:24PM -0700, Dave Leskovec wrote:
er, this time with the patch....
Dave Leskovec wrote:
This patch will use a file in the lxc configuration directory to store the tty forwarding process pid. The pid is stored after the process is fork()'d. It's loaded during startup when the config for a running container is loaded. The file is deleted when the domain is undefined. This should avoid "losing" the tty pid over a libvirtd restart.
The PID file should not be put in SYSCONFDIR. Transient state needs to go in LOCALSTATEDIR - which translates to /var. So /var/lib/libvirt/lxc/ Dan. -- |: Red Hat, Engineering, Boston -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 :|
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Leskovec -
Jim Meyering