5:20 p.m.
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
5:44 p.m.
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
1:38 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6:31 p.m.
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
3:51 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:49 a.m.
Hi Dave,
All four of your patches look good.
I've included a few comments on the fourth below.
Dave Leskovec <dlesko(a)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?
6:25 a.m.
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 :|
5832
days inactive
5834
days old
6 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Leskovec -
Jim Meyering