
Most of them effect virt-sandbox-service, with the biggest change beeing the addition of InteractiveContainers. We want to be able to support OpenShift, which requres the abiltiy to run a command within a container environement but does not want to use the systemd/unitfiles. These containers will run a setup script and then a user process to actually run the application.
From Dan Walsh <dwalsh@redhat.com> # This line is ignored. From: Dan Walsh <dwalsh@redhat.com> Subject: In-Reply-To:

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service-util.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bin/virt-sandbox-service-util.c b/bin/virt-sandbox-service-util.c index 4d164d8..430518f 100644 --- a/bin/virt-sandbox-service-util.c +++ b/bin/virt-sandbox-service-util.c @@ -194,8 +194,8 @@ static int set_process_label(pid_t pid) { static int container_execute( GVirSandboxContext *ctx, const gchar *command, pid_t pid ) { int ret = EXIT_FAILURE; - char **argv; - int argc; + char **argv = NULL; + int argc=-1; if (set_process_label(pid) < 0) goto cleanup; @@ -227,6 +227,10 @@ static int container_execute( GVirSandboxContext *ctx, const gchar *command, pid } cleanup: + if (argc > -1) + free(argv[0]); + free(argv); + return ret; } -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:17PM -0400, Dan Walsh wrote:
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service-util.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/bin/virt-sandbox-service-util.c b/bin/virt-sandbox-service-util.c index 4d164d8..430518f 100644 --- a/bin/virt-sandbox-service-util.c +++ b/bin/virt-sandbox-service-util.c @@ -194,8 +194,8 @@ static int set_process_label(pid_t pid) { static int container_execute( GVirSandboxContext *ctx, const gchar *command, pid_t pid ) {
int ret = EXIT_FAILURE; - char **argv; - int argc; + char **argv = NULL; + int argc=-1;
if (set_process_label(pid) < 0) goto cleanup; @@ -227,6 +227,10 @@ static int container_execute( GVirSandboxContext *ctx, const gchar *command, pid }
cleanup: + if (argc > -1) + free(argv[0]);
This is still leaking all except for the first argv element. You need to free all elements.
+ free(argv); +
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

We need to add support for interactive sandbox/containers for OpenShift. This patch will create the correct container type based off the /etc/libvirt-sandbox/service/* Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service-util.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/bin/virt-sandbox-service-util.c b/bin/virt-sandbox-service-util.c index 430518f..2630109 100644 --- a/bin/virt-sandbox-service-util.c +++ b/bin/virt-sandbox-service-util.c @@ -352,6 +352,7 @@ static gboolean libvirt_lxc_attach(const gchar *option_name, int main(int argc, char **argv) { GMainLoop *loop = NULL; + GVirSandboxConfigInteractive *interactive_cfg = NULL; GVirSandboxConfigService *cfg = NULL; GVirSandboxConfig *config = NULL; GVirSandboxContext *ctx = NULL; @@ -360,7 +361,6 @@ int main(int argc, char **argv) { int ret = EXIT_FAILURE; pid_t pid = 0; gchar *buf=NULL; - GVirSandboxContextService *service; gchar *uri = NULL; gchar *command = NULL; @@ -451,16 +451,28 @@ int main(int argc, char **argv) { goto cleanup; } - cfg = GVIR_SANDBOX_CONFIG_SERVICE(config); + if (GVIR_SANDBOX_IS_CONFIG_INTERACTIVE(config)) { + GVirSandboxContextInteractive *service; + interactive_cfg = GVIR_SANDBOX_CONFIG_INTERACTIVE(config); - if (!(service = gvir_sandbox_context_service_new(hv, cfg))) { - g_printerr(_("Unable to create new context service: %s\n"), - err && err->message ? err->message : _("unknown")); - goto cleanup; + if (!(service = gvir_sandbox_context_interactive_new(hv, interactive_cfg))) { + g_printerr(_("Unable to create new context service: %s\n"), + err && err->message ? err->message : _("unknown")); + goto cleanup; + } + ctx = GVIR_SANDBOX_CONTEXT(service); + } else { + GVirSandboxContextService *service; + cfg = GVIR_SANDBOX_CONFIG_SERVICE(config); + + if (!(service = gvir_sandbox_context_service_new(hv, cfg))) { + g_printerr(_("Unable to create new context service: %s\n"), + err && err->message ? err->message : _("unknown")); + goto cleanup; + } + ctx = GVIR_SANDBOX_CONTEXT(service); } - ctx = GVIR_SANDBOX_CONTEXT(service); - if (command) { container_execute(ctx, command, pid); } @@ -475,6 +487,9 @@ cleanup: free(buf); + if (interactive_cfg) + g_object_unref(interactive_cfg); + if (cfg) g_object_unref(cfg); -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:18PM -0400, Dan Walsh wrote:
We need to add support for interactive sandbox/containers for OpenShift.
This patch will create the correct container type based off the /etc/libvirt-sandbox/service/*
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service-util.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/bin/virt-sandbox-service-util.c b/bin/virt-sandbox-service-util.c index 430518f..2630109 100644 --- a/bin/virt-sandbox-service-util.c +++ b/bin/virt-sandbox-service-util.c @@ -352,6 +352,7 @@ static gboolean libvirt_lxc_attach(const gchar *option_name,
int main(int argc, char **argv) { GMainLoop *loop = NULL; + GVirSandboxConfigInteractive *interactive_cfg = NULL; GVirSandboxConfigService *cfg = NULL; GVirSandboxConfig *config = NULL;
This is rather overkill - just have the single 'GVirSandboxConfig *config' variable and cast it where needed, removing the intermediate variables.
GVirSandboxContext *ctx = NULL; @@ -360,7 +361,6 @@ int main(int argc, char **argv) { int ret = EXIT_FAILURE; pid_t pid = 0; gchar *buf=NULL; - GVirSandboxContextService *service; gchar *uri = NULL; gchar *command = NULL;
@@ -451,16 +451,28 @@ int main(int argc, char **argv) { goto cleanup; }
- cfg = GVIR_SANDBOX_CONFIG_SERVICE(config); + if (GVIR_SANDBOX_IS_CONFIG_INTERACTIVE(config)) { + GVirSandboxContextInteractive *service; + interactive_cfg = GVIR_SANDBOX_CONFIG_INTERACTIVE(config);
- if (!(service = gvir_sandbox_context_service_new(hv, cfg))) { - g_printerr(_("Unable to create new context service: %s\n"), - err && err->message ? err->message : _("unknown")); - goto cleanup; + if (!(service = gvir_sandbox_context_interactive_new(hv, interactive_cfg))) {
Replace 'interactive_cfg' with a direct call to GVIR_SANDBOX_CONFIG_INTERACTIVE(config)
+ g_printerr(_("Unable to create new context service: %s\n"), + err && err->message ? err->message : _("unknown")); + goto cleanup; + } + ctx = GVIR_SANDBOX_CONTEXT(service); + } else { + GVirSandboxContextService *service; + cfg = GVIR_SANDBOX_CONFIG_SERVICE(config); + + if (!(service = gvir_sandbox_context_service_new(hv, cfg))) { + g_printerr(_("Unable to create new context service: %s\n"), + err && err->message ? err->message : _("unknown")); + goto cleanup; + } + ctx = GVIR_SANDBOX_CONTEXT(service); }
- ctx = GVIR_SANDBOX_CONTEXT(service); - if (command) { container_execute(ctx, command, pid); } @@ -475,6 +487,9 @@ cleanup:
free(buf);
+ if (interactive_cfg) + g_object_unref(interactive_cfg); + if (cfg) g_object_unref(cfg);
And thus instead of needing these 2 blocks you can now just do if (config) g_object_unref(config) Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/Makefile.am | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/bin/Makefile.am b/bin/Makefile.am index 69af01e..4f98aa4 100644 --- a/bin/Makefile.am +++ b/bin/Makefile.am @@ -5,7 +5,7 @@ libexec_PROGRAMS = virt-sandbox-service-util bin_SCRIPTS = virt-sandbox-service -virtsandboxcompdir = $(sysconfdir)/bash_completion.d/ +virtsandboxcompdir = $(datarootdir)/bash-completion/completions/ virtsandboxcomp_DATA = virt-sandbox-service-bash-completion.sh crondailydir = $(sysconfdir)/cron.daily @@ -82,6 +82,22 @@ virt_sandbox_service_util_LDFLAGS = \ $(WARN_CFLAGS) \ $(NULL) +install-virtsandboxcompDATA: + @$(NORMAL_INSTALL) + @list='$(virtsandboxcomp_DATA)'; test -n "$(virtsandboxcompdir)" || list=; \ + if test -n "$$list"; then \ + echo " $(MKDIR_P) '$(DESTDIR)$(virtsandboxcompdir)'"; \ + $(MKDIR_P) "$(DESTDIR)$(virtsandboxcompdir)" || exit 1; \ + fi; \ + for p in $$list; do \ + if test -f "$$p"; then d=; else d="$(srcdir)/"; fi; \ + echo "$$d$$p"; \ + done | $(am__base_list) | \ + while read files; do \ + echo " $(INSTALL_DATA) $$files '$(DESTDIR)$(virtsandboxcompdir)'"; \ + $(INSTALL_DATA) $$files "$(DESTDIR)$(virtsandboxcompdir)"/virt-sandbox-service || exit $$?; \ + done + install-data-local: $(MKDIR_P) $(DESTDIR)$(sysconfdir)/libvirt-sandbox/services -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:19PM -0400, Dan Walsh wrote: GIT commit messages should have one initial line less than 70 characters, then a blank line, then the body of the commit message. This avoids getting crazy subject lines like you have here.
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/Makefile.am | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/bin/Makefile.am b/bin/Makefile.am index 69af01e..4f98aa4 100644 --- a/bin/Makefile.am +++ b/bin/Makefile.am @@ -5,7 +5,7 @@ libexec_PROGRAMS = virt-sandbox-service-util
bin_SCRIPTS = virt-sandbox-service
-virtsandboxcompdir = $(sysconfdir)/bash_completion.d/ +virtsandboxcompdir = $(datarootdir)/bash-completion/completions/ virtsandboxcomp_DATA = virt-sandbox-service-bash-completion.sh
crondailydir = $(sysconfdir)/cron.daily @@ -82,6 +82,22 @@ virt_sandbox_service_util_LDFLAGS = \ $(WARN_CFLAGS) \ $(NULL)
+install-virtsandboxcompDATA: + @$(NORMAL_INSTALL) + @list='$(virtsandboxcomp_DATA)'; test -n "$(virtsandboxcompdir)" || list=; \ + if test -n "$$list"; then \ + echo " $(MKDIR_P) '$(DESTDIR)$(virtsandboxcompdir)'"; \ + $(MKDIR_P) "$(DESTDIR)$(virtsandboxcompdir)" || exit 1; \ + fi; \ + for p in $$list; do \ + if test -f "$$p"; then d=; else d="$(srcdir)/"; fi; \ + echo "$$d$$p"; \ + done | $(am__base_list) | \ + while read files; do \ + echo " $(INSTALL_DATA) $$files '$(DESTDIR)$(virtsandboxcompdir)'"; \ + $(INSTALL_DATA) $$files "$(DESTDIR)$(virtsandboxcompdir)"/virt-sandbox-service || exit $$?; \ + done
This is really overkill - you can do the same thing in install-data-local in 2 lines of code.
+ install-data-local: $(MKDIR_P) $(DESTDIR)$(sysconfdir)/libvirt-sandbox/services
Kill the virtsandboxcomp_DATA variable earlier and then do install-data-local: $(MKDIR_P) $(DESTDIR)$(virtsandboxcompdir) cp virt-sandbox-service-bash-completion.sh $(DESTDIR)$(virtsandboxcompdir)/virt-sandbox-service uninstall-local: rm -f $(DESTDIR)$(virtsandboxcompdir)/virt-sandbox-service And add EXTRA_DIST += virt-sandbox-service-bash-completion.sh Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

The basic idea is to allow administrators or the systemd service to reload units which are running within a container. If you have one or more units defined for a container, then just those units will get the reloads, as opposed to stopping and restarting the container. Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 30 ++++++++++++++++++++--------- bin/virt-sandbox-service-bash-completion.sh | 3 ++- bin/virt-sandbox-service-reload.pod | 9 +++++++-- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 478769d..0e38577 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -164,12 +164,12 @@ After=libvirtd.service [Service] Type=simple ExecStart=/usr/bin/virt-sandbox-service start %(NAME)s -ExecReload=/usr/bin/virt-sandbox-service reload %(NAME)s +ExecReload=/usr/bin/virt-sandbox-service reload -u %(RELOAD)s %(NAME)s ExecStop=/usr/bin/virt-sandbox-service stop %(NAME)s [Install] WantedBy=%(TARGET)s -""" % { 'NAME':name, 'FOLLOW':self.__follow_units(), 'TARGET':self.__target() } +""" % { 'NAME':name, 'FOLLOW':self.__follow_units(), 'TARGET':self.__target(), 'RELOAD': " -u ".join(map(lambda x: x[0], self.unit_file_list)) } fd = open(self.unitfile, "w") fd.write(unit) fd.close() @@ -649,11 +649,17 @@ PrivateNetwork=false pass raise e - def reload(self): - # Crude way of doing this. - self.stop() - self.start() - # self.execute("systemctl reload %s.service" % self.get_name()) + def reload(self, unitfiles): + # + # Reload A ServiceContainer + # + class Args: + command = [] + nolabel = None + name = self.name + args = Args() + args.command = [ "systemctl", "reload" ] + map(lambda x: x[0], unitfiles) + execute(args) def __connect(self): if not self.conn: @@ -800,8 +806,11 @@ def sandbox_list(args): def sandbox_reload(args): - container = Container(args.name, args.uri) - container.reload() + config = read_config(args.name) + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + raise ValueError(_("Interactive Containers do not support reload")) + container = ServiceContainer(uri = args.uri, config = config) + container.reload(args.unitfiles) def start(args): os.execl("/usr/libexec/virt-sandbox-service-util", "virt-sandbox-service-util","-s", args.name) @@ -989,6 +998,9 @@ def gen_stop_args(subparser): def gen_reload_args(subparser): parser = subparser.add_parser("reload", help=_("Reload a running sandbox container")) + parser.add_argument("-u", "--unitfile", required=True, + action=CheckUnit, dest="unitfiles", + help=_("Systemd Unit file to reload within the sandbox container")) requires_name(parser) parser.set_defaults(func=sandbox_reload) diff --git a/bin/virt-sandbox-service-bash-completion.sh b/bin/virt-sandbox-service-bash-completion.sh index c855fd2..ce14a7d 100755 --- a/bin/virt-sandbox-service-bash-completion.sh +++ b/bin/virt-sandbox-service-bash-completion.sh @@ -57,6 +57,7 @@ _virt_sandbox_service () { [ALL]='-h --help' [CREATE]='-u --unitfile -p --path -t --type -l --level -d --dynamic -n --clone -i --image -s --size' [LIST]='-r --running' + [RELOAD]='-u --unitfile' [EXECUTE]='-N --noseclabel' ) @@ -88,7 +89,7 @@ _virt_sandbox_service () { COMPREPLY=( $(compgen -W "${OPTS[ALL]} $( __get_all_running_containers ) " -- "$cur") ) return 0 elif test "$verb" == "reload" ; then - COMPREPLY=( $(compgen -W "${OPTS[ALL]} $( __get_all_running_containers ) " -- "$cur") ) + COMPREPLY=( $(compgen -W "${OPTS[ALL]} ${OPTS[RELOAD]} $( __get_all_running_containers ) " -- "$cur") ) return 0 elif test "$verb" == "connect" ; then COMPREPLY=( $(compgen -W "${OPTS[ALL]} $( __get_all_running_containers ) " -- "$cur") ) diff --git a/bin/virt-sandbox-service-reload.pod b/bin/virt-sandbox-service-reload.pod index d26af82..b4919be 100644 --- a/bin/virt-sandbox-service-reload.pod +++ b/bin/virt-sandbox-service-reload.pod @@ -4,7 +4,7 @@ virt-sandbox-service reload - Reload a security container =head1 SYNOPSIS - virt-sandbox-service [-c URI] reload [-h] NAME + virt-sandbox-service [-c URI] reload [-h] -u UNIT_FILE NAME =head1 DESCRIPTION @@ -26,6 +26,11 @@ Display help message =item B<-c> URI, B<--connect URI> +=item B<-u UNIT_FILE>, B<--unitfile UNIT_FILE> + +Name of the systemd unit file to reload within the container. Can be repeated +if multiple unit files need to be reloaded within the sandbox. + The connection URI for the hypervisor (only LXC or QEMU are supported currently). @@ -35,7 +40,7 @@ supported currently). Reload the httpd1 container - # virt-sandbox-service reload httpd1 + # virt-sandbox-service reload -u httpd.service httpd1 =head1 SEE ALSO -- 1.8.2

s/This patch adds/Adds/ in the subject On Tue, Apr 02, 2013 at 06:11:20PM -0400, Dan Walsh wrote:
The basic idea is to allow administrators or the systemd service to reload units which are running within a container. If you have one or more units defined
Line wrapping is missing here
for a container, then just those units will get the reloads, as opposed to stopping and restarting the container.
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 30 ++++++++++++++++++++--------- bin/virt-sandbox-service-bash-completion.sh | 3 ++- bin/virt-sandbox-service-reload.pod | 9 +++++++-- 3 files changed, 30 insertions(+), 12 deletions(-)
ACK if the commit message is fixed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service-create.pod | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/virt-sandbox-service-create.pod b/bin/virt-sandbox-service-create.pod index 1f82e1d..3fb8ae0 100644 --- a/bin/virt-sandbox-service-create.pod +++ b/bin/virt-sandbox-service-create.pod @@ -4,7 +4,7 @@ virt-sandbox-service create - Create a Security container =head1 SYNOPSIS - virt-sandbox-service [-c URI] create [-h] -u UNIT_FILE [ --clone ] [-p PATH] [-N NETWORK-OPTS] [-s SECURITY-OPTS] [-i SIZE] [-n] NAME + virt-sandbox-service [-c URI] create [-h] -u UNIT_FILE [ --copy ] [-p PATH] [-N NETWORK-OPTS] [-s SECURITY-OPTS] [-i SIZE] [-n] NAME =head1 DESCRIPTION @@ -34,9 +34,9 @@ supported currently). Name of the systemd unit file to be to run within the container. Can be repeated if multiple unit files are required within the sandbox. -=item B<-C>, B<--clone> +=item B<-C>, B<--copy> -Clone content from /etc and /var directories that will be mounted within the container. +Copy content from /etc and /var directories that will be mounted within the container. =item B<-p PATH>, B<--path PATH> -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:21PM -0400, Dan Walsh wrote:
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service-create.pod | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/bin/virt-sandbox-service-create.pod b/bin/virt-sandbox-service-create.pod index 1f82e1d..3fb8ae0 100644 --- a/bin/virt-sandbox-service-create.pod +++ b/bin/virt-sandbox-service-create.pod @@ -4,7 +4,7 @@ virt-sandbox-service create - Create a Security container
=head1 SYNOPSIS
- virt-sandbox-service [-c URI] create [-h] -u UNIT_FILE [ --clone ] [-p PATH] [-N NETWORK-OPTS] [-s SECURITY-OPTS] [-i SIZE] [-n] NAME + virt-sandbox-service [-c URI] create [-h] -u UNIT_FILE [ --copy ] [-p PATH] [-N NETWORK-OPTS] [-s SECURITY-OPTS] [-i SIZE] [-n] NAME
=head1 DESCRIPTION
@@ -34,9 +34,9 @@ supported currently). Name of the systemd unit file to be to run within the container. Can be repeated if multiple unit files are required within the sandbox.
-=item B<-C>, B<--clone> +=item B<-C>, B<--copy>
-Clone content from /etc and /var directories that will be mounted within the container. +Copy content from /etc and /var directories that will be mounted within the container.
=item B<-p PATH>, B<--path PATH>
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 0e38577..a064e9a 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -91,7 +91,7 @@ class Container: if create: if config and os.path.exists(config): - raise ValueError(["Container already exists"]) + raise ValueError([_("Container already exists")]) self.config = LibvirtSandbox.ConfigService.new(name) return @@ -123,7 +123,7 @@ class Container: def __get_sandbox_target(self): if len(self.unit_file_list) > 1: - raise ValueError(["Only Valid for single units"]) + raise ValueError([_("Only Valid for single units")]) return "%s_sandbox.target" % self.__get_sandboxed_service() def __target(self): @@ -178,8 +178,8 @@ WantedBy=%(TARGET)s p = Popen(["/usr/bin/systemctl","enable", self.unitfile],stdout=PIPE, stderr=PIPE) p.communicate() if p.returncode and p.returncode != 0: - raise OSError("Failed to enable %s unit file " % self.unitfile) - sys.stdout.write("Created unit file %s\n" % self.unitfile) + raise OSError(_("Failed to enable %s unit file") % self.unitfile) + sys.stdout.write(_("Created unit file %s\n") % self.unitfile) def __add_dir(self, newd): if newd in self.all_dirs: @@ -229,7 +229,7 @@ WantedBy=%(TARGET)s def get_name(self): if self.config: return self.config.get_name() - raise ValueError(["Name not configured"]) + raise ValueError([_("Name not configured")]) def set_copy(self, copy): self.copy = copy @@ -255,10 +255,10 @@ WantedBy=%(TARGET)s selabel = self.get_security_label() if selabel is None: - raise ValueError(["Missing security label configuration"]) + raise ValueError([_("Missing security label configuration")]) parts = selabel.split(":") if len(parts) != 5 and len(parts) != 4: - raise ValueError(["Expected 5 parts in SELinux security label %s" % parts]) + raise ValueError([_("Expected 5 parts in SELinux security label %s") % parts]) if len(parts) == 5: selinux.chcon(self.dest, "system_u:object_r:%s:%s:%s" % ( @@ -276,7 +276,7 @@ WantedBy=%(TARGET)s def set_name(self, name): if self.config: - raise ValueError(["Cannot modify Name"]) + raise ValueError([_("Cannot modify Name")]) self.dest = "%s/%s" % (self.path, self.name) self.config = LibvirtSandbox.ConfigService.new(name) @@ -317,7 +317,7 @@ WantedBy=%(TARGET)s try: h = mi.next(); except exceptions.StopIteration: - raise ValueError(["Cannot find package containing %s" % unitfile]) + raise ValueError([_("Cannot find package containing %s") % unitfile]) for fentry in h.fiFromHeader(): fname = fentry[0] @@ -337,7 +337,7 @@ WantedBy=%(TARGET)s try: h = mi.next(); except exceptions.StopIteration: - raise ValueError(["Cannot find base package %s" % srcrpmbits[0]]) + raise ValueError([_("Cannot find base package %s") % srcrpmbits[0]]) for fentry in h.fiFromHeader(): fname = fentry[0] @@ -528,7 +528,7 @@ PrivateNetwork=false p = Popen(["/bin/umount", self.dest]) p.communicate() if p.returncode and p.returncode != 0: - raise OSError("Failed to unmount image %s from %s " % (self.image, self.dest)) + raise OSError(_("Failed to unmount image %s from %s") % (self.image, self.dest)) def __create_image(self): fd = open(self.image, "w") @@ -537,12 +537,12 @@ PrivateNetwork=false p = Popen(["/sbin/mkfs","-F", "-t", "ext4", self.image],stdout=PIPE, stderr=PIPE) p.communicate() if p.returncode and p.returncode != 0: - raise OSError("Failed to build image %s" % self.image ) + raise OSError(_("Failed to build image %s") % self.image ) p = Popen(["/bin/mount", self.image, self.dest]) p.communicate() if p.returncode and p.returncode != 0: - raise OSError("Failed to mount image %s on %s " % (self.image, self.dest)) + raise OSError(_("Failed to mount image %s on %s") % (self.image, self.dest)) def save_config(self): config = self.get_config_path() @@ -551,7 +551,7 @@ PrivateNetwork=false self.config.save_to_path(config) if selinux is not None: selinux.restorecon(config) - sys.stdout.write("Created sandbox config %s\n" % config) + sys.stdout.write(_("Created sandbox config %s\n") % config) def __get_image_path(self): mounts = self.config.get_mounts() @@ -593,7 +593,7 @@ PrivateNetwork=false p = Popen(["/usr/bin/systemctl","disable", self.unitfile],stdout=PIPE, stderr=PIPE) p.communicate() if p.returncode and p.returncode != 0: - raise OSError("Failed to disable %s unit file " % self.unitfile) + raise OSError(_("Failed to disable %s unit file") % self.unitfile) fd = open(self.unitfile,"r") recs = fd.readlines() fd.close() @@ -628,17 +628,17 @@ PrivateNetwork=false self.__create_image() self.__gen_content() self.__umount() - sys.stdout.write("Created sandbox container image %s\n" % self.image) + sys.stdout.write(_("Created sandbox container image %s\n") % self.image) else: self.__gen_content() - sys.stdout.write("Created sandbox container dir %s\n" % self.dest) + sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) self.set_security_label() self.save_config() self.__create_system_unit() def create(self): if os.path.exists(self.dest): - raise OSError("%s already exists" % self.dest) + raise OSError(_("%s already exists") % self.dest) try: self._create() @@ -868,11 +868,11 @@ def clone(args): new_image_path = container.get_image_path(args.dest) newrec = newrec.replace(old_image_path, new_image_path) shutil.copy(old_image_path, new_image_path) - sys.stdout.write("Created sandbox container image %s\n" % new_image_path) + sys.stdout.write(_("Created sandbox container image %s\n") % new_image_path) os.mkdir(new_path) else: shutil.copytree(old_path, new_path, symlinks=True) - sys.stdout.write("Created sandbox container dir %s\n" % new_path) + sys.stdout.write(_("Created sandbox container dir %s\n") % new_path) fd = open(container.get_unit_path()) recs = fd.read() @@ -886,7 +886,7 @@ def clone(args): fd = open(new_unit,"wx") fd.write(newrec) fd.close() - sys.stdout.write("Created unit file %s\n" % new_unit) + sys.stdout.write(_("Created unit file %s\n") % new_unit) container = Container(args.dest, args.uri) if args.security: @@ -907,7 +907,7 @@ class CheckUnit(argparse.Action): if not os.path.exists(src): src = "/lib/systemd/system/" + values if not os.path.exists(src): - raise OSError("Requested unit %s does not exist" % values) + raise OSError(_("Requested unit %s does not exist") % values) unitfiles = getattr(namespace, self.dest) if unitfiles: unitfiles.append((values, src)) @@ -974,7 +974,7 @@ def gen_connect_args(subparser): def gen_execute_args(subparser): parser = subparser.add_parser("execute", - help=("Execute a command within a sandbox container")) + help=_("Execute a command within a sandbox container")) parser.add_argument("-N", "--noseclabel", dest="noseclabel", default=False, action="store_true", help=_("do not modify the label of the executable process. By default all commands execute with the label of the sandbox")) @@ -1015,7 +1015,7 @@ def gen_clone_args(subparser): parser.add_argument("source", help=_("source sandbox container name")) parser.add_argument("dest", - help=("dest name of the new sandbox container")) + help=_("dest name of the new sandbox container")) def gen_delete_args(subparser): parser = subparser.add_parser("delete", -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:22PM -0400, Dan Walsh wrote:
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-)
Well, we haven't enabled localization at all in libvirt-sandbox, but this doesn't hurt, so ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Python makes assumptions about __METHOD names that will break some of the other patches that I am adding, involving inheritance of classes. The _METHODS are treated the same as any methods, but still give maintainers an idea that they should not be used. Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 108 +++++++++++++++++++++++------------------------ 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index a064e9a..7f4dcc8 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -103,7 +103,7 @@ class Container: except GLib.GError, e: raise OSError(config + ": " + str(e)) - def __follow_units(self): + def _follow_units(self): unitst="" for i, src in self.unit_file_list: unitst += "ReloadPropagatedFrom=%s\n" % i @@ -118,17 +118,17 @@ class Container: def set_unit_file_list(self, unit_file_list): self.unit_file_list = unit_file_list - def __get_sandboxed_service(self): + def _get_sandboxed_service(self): return self.unit_file_list[0][0].split(".")[0] - def __get_sandbox_target(self): + def _get_sandbox_target(self): if len(self.unit_file_list) > 1: raise ValueError([_("Only Valid for single units")]) - return "%s_sandbox.target" % self.__get_sandboxed_service() + return "%s_sandbox.target" % self._get_sandboxed_service() - def __target(self): + def _target(self): try: - name = self.__get_sandbox_target() + name = self._get_sandbox_target() path = "/etc/systemd/system/" + name if not os.path.exists(path): fd = open(path, "w") @@ -147,12 +147,12 @@ Documentation=man:virt-sandbox-service(1) [Install] WantedBy=multi-user.target -""" % { "NAME" : self.__get_sandboxed_service() }) +""" % { "NAME" : self._get_sandboxed_service() }) return "%s" % name except OSError: return "" - def __create_system_unit(self): + def _create_system_unit(self): self.unitfile = self.get_unit_path() name = self.config.get_name() unit = r""" @@ -169,7 +169,7 @@ ExecStop=/usr/bin/virt-sandbox-service stop %(NAME)s [Install] WantedBy=%(TARGET)s -""" % { 'NAME':name, 'FOLLOW':self.__follow_units(), 'TARGET':self.__target(), 'RELOAD': " -u ".join(map(lambda x: x[0], self.unit_file_list)) } +""" % { 'NAME':name, 'FOLLOW':self._follow_units(), 'TARGET':self._target(), 'RELOAD': " -u ".join(map(lambda x: x[0], self.unit_file_list)) } fd = open(self.unitfile, "w") fd.write(unit) fd.close() @@ -181,7 +181,7 @@ WantedBy=%(TARGET)s raise OSError(_("Failed to enable %s unit file") % self.unitfile) sys.stdout.write(_("Created unit file %s\n") % self.unitfile) - def __add_dir(self, newd): + def _add_dir(self, newd): if newd in self.all_dirs: return for ignd in self.IGNORE_DIRS: @@ -200,7 +200,7 @@ WantedBy=%(TARGET)s self.dirs.append(newd) break; - def __add_file(self, newf): + def _add_file(self, newf): if newf in self.files: return for d in self.IGNORE_DIRS: @@ -280,7 +280,7 @@ WantedBy=%(TARGET)s self.dest = "%s/%s" % (self.path, self.name) self.config = LibvirtSandbox.ConfigService.new(name) - def __extract_rpms(self): + def _extract_rpms(self): self.all_dirs = [] self.dirs = [] self.files = [] @@ -288,9 +288,9 @@ WantedBy=%(TARGET)s self.ts = rpm.ts() for u, src in self.unit_file_list: - self.__extract_rpm_for_unit(src) + self._extract_rpm_for_unit(src) - def __split_filename(self, filename): + def _split_filename(self, filename): if filename[-4:] == '.rpm': filename = filename[:-4] @@ -312,7 +312,7 @@ WantedBy=%(TARGET)s name = filename[epochIndex + 1:verIndex] return name, ver, rel, epoch, arch - def __extract_rpm_for_unit(self, unitfile): + def _extract_rpm_for_unit(self, unitfile): mi = self.ts.dbMatch(rpm.RPMTAG_BASENAMES, unitfile) try: h = mi.next(); @@ -323,12 +323,12 @@ WantedBy=%(TARGET)s fname = fentry[0] if os.path.isdir(fname): - self.__add_dir(fname) + self._add_dir(fname) if os.path.isfile(fname): - self.__add_file(fname) + self._add_file(fname) srcrpm = h[rpm.RPMTAG_SOURCERPM] - srcrpmbits = self.__split_filename(srcrpm) + srcrpmbits = self._split_filename(srcrpm) if srcrpmbits[0] == h[rpm.RPMTAG_NAME]: return @@ -343,11 +343,11 @@ WantedBy=%(TARGET)s fname = fentry[0] if os.path.isdir(fname): - self.__add_dir(fname) + self._add_dir(fname) if os.path.isfile(fname): - self.__add_file(fname) + self._add_file(fname) - def __add_mount(self, source, dest): + def _add_mount(self, source, dest): mount = LibvirtSandbox.ConfigMountHostBind.new(source, dest) self.config.add_mount(mount) @@ -376,7 +376,7 @@ WantedBy=%(TARGET)s os.symlink(self.dest + jpath, jpath) - def __gen_filesystems(self): + def _gen_filesystems(self): if self.use_image: self.image = self.DEFAULT_IMAGE % self.get_name() mount = LibvirtSandbox.ConfigMountHostImage.new(self.image, self.dest) @@ -397,12 +397,12 @@ WantedBy=%(TARGET)s for d in self.BIND_SYSTEM_DIRS: if os.path.exists(d): source = "%s%s" % ( self.dest, d) - self.__add_mount(source, d) + self._add_mount(source, d) for f in self.BIND_SYSTEM_FILES: if os.path.exists(f): source = "%s%s" % ( self.dest, f) - self.__add_mount(source, f) + self._add_mount(source, f) for d in self.dirs: found = False @@ -413,12 +413,12 @@ WantedBy=%(TARGET)s break if not found: source = "%s%s" % ( self.dest, d) - self.__add_mount(source, d) + self._add_mount(source, d) - def __get_init_path(self): + def _get_init_path(self): return "%s/%s/init" % (self.path, self.get_name()) - def __create_container_unit(self, src, dest, unit): + def _create_container_unit(self, src, dest, unit): fd = open(dest + "/" + unit, "w") fd.write(""".include %s [Service] @@ -427,7 +427,7 @@ PrivateNetwork=false """ % src ) fd.close() - def __fix_stat(self, f): + def _fix_stat(self, f): try: s = os.stat(f) path = "%s%s" % (self.dest, f) @@ -437,15 +437,15 @@ PrivateNetwork=false if not e.errno == errno.ENOENT: raise e - def __fix_protection(self): + def _fix_protection(self): l = len(self.dest) for root, dirs, files in os.walk(self.dest): for f in files: dest = root + "/" + f - self.__fix_stat(dest[l:]) + self._fix_stat(dest[l:]) for d in dirs: dest = root + "/" + d - self.__fix_stat(dest[l:]) + self._fix_stat(dest[l:]) def makedirs(self, d): try: @@ -465,7 +465,7 @@ PrivateNetwork=false if not e.errno == errno.EEXIST: raise e - def __gen_content(self): + def _gen_content(self): if self.copy: for d in self.dirs: shutil.copytree(d, "%s%s" % (self.dest, d), symlinks=True) @@ -505,7 +505,7 @@ PrivateNetwork=false os.symlink("/run", self.dest + "/var/run") for i, src in self.unit_file_list: - self.__create_container_unit(src, self.dest + unitdir, i) + self._create_container_unit(src, self.dest + unitdir, i) os.symlink("../" + i, self.dest + tgtdir + "/" + i) tgtfile = unitdir + "/multi-user.target" @@ -522,15 +522,15 @@ PrivateNetwork=false profile = "/etc/skel/" + p shutil.copy(profile, self.dest + "/root/") - self.__fix_protection() + self._fix_protection() - def __umount(self): + def _umount(self): p = Popen(["/bin/umount", self.dest]) p.communicate() if p.returncode and p.returncode != 0: raise OSError(_("Failed to unmount image %s from %s") % (self.image, self.dest)) - def __create_image(self): + def _create_image(self): fd = open(self.image, "w") fd.truncate(self.size) fd.close() @@ -553,7 +553,7 @@ PrivateNetwork=false selinux.restorecon(config) sys.stdout.write(_("Created sandbox config %s\n") % config) - def __get_image_path(self): + def _get_image_path(self): mounts = self.config.get_mounts() for m in mounts: if type(m) != LibvirtSandbox.ConfigMountHostImage: @@ -577,7 +577,7 @@ PrivateNetwork=false if os.path.exists(self.dest): shutil.rmtree(self.dest) - mount_path = self.__get_image_path() + mount_path = self._get_image_path() if mount_path: os.remove(mount_path) @@ -618,23 +618,23 @@ PrivateNetwork=false self._delete() def _create(self): - self.__connect() + self._connect() self.config.set_shell(True) self.config.set_boot_target("multi-user.target") - self.__extract_rpms() - self.__gen_filesystems() + self._extract_rpms() + self._gen_filesystems() os.mkdir(self.dest) if self.image: - self.__create_image() - self.__gen_content() - self.__umount() + self._create_image() + self._gen_content() + self._umount() sys.stdout.write(_("Created sandbox container image %s\n") % self.image) else: - self.__gen_content() + self._gen_content() sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) self.set_security_label() self.save_config() - self.__create_system_unit() + self._create_system_unit() def create(self): if os.path.exists(self.dest): @@ -661,12 +661,12 @@ PrivateNetwork=false args.command = [ "systemctl", "reload" ] + map(lambda x: x[0], unitfiles) execute(args) - def __connect(self): + def _connect(self): if not self.conn: self.conn=LibvirtGObject.Connection.new(self.uri) self.conn.open(None) - def __disconnect(self): + def _disconnect(self): if self.conn: self.conn.close() self.conn = None @@ -675,10 +675,10 @@ PrivateNetwork=false def closed(obj, error): self.loop.quit() try: - self.__connect() + self._connect() context = LibvirtSandbox.ContextService.new(self.conn, self.config) context.attach() - self.__disconnect() + self._disconnect() return 1 except GLib.GError, e: return 0 @@ -688,7 +688,7 @@ PrivateNetwork=false self.loop.quit() try: - self.__connect() + self._connect() context = LibvirtSandbox.ContextService.new(self.conn, self.config) context.start() console = context.get_log_console() @@ -709,7 +709,7 @@ PrivateNetwork=false def stop(self): try: - self.__connect() + self._connect() context = LibvirtSandbox.ContextService.new(self.conn, self.config) context.attach() context.stop() @@ -721,7 +721,7 @@ PrivateNetwork=false self.loop.quit() try: - self.__connect() + self._connect() context = LibvirtSandbox.ContextService.new(self.conn, self.config) context.attach() console = context.get_shell_console() @@ -741,7 +741,7 @@ PrivateNetwork=false self.loop.quit() try: - self.__connect() + self._connect() context = LibvirtSandbox.ContextService.new(self.conn, self.config) context.attach() console = context.get_shell_console() -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:23PM -0400, Dan Walsh wrote:
Python makes assumptions about __METHOD names that will break some of the other patches that I am adding, involving inheritance of classes. The _METHODS are treated the same as any methods, but still give maintainers an idea that they should not be used.
None of this code is public API, so I'm not sure there's any point in even using '_' - everything is internal. So I'd just remove all use of leading '_' in method names. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/03/2013 07:28 AM, Daniel P. Berrange wrote:
On Tue, Apr 02, 2013 at 06:11:23PM -0400, Dan Walsh wrote:
Python makes assumptions about __METHOD names that will break some of the other patches that I am adding, involving inheritance of classes. The _METHODS are treated the same as any methods, but still give maintainers an idea that they should not be used.
None of this code is public API, so I'm not sure there's any point in even using '_' - everything is internal. So I'd just remove all use of leading '_' in method names.
Daniel
I was using them more as an indicator to myself and future maintainers which interfaces to use publicly. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlFcOHYACgkQrlYvE4MpobMdtACggGLOeNQ330fcqzq0tXIAxxin KdEAoLZqMFkYoW4fuAZo8KKJYkB2xyvP =YHFN -----END PGP SIGNATURE-----

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 7f4dcc8..b816933 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -50,11 +50,17 @@ except IOError: import __builtin__ __builtin__.__dict__['_'] = unicode +CONFIG_PATH = "/etc/libvirt-sandbox/services/" +def get_config_path(name): + return CONFIG_PATH + name + ".sandbox" + +def read_config(name): + return LibvirtSandbox.Config.load_from_path(get_config_path(name)) + class Container: IGNORE_DIRS = [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ] DEFAULT_DIRS = [ "/etc", "/var" ] PROFILE_FILES = [ ".bashrc", ".bash_profile" ] - CONFIG_PATH = "/etc/libvirt-sandbox/services/" MACHINE_ID = "/etc/machine-id" HOSTNAME = "/etc/hostname" FUNCTIONS = "/etc/rc.d/init.d/functions" @@ -214,7 +220,7 @@ WantedBy=%(TARGET)s def get_config_path(self, name = None): if not name: name = self.get_name() - return self.CONFIG_PATH + name + ".sandbox" + return get_config_path() def get_filesystem_path(self, name = None): if not name: @@ -789,8 +795,7 @@ def usage(parser, msg): def sandbox_list(args): import glob - container = Container() - g = glob.glob(container.CONFIG_PATH + "*.sandbox") + g = glob.glob(CONFIG_PATH + "*.sandbox") g.sort() for gc in g: try: -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:24PM -0400, Dan Walsh wrote: The subject line is too long again. Please re-write with a short first line, then a blank line, and then the full description
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
ACK, if the commit message is fixed before pushing. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index b816933..ca472f5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -1074,3 +1074,7 @@ if __name__ == '__main__': sys.stderr.write("%s: %s\n" % (sys.argv[0], e)) sys.stderr.flush() sys.exit(1) + except GLib.GError, e: + sys.stderr.write("%s: %s\n" % (sys.argv[0], e)) + sys.stderr.flush() + sys.exit(1) -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:25PM -0400, Dan Walsh wrote: Again, bad subject line which is too long.
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index b816933..ca472f5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -1074,3 +1074,7 @@ if __name__ == '__main__': sys.stderr.write("%s: %s\n" % (sys.argv[0], e)) sys.stderr.flush() sys.exit(1) + except GLib.GError, e: + sys.stderr.write("%s: %s\n" % (sys.argv[0], e)) + sys.stderr.flush() + sys.exit(1)
ACK if the commit message is fixed. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index ca472f5..f32fd4a 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -551,13 +551,13 @@ PrivateNetwork=false raise OSError(_("Failed to mount image %s on %s") % (self.image, self.dest)) def save_config(self): - config = self.get_config_path() - if os.path.exists(config): - os.remove(config) - self.config.save_to_path(config) + config_path = self.get_config_path() + if os.path.exists(config_path): + os.remove(config_path) + self.config.save_to_path(config_path) if selinux is not None: - selinux.restorecon(config) - sys.stdout.write(_("Created sandbox config %s\n") % config) + selinux.restorecon(config_path) + sys.stdout.write(_("Created sandbox config %s\n") % config_path) def _get_image_path(self): mounts = self.config.get_mounts() -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:26PM -0400, Dan Walsh wrote:
Signed-off-by: Dan Walsh <dwalsh@redhat.com> --- bin/virt-sandbox-service | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index ca472f5..f32fd4a 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -551,13 +551,13 @@ PrivateNetwork=false raise OSError(_("Failed to mount image %s on %s") % (self.image, self.dest))
def save_config(self): - config = self.get_config_path() - if os.path.exists(config): - os.remove(config) - self.config.save_to_path(config) + config_path = self.get_config_path() + if os.path.exists(config_path): + os.remove(config_path) + self.config.save_to_path(config_path) if selinux is not None: - selinux.restorecon(config) - sys.stdout.write(_("Created sandbox config %s\n") % config) + selinux.restorecon(config_path) + sys.stdout.write(_("Created sandbox config %s\n") % config_path)
def _get_image_path(self): mounts = self.config.get_mounts()
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- bin/virt-sandbox-service | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index f32fd4a..9f4941b 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -453,7 +453,7 @@ PrivateNetwork=false dest = root + "/" + d self._fix_stat(dest[l:]) - def makedirs(self, d): + def _makedirs(self, d): try: path = "%s%s" % (self.dest, d) os.makedirs(path) @@ -461,8 +461,8 @@ PrivateNetwork=false if not e.errno == errno.EEXIST: raise e - def makefile(self, f): - self.makedirs(os.path.dirname(f)) + def _makefile(self, f): + self._makedirs(os.path.dirname(f)) try: path = "%s%s" % (self.dest, f) fd=open(path, "w") @@ -476,20 +476,20 @@ PrivateNetwork=false for d in self.dirs: shutil.copytree(d, "%s%s" % (self.dest, d), symlinks=True) for f in self.files: - self.makedirs(os.path.dirname(f)) + self._makedirs(os.path.dirname(f)) shutil.copy(f, "%s%s" % (self.dest, f)) else: for d in self.all_dirs: - self.makedirs(d) + self._makedirs(d) for f in self.files: - self.makedirs(os.path.dirname(f)) - self.makefile(f) + self._makedirs(os.path.dirname(f)) + self._makefile(f) for d in self.BIND_SYSTEM_DIRS + self.MAKE_SYSTEM_DIRS: - self.makedirs(d) + self._makedirs(d) for f in self.BIND_SYSTEM_FILES: - self.makefile(f) + self._makefile(f) shutil.copy(self.FUNCTIONS, "%s%s" % (self.dest, self.FUNCTIONS)) @@ -506,8 +506,8 @@ PrivateNetwork=false unitdir = "/etc/systemd/system" tgtdir = unitdir + "/multi-user.target.wants" - self.makedirs(unitdir) - self.makedirs(tgtdir) + self._makedirs(unitdir) + self._makedirs(tgtdir) os.symlink("/run", self.dest + "/var/run") for i, src in self.unit_file_list: -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:27PM -0400, Dan Walsh wrote:
--- bin/virt-sandbox-service | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Per my earlier comments, I'd rather not use "_" anywhere here. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This way we can share common methods between the ServiceContainer and the InteractiveContainer --- bin/virt-sandbox-service | 823 ++++++++++++++++++++++++++--------------------- 1 file changed, 450 insertions(+), 373 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 9f4941b..f4d0eff 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -58,56 +58,387 @@ def read_config(name): return LibvirtSandbox.Config.load_from_path(get_config_path(name)) class Container: + DEFAULT_PATH = "/var/lib/libvirt/filesystems" + DEFAULT_IMAGE = "/var/lib/libvirt/images/%s.raw" + SELINUX_FILE_TYPE = "svirt_lxc_file_t" + + def __init__(self, name=None, uri = "lxc:///", path = DEFAULT_PATH, config=None, create=False): + self.uri = uri + self.use_image = False + self.size = 10 * MB + self.path = path + self.config = None + if self.config: + self.name = self.config.get_name() + else: + self.name = name + self.dest = "%s/%s" % (self.path, self.name) + self.file_type = self.SELINUX_FILE_TYPE + self.conn = None + self.image = None + self.uid = 0 + + def get_file_type(self): + return self.file_type + + def set_file_type(self, file_type): + self.file_type = file_type + + def set_uid(self, uid): + self.uid = uid + + def get_uid(self): + return self.uid + + def get_config_path(self, name = None): + if not name: + name = self.name + return get_config_path(name) + + def get_filesystem_path(self, name = None): + if not name: + name = self.get_name() + return self.DEFAULT_PATH + "/" + name + + def get_image_path(self, name = None): + if not name: + name = self.get_name() + return self.DEFAULT_IMAGE % name + + def set_image(self, size): + self.use_image = True + self.size = size * MB + + def set_path(self, path): + self.path = path + self.dest = "%s/%s" % (self.path, self.name) + + def get_name(self): + return self.name + + def set_name(self, name): + if self.config: + raise ValueError([_("Cannot modify Name")]) + self.name = name + self.dest = "%s/%s" % (self.path, self.name) + + def set_security(self, val): + return self.config.set_security_opts(val) + + def add_network(self, val): + return self.config.add_network_opts(val) + + def get_security_dynamic(self): + return self.config.get_security_dynamic() + + def get_security_type(self): + try: + if self.config: + con = self.config.get_security_label().split(':') + return con[2] + except: + pass + return "svirt_lxc_net_t" + + def get_security_level(self): + try: + if self.config: + con = self.config.get_security_label().split(':') + return ":".join(con[3:]) + except: + pass + return "s0" + + def get_security_label(self): + return self.config.get_security_label() + + def set_security_label(self): + if selinux is None: + return + + if self.image or self.get_security_dynamic(): + return + + selabel = self.get_security_label() + if selabel is None: + raise ValueError([_("Missing security label configuration")]) + parts = selabel.split(":") + selinux.chcon(self.dest, "system_u:object_r:%s:%s" % ( + self.get_file_type(), ":".join(parts[3:])), True) + + def set_security_type(self, security_type): + label = "system_u:system_r:%s:%s" % (security_type, self.get_security_level()) + try: + selinux.security_check_context(label) + self.config.set_security_label(label) + except OSError, e: + raise OSError(_("Invalid Security Type %s: %s ") % (security_type, e)) + + def set_security_level(self, security_level): + label = "system_u:system_r:%s:%s" % (self.get_security_type(), security_level) + try: + selinux.security_check_context(label) + self.config.set_security_label(label) + except OSError, e: + raise OSError(_("Invalid Security Level %s: %s ") % (security_level, e)) + + def _gen_filesystems(self): + if self.use_image: + self.image = self.DEFAULT_IMAGE % self.get_name() + mount = LibvirtSandbox.ConfigMountHostImage.new(self.image, self.dest) + self.config.add_mount(mount) + + # 10 MB /run + mount = LibvirtSandbox.ConfigMountRam.new("/run", 10 * 1024 * 1024); + self.config.add_mount(mount) + + # 100 MB /tmp + mount = LibvirtSandbox.ConfigMountRam.new("/tmp", 100 * 1024 * 1024); + self.config.add_mount(mount) + + # 100 MB /tmp + mount = LibvirtSandbox.ConfigMountRam.new("/dev/shm", 100 * 1024 * 1024); + self.config.add_mount(mount) + + def _fix_stat(self, f): + try: + s = os.stat(f) + path = "%s%s" % (self.dest, f) + os.chown(path, s.st_uid, s.st_gid) + os.chmod(path, s.st_mode) + except OSError, e: + if not e.errno == errno.ENOENT: + raise e + + def _fix_protection(self): + l = len(self.dest) + for root, dirs, files in os.walk(self.dest): + for f in files: + dest = root + "/" + f + self._fix_stat(dest[l:]) + for d in dirs: + dest = root + "/" + d + self._fix_stat(dest[l:]) + + def _makedirs(self, d): + try: + path = "%s%s" % (self.dest, d) + os.makedirs(path) + except OSError, e: + if not e.errno == errno.EEXIST: + raise e + + def _makefile(self, f): + self._makedirs(os.path.dirname(f)) + try: + path = "%s%s" % (self.dest, f) + fd=open(path, "w") + fd.close() + except OSError, e: + if not e.errno == errno.EEXIST: + raise e + + def _umount(self): + p = Popen(["/bin/umount", self.dest]) + p.communicate() + if p.returncode and p.returncode != 0: + raise OSError(_("Failed to unmount image %s from %s") % (self.image, self.dest)) + + def _create_image(self): + fd = open(self.image, "w") + fd.truncate(self.size) + fd.close() + p = Popen(["/sbin/mkfs","-F", "-t", "ext4", self.image],stdout=PIPE, stderr=PIPE) + p.communicate() + if p.returncode and p.returncode != 0: + raise OSError(_("Failed to build image %s") % self.image ) + + p = Popen(["/bin/mount", self.image, self.dest]) + p.communicate() + if p.returncode and p.returncode != 0: + raise OSError(_("Failed to mount image %s on %s") % (self.image, self.dest)) + + def save_config(self): + config_path = self.get_config_path() + if os.path.exists(config_path): + os.remove(config_path) + self.config.save_to_path(config_path) + if selinux is not None: + selinux.restorecon(config_path) + sys.stdout.write(_("Created sandbox config %s\n") % config_path) + + def _get_image_path(self): + mounts = self.config.get_mounts() + for m in mounts: + if type(m) != LibvirtSandbox.ConfigMountHostImage: + continue + + if m.get_target() == self.dest: + return m.get_source() + return None + + def delete(self): + # Stop service if it is running + try: + self.stop() + except: + pass + + config = self.get_config_path() + if os.path.exists(config): + os.remove(config) + + print (_("You need to manually remove the '%s' content") % self.name) + return + + # Not sure we should remove content + if os.path.exists(self.dest): + shutil.rmtree(self.dest) + mount_path = self._get_image_path() + if mount_path: + os.remove(mount_path) + + image = self.get_image_path() + if os.path.exists(image): + os.remove(image) + + def create(self): + self._connect() + self.config.set_shell(True) + os.mkdir(self.dest) + + def _connect(self): + if not self.conn: + self.conn=LibvirtGObject.Connection.new(self.uri) + self.conn.open(None) + + def _disconnect(self): + if self.conn: + self.conn.close() + self.conn = None + + def running(self): + def closed(obj, error): + self.loop.quit() + try: + self._connect() + context = LibvirtSandbox.ContextService.new(self.conn, self.config) + context.attach() + self._disconnect() + return 1 + except GLib.GError, e: + return 0 + + def start(self): + def closed(obj, error): + self.loop.quit() + + try: + self._connect() + context = LibvirtSandbox.ContextService.new(self.conn, self.config) + context.start() + console = context.get_log_console() + console.connect("closed", closed) + console.attach_stderr() + self.loop = GLib.MainLoop() + self.loop.run() + try: + console.detach() + except: + pass + try: + context.stop() + except: + pass + except GLib.GError, e: + raise OSError(str(e)) + + def stop(self): + try: + self._connect() + context = LibvirtSandbox.ContextService.new(self.conn, self.config) + context.attach() + context.stop() + except GLib.GError, e: + raise OSError(str(e)) + + def connect(self): + def closed(obj, error): + self.loop.quit() + + try: + self._connect() + context = LibvirtSandbox.ContextService.new(self.conn, self.config) + context.attach() + console = context.get_shell_console() + console.connect("closed", closed) + console.attach_stdio() + self.loop = GLib.MainLoop() + self.loop.run() + try: + console.detach() + except: + pass + except GLib.GError, e: + raise OSError(str(e)) + + def execute(self, command): + def closed(obj, error): + self.loop.quit() + + try: + self._connect() + context = LibvirtSandbox.ContextService.new(self.conn, self.config) + context.attach() + console = context.get_shell_console() + console.connect("closed", closed) + console.attach_stdio() + print "not currently implemented" + console.detach() + return + self.loop = GLib.MainLoop() + self.loop.run() + try: + console.detach() + except: + pass + except GLib.GError, e: + raise OSError(str(e)) + + def add_bind_mount(self, source, dest): + mount = LibvirtSandbox.ConfigMountHostBind.new(source, dest) + self.config.add_mount(mount) + + def add_ram_mount(self, dest, size): + mount = LibvirtSandbox.ConfigMountRam.new(dest, size); + self.config.add_mount(mount) + +class ServiceContainer(Container): IGNORE_DIRS = [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ] DEFAULT_DIRS = [ "/etc", "/var" ] PROFILE_FILES = [ ".bashrc", ".bash_profile" ] MACHINE_ID = "/etc/machine-id" HOSTNAME = "/etc/hostname" - FUNCTIONS = "/etc/rc.d/init.d/functions" + SYSVINIT_PATH = "/etc/rc.d" ANACONDA_WANTS_PATH = "/usr/lib/systemd/system/anaconda.target.wants" MULTI_USER_WANTS_PATH = "/usr/lib/systemd/system/multi-user.target.wants" SYSINIT_WANTS_PATH = "/usr/lib/systemd/system/sysinit.target.wants" SOCKET_WANTS_PATH = "/usr/lib/systemd/system/sockets.target.wants" - MAKE_SYSTEM_DIRS = [ "/var/lib/dhclient", "/var/lib/dbus", "/var/log", "/var/spool", "/var/cache", "/var/tmp", "/var/lib/nfs/rpc_pipefs", "/etc/rc.d/init.d"] + MAKE_SYSTEM_DIRS = [ "/var/lib/dhclient", "/var/lib/dbus", "/var/log", "/var/spool", "/var/cache", "/var/tmp", "/var/lib/nfs/rpc_pipefs", SYSVINIT_PATH ] BIND_SYSTEM_DIRS = [ "/var", "/home", "/root", "/etc/systemd/system", "/etc/rc.d", "/usr/lib/systemd/system/basic.target.wants", "/usr/lib/systemd/system/local-fs.target.wants", ANACONDA_WANTS_PATH, MULTI_USER_WANTS_PATH, SYSINIT_WANTS_PATH, SOCKET_WANTS_PATH ] BIND_SYSTEM_FILES = [ MACHINE_ID, "/etc/fstab", HOSTNAME ] LOCAL_LINK_FILES = { SYSINIT_WANTS_PATH : [ "systemd-tmpfiles-setup.service" ] , SOCKET_WANTS_PATH : [ "dbus.socket", "systemd-journald.socket", "systemd-shutdownd.socket" ] } - SELINUX_FILE_TYPE = "svirt_lxc_file_t" - DEFAULT_PATH = "/var/lib/libvirt/filesystems" - DEFAULT_IMAGE = "/var/lib/libvirt/images/%s.raw" DEFAULT_UNIT = "/etc/systemd/system/%s_sandbox.service" - def __init__(self, name=None, uri = "lxc:///", path = DEFAULT_PATH, config=None, create=False): - self.uri = uri + def __init__(self, name=None, uri = "lxc:///", path = Container.DEFAULT_PATH, config=None, create=False): + Container.__init__(self, name, uri, path, config, create) self.copy = False - self.use_image = False - self.path = path - self.conn = None - self.image = None - self.size = 10 * MB - self.config = None - self.unitfile = None self.unit_file_list = [] - self.name = name - - if name: - config = self.get_config_path(name) - self.dest = "%s/%s" % (self.path, name) - if create: - if config and os.path.exists(config): - raise ValueError([_("Container already exists")]) - self.config = LibvirtSandbox.ConfigService.new(name) - return - - if config: - try: - self.config = LibvirtSandbox.Config.load_from_path(config) - self.unitfile = self.get_unit_path() - except GLib.GError, e: - raise OSError(config + ": " + str(e)) + else: + self.unitfile = self.get_unit_path() def _follow_units(self): unitst="" @@ -128,14 +459,11 @@ class Container: return self.unit_file_list[0][0].split(".")[0] def _get_sandbox_target(self): - if len(self.unit_file_list) > 1: - raise ValueError([_("Only Valid for single units")]) return "%s_sandbox.target" % self._get_sandboxed_service() - def _target(self): + def _create_target(self, name): try: - name = self._get_sandbox_target() - path = "/etc/systemd/system/" + name + path = "/etc/systemd/system/%s_sandbox.target" % name if not os.path.exists(path): fd = open(path, "w") fd.write(""" @@ -151,16 +479,18 @@ class Container: Description=%(NAME)s Sandbox Container Target Documentation=man:virt-sandbox-service(1) -[Install] -WantedBy=multi-user.target """ % { "NAME" : self._get_sandboxed_service() }) - return "%s" % name + except ValueError: + return "" except OSError: return "" + path += ".wants" + if not os.path.exists(path): + os.makedirs(path) + os.symlink(self.get_unit_path(), path + "/" + os.path.basename(self.get_unit_path())) def _create_system_unit(self): self.unitfile = self.get_unit_path() - name = self.config.get_name() unit = r""" [Unit] Description=Secure Sandbox Container %(NAME)s @@ -175,17 +505,16 @@ ExecStop=/usr/bin/virt-sandbox-service stop %(NAME)s [Install] WantedBy=%(TARGET)s -""" % { 'NAME':name, 'FOLLOW':self._follow_units(), 'TARGET':self._target(), 'RELOAD': " -u ".join(map(lambda x: x[0], self.unit_file_list)) } +""" % { 'NAME':self.name, 'FOLLOW':self._follow_units(), 'TARGET':self._get_sandbox_target(), 'RELOAD': " -u ".join(map(lambda x: x[0], self.unit_file_list)) } + fd = open(self.unitfile, "w") fd.write(unit) fd.close() if selinux is not None: selinux.restorecon(self.unitfile) - p = Popen(["/usr/bin/systemctl","enable", self.unitfile],stdout=PIPE, stderr=PIPE) - p.communicate() - if p.returncode and p.returncode != 0: - raise OSError(_("Failed to enable %s unit file") % self.unitfile) sys.stdout.write(_("Created unit file %s\n") % self.unitfile) + for i, src in self.unit_file_list: + self._create_target(i.split(".service")[0]) def _add_dir(self, newd): if newd in self.all_dirs: @@ -204,87 +533,29 @@ WantedBy=%(TARGET)s tmp_dirs.append(d) self.dirs = tmp_dirs self.dirs.append(newd) - break; - - def _add_file(self, newf): - if newf in self.files: - return - for d in self.IGNORE_DIRS: - if newf.startswith(d): - return - for d in self.DEFAULT_DIRS: - if newf.startswith(d): - self.files.append(newf) - break; - - def get_config_path(self, name = None): - if not name: - name = self.get_name() - return get_config_path() - - def get_filesystem_path(self, name = None): - if not name: - name = self.get_name() - return self.DEFAULT_PATH + "/" + name - - def get_image_path(self, name = None): - if not name: - name = self.get_name() - return self.DEFAULT_IMAGE % name - - def get_name(self): - if self.config: - return self.config.get_name() - raise ValueError([_("Name not configured")]) - - def set_copy(self, copy): - self.copy = copy - - def set_security(self, val): - return self.config.set_security_opts(val) - - def add_network(self, val): - return self.config.add_network_opts(val) - - def get_security_dynamic(self): - return self.config.get_security_dynamic() - - def get_security_label(self): - return self.config.get_security_label() - - def set_security_label(self): - if selinux is None: - return - - if self.image or self.get_security_dynamic(): - return - - selabel = self.get_security_label() - if selabel is None: - raise ValueError([_("Missing security label configuration")]) - parts = selabel.split(":") - if len(parts) != 5 and len(parts) != 4: - raise ValueError([_("Expected 5 parts in SELinux security label %s") % parts]) - - if len(parts) == 5: - selinux.chcon(self.dest, "system_u:object_r:%s:%s:%s" % ( - self.SELINUX_FILE_TYPE, parts[3], parts[4]), True) - else: - selinux.chcon(self.dest, "system_u:object_r:%s:%s" % ( - self.SELINUX_FILE_TYPE, parts[3]), True) - - def set_image(self, size): - self.use_image = True - self.size = size * MB + break; - def set_path(self, path): - self.path = path + def _add_file(self, newf): + if newf in self.files: + return + for d in self.IGNORE_DIRS: + if newf.startswith(d): + return + for d in self.DEFAULT_DIRS: + if newf.startswith(d): + self.files.append(newf) + break; - def set_name(self, name): + def get_name(self): if self.config: - raise ValueError([_("Cannot modify Name")]) - self.dest = "%s/%s" % (self.path, self.name) - self.config = LibvirtSandbox.ConfigService.new(name) + return self.config.get_name() + raise ValueError([_("Name not configured")]) + + def set_copy(self, copy): + self.copy = copy + + def get_security_dynamic(self): + return self.config.get_security_dynamic() def _extract_rpms(self): self.all_dirs = [] @@ -323,7 +594,7 @@ WantedBy=%(TARGET)s try: h = mi.next(); except exceptions.StopIteration: - raise ValueError([_("Cannot find package containing %s") % unitfile]) + raise ValueError(["Cannot find package containing %s" % unitfile]) for fentry in h.fiFromHeader(): fname = fentry[0] @@ -343,7 +614,7 @@ WantedBy=%(TARGET)s try: h = mi.next(); except exceptions.StopIteration: - raise ValueError([_("Cannot find base package %s") % srcrpmbits[0]]) + raise ValueError(["Cannot find base package %s" % srcrpmbits[0]]) for fentry in h.fiFromHeader(): fname = fentry[0] @@ -353,10 +624,6 @@ WantedBy=%(TARGET)s if os.path.isfile(fname): self._add_file(fname) - def _add_mount(self, source, dest): - mount = LibvirtSandbox.ConfigMountHostBind.new(source, dest) - self.config.add_mount(mount) - def gen_hostname(self): fd=open(self.dest + self.HOSTNAME, "w") fd.write("%s\n" % self.name ) @@ -383,32 +650,16 @@ WantedBy=%(TARGET)s os.symlink(self.dest + jpath, jpath) def _gen_filesystems(self): - if self.use_image: - self.image = self.DEFAULT_IMAGE % self.get_name() - mount = LibvirtSandbox.ConfigMountHostImage.new(self.image, self.dest) - self.config.add_mount(mount) - - # 10 MB /run - mount = LibvirtSandbox.ConfigMountRam.new("/run", 10 * 1024 * 1024); - self.config.add_mount(mount) - - # 100 MB /tmp - mount = LibvirtSandbox.ConfigMountRam.new("/tmp", 100 * 1024 * 1024); - self.config.add_mount(mount) - - # 100 MB /tmp - mount = LibvirtSandbox.ConfigMountRam.new("/dev/shm", 100 * 1024 * 1024); - self.config.add_mount(mount) - + Container._gen_filesystems(self) for d in self.BIND_SYSTEM_DIRS: if os.path.exists(d): source = "%s%s" % ( self.dest, d) - self._add_mount(source, d) + self.add_bind_mount(source, d) for f in self.BIND_SYSTEM_FILES: if os.path.exists(f): source = "%s%s" % ( self.dest, f) - self._add_mount(source, f) + self.add_bind_mount(source, f) for d in self.dirs: found = False @@ -419,10 +670,7 @@ WantedBy=%(TARGET)s break if not found: source = "%s%s" % ( self.dest, d) - self._add_mount(source, d) - - def _get_init_path(self): - return "%s/%s/init" % (self.path, self.get_name()) + self.add_bind_mount(source, d) def _create_container_unit(self, src, dest, unit): fd = open(dest + "/" + unit, "w") @@ -433,44 +681,6 @@ PrivateNetwork=false """ % src ) fd.close() - def _fix_stat(self, f): - try: - s = os.stat(f) - path = "%s%s" % (self.dest, f) - os.chown(path, s.st_uid, s.st_gid) - os.chmod(path, s.st_mode) - except OSError, e: - if not e.errno == errno.ENOENT: - raise e - - def _fix_protection(self): - l = len(self.dest) - for root, dirs, files in os.walk(self.dest): - for f in files: - dest = root + "/" + f - self._fix_stat(dest[l:]) - for d in dirs: - dest = root + "/" + d - self._fix_stat(dest[l:]) - - def _makedirs(self, d): - try: - path = "%s%s" % (self.dest, d) - os.makedirs(path) - except OSError, e: - if not e.errno == errno.EEXIST: - raise e - - def _makefile(self, f): - self._makedirs(os.path.dirname(f)) - try: - path = "%s%s" % (self.dest, f) - fd=open(path, "w") - fd.close() - except OSError, e: - if not e.errno == errno.EEXIST: - raise e - def _gen_content(self): if self.copy: for d in self.dirs: @@ -491,10 +701,13 @@ PrivateNetwork=false for f in self.BIND_SYSTEM_FILES: self._makefile(f) - shutil.copy(self.FUNCTIONS, "%s%s" % (self.dest, self.FUNCTIONS)) + destpath = self.dest + self.SYSVINIT_PATH + for i in range(7): + os.mkdir(destpath+("/rc%s.d" % i)) + os.mkdir(destpath+"/init.d") + shutil.copy(self.SYSVINIT_PATH + "/init.d/functions" , destpath + "/init.d") self.gen_machine_id() - self.gen_hostname() for k in self.LOCAL_LINK_FILES: @@ -530,70 +743,20 @@ PrivateNetwork=false self._fix_protection() - def _umount(self): - p = Popen(["/bin/umount", self.dest]) - p.communicate() - if p.returncode and p.returncode != 0: - raise OSError(_("Failed to unmount image %s from %s") % (self.image, self.dest)) - - def _create_image(self): - fd = open(self.image, "w") - fd.truncate(self.size) - fd.close() - p = Popen(["/sbin/mkfs","-F", "-t", "ext4", self.image],stdout=PIPE, stderr=PIPE) - p.communicate() - if p.returncode and p.returncode != 0: - raise OSError(_("Failed to build image %s") % self.image ) - - p = Popen(["/bin/mount", self.image, self.dest]) - p.communicate() - if p.returncode and p.returncode != 0: - raise OSError(_("Failed to mount image %s on %s") % (self.image, self.dest)) - - def save_config(self): - config_path = self.get_config_path() - if os.path.exists(config_path): - os.remove(config_path) - self.config.save_to_path(config_path) - if selinux is not None: - selinux.restorecon(config_path) - sys.stdout.write(_("Created sandbox config %s\n") % config_path) - - def _get_image_path(self): - mounts = self.config.get_mounts() - for m in mounts: - if type(m) != LibvirtSandbox.ConfigMountHostImage: - continue - - if m.get_target() == self.dest: - return m.get_source() - return None - def _delete(self): - # Should be stored in config + # + # Delete A ServiceContainer + # try: fd=open(self.dest + self.MACHINE_ID, "r") uuid = fd.read().rstrip() fd.close() - jpath = "/var/log/journal/" + uuid - if os.path.exists(jpath): - os.remove(jpath) - except Exception, e: - pass - - if os.path.exists(self.dest): - shutil.rmtree(self.dest) - mount_path = self._get_image_path() - if mount_path: - os.remove(mount_path) - - config = self.get_config_path() - if os.path.exists(config): - os.remove(config) + os.remove("/var/log/journal/" + uuid) + except OSError, e: + sys.stderr.write("%s: %s\n" % (sys.argv[0], e)) + sys.stderr.flush() - image = self.get_image_path() - if os.path.exists(image): - os.remove(image) + Container.delete(self) if self.unitfile is not None and os.path.exists(self.unitfile): p = Popen(["/usr/bin/systemctl","disable", self.unitfile],stdout=PIPE, stderr=PIPE) @@ -624,12 +787,13 @@ PrivateNetwork=false self._delete() def _create(self): - self._connect() - self.config.set_shell(True) - self.config.set_boot_target("multi-user.target") + # + # Create A ServiceContainer + # self._extract_rpms() + Container.create(self) self._gen_filesystems() - os.mkdir(self.dest) + if self.image: self._create_image() self._gen_content() @@ -639,8 +803,9 @@ PrivateNetwork=false self._gen_content() sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) self.set_security_label() - self.save_config() self._create_system_unit() + self.config.set_boot_target("multi-user.target") + self.save_config() def create(self): if os.path.exists(self.dest): @@ -667,119 +832,22 @@ PrivateNetwork=false args.command = [ "systemctl", "reload" ] + map(lambda x: x[0], unitfiles) execute(args) - def _connect(self): - if not self.conn: - self.conn=LibvirtGObject.Connection.new(self.uri) - self.conn.open(None) - - def _disconnect(self): - if self.conn: - self.conn.close() - self.conn = None - - def running(self): - def closed(obj, error): - self.loop.quit() - try: - self._connect() - context = LibvirtSandbox.ContextService.new(self.conn, self.config) - context.attach() - self._disconnect() - return 1 - except GLib.GError, e: - return 0 - - def start(self): - def closed(obj, error): - self.loop.quit() - - try: - self._connect() - context = LibvirtSandbox.ContextService.new(self.conn, self.config) - context.start() - console = context.get_log_console() - console.connect("closed", closed) - console.attach_stderr() - self.loop = GLib.MainLoop() - self.loop.run() - try: - console.detach() - except: - pass - try: - context.stop() - except: - pass - except GLib.GError, e: - raise OSError(str(e)) - - def stop(self): - try: - self._connect() - context = LibvirtSandbox.ContextService.new(self.conn, self.config) - context.attach() - context.stop() - except GLib.GError, e: - raise OSError(str(e)) - - def connect(self): - def closed(obj, error): - self.loop.quit() - - try: - self._connect() - context = LibvirtSandbox.ContextService.new(self.conn, self.config) - context.attach() - console = context.get_shell_console() - console.connect("closed", closed) - console.attach_stdio() - self.loop = GLib.MainLoop() - self.loop.run() - try: - console.detach() - except: - pass - except GLib.GError, e: - raise OSError(str(e)) - - def execute(self, command): - def closed(obj, error): - self.loop.quit() - - try: - self._connect() - context = LibvirtSandbox.ContextService.new(self.conn, self.config) - context.attach() - console = context.get_shell_console() - console.connect("closed", closed) - console.attach_stdio() - print "not currently implemented" - console.detach() - return - self.loop = GLib.MainLoop() - self.loop.run() - try: - console.detach() - except: - pass - except GLib.GError, e: - raise OSError(str(e)) - MB = int(1000000) def delete(args): - container = Container(args.name) + config = read_config(args.name) + container = ServiceContainer(uri=args.uri, config = config) container.delete() def create(args): - container = Container(name = args.name, create = True) + container = ServiceContainer(name = args.name, uri=args.uri, create = True) container.set_copy(args.copy) - if args.network: - for net in args.network: - container.add_network(net) + for net in args.network: + container.add_network(net) if args.security: container.set_security(args.security) container.set_unit_file_list(args.unitfiles) + container.set_path(args.path) if args.imagesize: container.set_image(args.imagesize) @@ -788,7 +856,6 @@ def create(args): def usage(parser, msg): parser.print_help() - sys.stderr.write("\n%s\n" % msg) sys.stderr.flush() sys.exit(1) @@ -799,7 +866,7 @@ def sandbox_list(args): g.sort() for gc in g: try: - c = Container(config = gc) + c = LibvirtSandbox.Config.load_from_path(gc) if args.running: if c.running(): print c.get_name() @@ -812,8 +879,6 @@ def sandbox_list(args): def sandbox_reload(args): config = read_config(args.name) - if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): - raise ValueError(_("Interactive Containers do not support reload")) container = ServiceContainer(uri = args.uri, config = config) container.reload(args.unitfiles) @@ -860,7 +925,8 @@ def execute(args): # os.execv("/usr/libexec/virt-sandbox-service-util", myexec) def clone(args): - container = Container(args.source, args.uri) + config = read_config(args.source) + container = Container(uri = args.uri, config=config) fd = open(container.get_config_path(),"r") recs = fd.read() fd.close() @@ -893,7 +959,7 @@ def clone(args): fd.close() sys.stdout.write(_("Created unit file %s\n") % new_unit) - container = Container(args.dest, args.uri) + container = ServiceContainer(name=args.dest, uri=args.uri, create=True) if args.security: container.set_security(args.security) container.gen_machine_id() @@ -907,17 +973,28 @@ class SizeAction(argparse.Action): setattr(namespace, self.dest, int(values)) class CheckUnit(argparse.Action): - def __call__(self, parser, namespace, values, option_string=None): - src = "/etc/systemd/system/" + values - if not os.path.exists(src): - src = "/lib/systemd/system/" + values - if not os.path.exists(src): - raise OSError(_("Requested unit %s does not exist") % values) + def __call__(self, parser, namespace, value, option_string=None): + def check_unit(unit): + src = "/etc/systemd/system/" + unit + if os.path.exists(src): + return src + src = "/lib/systemd/system/" + unit + if os.path.exists(src): + return src + return None + src = check_unit(value) + if not src: + src = check_unit(value + ".service") + if src: + value = value + ".service" + else: + raise OSError("Requested unit %s does not exist" % value) + unitfiles = getattr(namespace, self.dest) if unitfiles: - unitfiles.append((values, src)) + unitfiles.append((value, src)) else: - unitfiles = [ (values, src) ] + unitfiles = [ (value, src) ] setattr(namespace, self.dest, unitfiles) class SetNet(argparse.Action): @@ -1011,7 +1088,7 @@ def gen_reload_args(subparser): def gen_clone_args(subparser): parser = subparser.add_parser("clone", - help=_("Clone an existing sandbox container")) + help=_("Clone an existing sandbox container")) parser.set_defaults(func=clone) parser.add_argument("-s", "--security", dest="security", default=default_security_opts(), -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:28PM -0400, Dan Walsh wrote:
This way we can share common methods between the ServiceContainer and the InteractiveContainer --- bin/virt-sandbox-service | 823 ++++++++++++++++++++++++++--------------------- 1 file changed, 450 insertions(+), 373 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 9f4941b..f4d0eff 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -58,56 +58,387 @@ def read_config(name): return LibvirtSandbox.Config.load_from_path(get_config_path(name))
class Container: + DEFAULT_PATH = "/var/lib/libvirt/filesystems" + DEFAULT_IMAGE = "/var/lib/libvirt/images/%s.raw" + SELINUX_FILE_TYPE = "svirt_lxc_file_t" + + def __init__(self, name=None, uri = "lxc:///", path = DEFAULT_PATH, config=None, create=False): + self.uri = uri + self.use_image = False + self.size = 10 * MB + self.path = path + self.config = None + if self.config: + self.name = self.config.get_name() + else: + self.name = name + self.dest = "%s/%s" % (self.path, self.name) + self.file_type = self.SELINUX_FILE_TYPE + self.conn = None + self.image = None + self.uid = 0 + + def get_file_type(self): + return self.file_type + + def set_file_type(self, file_type): + self.file_type = file_type + + def set_uid(self, uid): + self.uid = uid + + def get_uid(self): + return self.uid + + def get_config_path(self, name = None): + if not name: + name = self.name + return get_config_path(name) + + def get_filesystem_path(self, name = None): + if not name: + name = self.get_name() + return self.DEFAULT_PATH + "/" + name + + def get_image_path(self, name = None): + if not name: + name = self.get_name() + return self.DEFAULT_IMAGE % name + + def set_image(self, size): + self.use_image = True + self.size = size * MB + + def set_path(self, path): + self.path = path + self.dest = "%s/%s" % (self.path, self.name) + + def get_name(self): + return self.name + + def set_name(self, name): + if self.config: + raise ValueError([_("Cannot modify Name")]) + self.name = name + self.dest = "%s/%s" % (self.path, self.name) + + def set_security(self, val): + return self.config.set_security_opts(val) + + def add_network(self, val): + return self.config.add_network_opts(val) + + def get_security_dynamic(self): + return self.config.get_security_dynamic() +
+ def get_security_type(self): + try: + if self.config: + con = self.config.get_security_label().split(':') + return con[2] + except: + pass + return "svirt_lxc_net_t" + + def get_security_level(self): + try: + if self.config: + con = self.config.get_security_label().split(':') + return ":".join(con[3:]) + except: + pass + return "s0" + + def get_security_label(self): + return self.config.get_security_label() + + def set_security_label(self): + if selinux is None: + return + + if self.image or self.get_security_dynamic(): + return + + selabel = self.get_security_label() + if selabel is None: + raise ValueError([_("Missing security label configuration")]) + parts = selabel.split(":") + selinux.chcon(self.dest, "system_u:object_r:%s:%s" % ( + self.get_file_type(), ":".join(parts[3:])), True) + + def set_security_type(self, security_type): + label = "system_u:system_r:%s:%s" % (security_type, self.get_security_level()) + try: + selinux.security_check_context(label) + self.config.set_security_label(label) + except OSError, e: + raise OSError(_("Invalid Security Type %s: %s ") % (security_type, e)) + + def set_security_level(self, security_level): + label = "system_u:system_r:%s:%s" % (self.get_security_type(), security_level) + try: + selinux.security_check_context(label) + self.config.set_security_label(label) + except OSError, e: + raise OSError(_("Invalid Security Level %s: %s ") % (security_level, e))
This patch seems to have had a bad merge. You're adding in new methods here, which don't exist in the code being removed later. These methods were things I deleted when removing SELinux-isms from this code.
@@ -491,10 +701,13 @@ PrivateNetwork=false for f in self.BIND_SYSTEM_FILES: self._makefile(f)
- shutil.copy(self.FUNCTIONS, "%s%s" % (self.dest, self.FUNCTIONS)) + destpath = self.dest + self.SYSVINIT_PATH + for i in range(7): + os.mkdir(destpath+("/rc%s.d" % i)) + os.mkdir(destpath+"/init.d")
This seems to be adding new functionality, not related to plain refactoring
+ shutil.copy(self.SYSVINIT_PATH + "/init.d/functions" , destpath + "/init.d")
self.gen_machine_id() - self.gen_hostname()
for k in self.LOCAL_LINK_FILES:
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Differentiating on which kind of container to create based off of the --command == InteractiveContainer --unitfile == ServiceContainer Resorted create args to be shown aphabetically except for the --command and --unitfile which I want to come at the end. --- bin/virt-sandbox-service | 139 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 99 insertions(+), 40 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index f4d0eff..b559cf5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -413,6 +413,45 @@ class Container: mount = LibvirtSandbox.ConfigMountRam.new(dest, size); self.config.add_mount(mount) +class InteractiveContainer(Container): + def __init__(self, name=None, uri = "lxc:///", path = Container.DEFAULT_PATH, config=None, create=False): + Container.__init__(self, name, uri, path, config, create) + + if create: + self.config = LibvirtSandbox.ConfigInteractive.new(name) + + def _gen_filesystems(self): + Container._gen_filesystems(self) + self.add_bind_mount(self.dest, self.path) + + def _create(self): + # + # Create an InteractiveContainer + # + Container.create(self) + self._gen_filesystems() + + if self.image: + self._create_image() + self._umount() + sys.stdout.write(_("Created sandbox container image %s\n") % self.image) + else: + sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) + self.save_config() + + def create(self): + try: + self._create() + except Exception, e: + try: + self._delete() + except Exception, e2: + pass + raise e + + def set_command(self, command): + self.config.set_command(command) + class ServiceContainer(Container): IGNORE_DIRS = [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ] DEFAULT_DIRS = [ "/etc", "/var" ] @@ -836,19 +875,26 @@ MB = int(1000000) def delete(args): config = read_config(args.name) - container = ServiceContainer(uri=args.uri, config = config) + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + container = InteractiveContainer(uri=args.uri, config = config) + else: + container = ServiceContainer(uri=args.uri, config = config) container.delete() def create(args): - container = ServiceContainer(name = args.name, uri=args.uri, create = True) - container.set_copy(args.copy) + if args.command: + container = InteractiveContainer(name = args.name, uri=args.uri, create = True) + container.set_command(args.command.split()) + else: + container = ServiceContainer(name = args.name, uri=args.uri, create = True) + container.set_copy(args.copy) + container.set_unit_file_list(args.unitfiles) for net in args.network: container.add_network(net) if args.security: container.set_security(args.security) - container.set_unit_file_list(args.unitfiles) container.set_path(args.path) - + container.set_file_type(args.file_type) if args.imagesize: container.set_image(args.imagesize) @@ -879,17 +925,21 @@ def sandbox_list(args): def sandbox_reload(args): config = read_config(args.name) + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + raise ValueError(_("Interactive Containers do not support reload")) container = ServiceContainer(uri = args.uri, config = config) container.reload(args.unitfiles) def start(args): os.execl("/usr/libexec/virt-sandbox-service-util", "virt-sandbox-service-util","-s", args.name) -# container = Container(args.name, args.uri) +# config = read_config(args.name) +# container = Container(uri = args.uri, config=config) # container.start() def stop(args): os.execl("/usr/libexec/virt-sandbox-service-util", "virt-sandbox-service-util","-S", args.name) -# container = Container(args.name, args.uri) +# config = read_config(args.name) +# container = Container(uri = args.uri, config=config) # container.stop() def connect(args): @@ -898,7 +948,8 @@ Connected to %s. Type 'Ctrl + ]' to detach from the console. """ % ( args.name ) os.execl("/usr/libexec/virt-sandbox-service-util", "virt-sandbox-service-util","-a", args.name) -# container = Container(args.name, args.uri) +# config = read_config(args.name) +# container = Container(uri = args.uri, config=config) # container.connect() # @@ -945,25 +996,27 @@ def clone(args): shutil.copytree(old_path, new_path, symlinks=True) sys.stdout.write(_("Created sandbox container dir %s\n") % new_path) - fd = open(container.get_unit_path()) - recs = fd.read() - fd.close() - - fd = open(container.get_unit_path(args.dest), "wx") - fd.write(recs.replace(args.source, args.dest)) - fd.close() + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + container = InteractiveContainer(name=args.dest, uri=args.uri, create=True) + else: + fd = open(container.get_unit_path()) + recs = fd.read() + fd.close() + fd = open(container.get_unit_path(args.dest), "wx") + fd.write(recs.replace(args.source, args.dest)) + fd.close() + new_unit = container.get_config_path(args.dest) + fd = open(new_unit,"wx") + fd.write(newrec) + fd.close() + sys.stdout.write(_("Created unit file %s\n") % new_unit) - new_unit = container.get_config_path(args.dest) - fd = open(new_unit,"wx") - fd.write(newrec) - fd.close() - sys.stdout.write(_("Created unit file %s\n") % new_unit) + container = ServiceContainer(name=args.dest, uri=args.uri, create=True) + container.gen_machine_id() + container.gen_hostname() - container = ServiceContainer(name=args.dest, uri=args.uri, create=True) if args.security: container.set_security(args.security) - container.gen_machine_id() - container.gen_hostname() container.set_security_label() container.save_config() @@ -1022,28 +1075,34 @@ def gen_create_args(subparser): parser = subparser.add_parser("create", help=_("create a sandbox container")) - parser.add_argument("-u", "--unitfile", required=True, - action=CheckUnit, - dest="unitfiles", - help=_("Systemd Unit file to run within the sandbox container")) - parser.add_argument("-p", "--path", dest="path", default=None, - help=_("select path to store sandbox content. Default: %s") % c.DEFAULT_PATH) + parser.add_argument("-C", "--copy", default=False, + action="store_true", + help=_("copy content from the hosts /etc and /var directories that will be mounted within the sandbox")) + parser.add_argument("-f", "--filetype", dest="file_type", + default=c.get_file_type(), + help=_("SELinux file type to assign to content within the sandbox. Default: %s") % c.get_file_type()) + parser.add_argument("-i", "--imagesize", dest="imagesize", default = None, + action=SizeAction, + help=_("create image of this many megabytes.")) + parser.add_argument("-n", "--network", dest="network", + action=SetNet, default=[], + help=_("Specify the network configuration")) + parser.add_argument("-p", "--path", dest="path", default=c.DEFAULT_PATH, + help=_("select path to store sandbox content. Default: %s") % c.DEFAULT_PATH) parser.add_argument("-s", "--security", dest="security", default=default_security_opts(), help=_("Specify the security model configuration for the sandbox: Defaults to dynamic")) - parser.add_argument("-N", "--network", dest="network", - action=SetNet, - help=_("Specify the network configuration")) - image = parser.add_argument_group("Create On Disk Image File") - - image.add_argument("-i", "--imagesize", dest="imagesize", default = None, - action=SizeAction, - help=_("create image of this many megabytes.")) - parser.add_argument("-C", "--copy", default=False, - action="store_true", - help=_("copy content from /etc and /var directories that will be mounted within the sandbox")) + ctype = parser.add_argument_group(_("Type of sandbox container to create")) + group = ctype.add_mutually_exclusive_group(required=True) + group.add_argument("-c", "--command", + dest="command", default=None, + help=_("Command to run within the sandbox container")) + group.add_argument("-u", "--unitfile", + action=CheckUnit, + dest="unitfiles", + help=_("Systemd Unit file to run within the sandbox container")) requires_name(parser) parser.set_defaults(func=create) -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:29PM -0400, Dan Walsh wrote:
Differentiating on which kind of container to create based off of the
--command == InteractiveContainer --unitfile == ServiceContainer
Resorted create args to be shown aphabetically except for the --command and --unitfile which I want to come at the end. --- bin/virt-sandbox-service | 139 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 99 insertions(+), 40 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index f4d0eff..b559cf5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -413,6 +413,45 @@ class Container: mount = LibvirtSandbox.ConfigMountRam.new(dest, size); self.config.add_mount(mount)
+class InteractiveContainer(Container): + def __init__(self, name=None, uri = "lxc:///", path = Container.DEFAULT_PATH, config=None, create=False): + Container.__init__(self, name, uri, path, config, create) + + if create: + self.config = LibvirtSandbox.ConfigInteractive.new(name) + + def _gen_filesystems(self): + Container._gen_filesystems(self) + self.add_bind_mount(self.dest, self.path) + + def _create(self): + # + # Create an InteractiveContainer + # + Container.create(self) + self._gen_filesystems() + + if self.image: + self._create_image() + self._umount() + sys.stdout.write(_("Created sandbox container image %s\n") % self.image) + else: + sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) + self.save_config() + + def create(self): + try: + self._create() + except Exception, e: + try: + self._delete() + except Exception, e2: + pass + raise e + + def set_command(self, command): + self.config.set_command(command) + class ServiceContainer(Container): IGNORE_DIRS = [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ] DEFAULT_DIRS = [ "/etc", "/var" ] @@ -836,19 +875,26 @@ MB = int(1000000)
def delete(args): config = read_config(args.name) - container = ServiceContainer(uri=args.uri, config = config) + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + container = InteractiveContainer(uri=args.uri, config = config) + else: + container = ServiceContainer(uri=args.uri, config = config) container.delete()
def create(args): - container = ServiceContainer(name = args.name, uri=args.uri, create = True) - container.set_copy(args.copy) + if args.command: + container = InteractiveContainer(name = args.name, uri=args.uri, create = True) + container.set_command(args.command.split())
This is bad because it does not take account of shell quoting rules so if you have a command /usr/bin/foo "some bar" You'll create ['/usr/bin/foo', 'some', 'bar']
@@ -1022,28 +1075,34 @@ def gen_create_args(subparser): parser = subparser.add_parser("create", help=_("create a sandbox container"))
+ parser.add_argument("-n", "--network", dest="network", + action=SetNet, default=[], + help=_("Specify the network configuration"))
- parser.add_argument("-N", "--network", dest="network", - action=SetNet, - help=_("Specify the network configuration"))
You've changed '-N' into '-n' - please don't - the use of '-N' was intentionale to match 'virt-sandbox' command line arg names.
- image = parser.add_argument_group("Create On Disk Image File") - - image.add_argument("-i", "--imagesize", dest="imagesize", default = None, - action=SizeAction, - help=_("create image of this many megabytes.")) - parser.add_argument("-C", "--copy", default=False, - action="store_true", - help=_("copy content from /etc and /var directories that will be mounted within the sandbox")) + ctype = parser.add_argument_group(_("Type of sandbox container to create")) + group = ctype.add_mutually_exclusive_group(required=True) + group.add_argument("-c", "--command", + dest="command", default=None, + help=_("Command to run within the sandbox container"))
IMHO it is better to make this work like virt-sandbox, where you can pass the command + its args as args on the end of the command line eg instead of virt-sandbox-service create -c "/usr/bin/foo bar wizz" We should allow virt-sandbox-service create -c /usr/bin/foo bar wizz virt-sandbox-service create -c -- /usr/bin/foo bar wizz so we don't have todo parsing of the -c arg - we just get the string list straight from the sys.argv Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/03/2013 08:47 AM, Daniel P. Berrange wrote:
On Tue, Apr 02, 2013 at 06:11:29PM -0400, Dan Walsh wrote:
Differentiating on which kind of container to create based off of the
--command == InteractiveContainer --unitfile == ServiceContainer
Resorted create args to be shown aphabetically except for the --command and --unitfile which I want to come at the end. --- bin/virt-sandbox-service | 139 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 99 insertions(+), 40 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index f4d0eff..b559cf5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -413,6 +413,45 @@ class Container: mount = LibvirtSandbox.ConfigMountRam.new(dest, size); self.config.add_mount(mount)
+class InteractiveContainer(Container): + def __init__(self, name=None, uri = "lxc:///", path = Container.DEFAULT_PATH, config=None, create=False): + Container.__init__(self, name, uri, path, config, create) + + if create: + self.config = LibvirtSandbox.ConfigInteractive.new(name) + + def _gen_filesystems(self): + Container._gen_filesystems(self) + self.add_bind_mount(self.dest, self.path) + + def _create(self): + # + # Create an InteractiveContainer + # + Container.create(self) + self._gen_filesystems() + + if self.image: + self._create_image() + self._umount() + sys.stdout.write(_("Created sandbox container image %s\n") % self.image) + else: + sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) + self.save_config() + + def create(self): + try: + self._create() + except Exception, e: + try: + self._delete() + except Exception, e2: + pass + raise e + + def set_command(self, command): + self.config.set_command(command) + class ServiceContainer(Container): IGNORE_DIRS = [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ] DEFAULT_DIRS = [ "/etc", "/var" ] @@ -836,19 +875,26 @@ MB = int(1000000)
def delete(args): config = read_config(args.name) - container = ServiceContainer(uri=args.uri, config = config) + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + container = InteractiveContainer(uri=args.uri, config = config) + else: + container = ServiceContainer(uri=args.uri, config = config) container.delete()
def create(args): - container = ServiceContainer(name = args.name, uri=args.uri, create = True) - container.set_copy(args.copy) + if args.command: + container = InteractiveContainer(name = args.name, uri=args.uri, create = True) + container.set_command(args.command.split())
This is bad because it does not take account of shell quoting rules so if you have a command
/usr/bin/foo "some bar"
You'll create
['/usr/bin/foo', 'some', 'bar']
@@ -1022,28 +1075,34 @@ def gen_create_args(subparser): parser = subparser.add_parser("create", help=_("create a sandbox container"))
+ parser.add_argument("-n", "--network", dest="network", + action=SetNet, default=[], + help=_("Specify the network configuration"))
- parser.add_argument("-N", "--network", dest="network", - action=SetNet, - help=_("Specify the network configuration"))
You've changed '-N' into '-n' - please don't - the use of '-N' was intentionale to match 'virt-sandbox' command line arg names.
- image = parser.add_argument_group("Create On Disk Image File") - - image.add_argument("-i", "--imagesize", dest="imagesize", default = None, - action=SizeAction, - help=_("create image of this many megabytes.")) - parser.add_argument("-C", "--copy", default=False, - action="store_true", - help=_("copy content from /etc and /var directories that will be mounted within the sandbox")) + ctype = parser.add_argument_group(_("Type of sandbox container to create")) + group = ctype.add_mutually_exclusive_group(required=True) + group.add_argument("-c", "--command", + dest="command", default=None, + help=_("Command to run within the sandbox container"))
IMHO it is better to make this work like virt-sandbox, where you can pass the command + its args as args on the end of the command line eg instead of
virt-sandbox-service create -c "/usr/bin/foo bar wizz"
We should allow
virt-sandbox-service create -c /usr/bin/foo bar wizz
virt-sandbox-service create -c -- /usr/bin/foo bar wizz
so we don't have todo parsing of the -c arg - we just get the string list straight from the sys.argv
Daniel
Shouldn't the syntax be virt-sandbox-service create foo1 -- /usr/bin/foo bar wizz And virt-sandbox-service create -u httpd.service httpd1 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlFcPfkACgkQrlYvE4MpobNaVACgzqqsaPfKML0auNrN47966RG7 75gAoKkDE4p7ZhaUiqlQATb/A1m+Ey/H =pBjz -----END PGP SIGNATURE-----

On Wed, Apr 03, 2013 at 10:34:33AM -0400, Daniel J Walsh wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/03/2013 08:47 AM, Daniel P. Berrange wrote:
On Tue, Apr 02, 2013 at 06:11:29PM -0400, Dan Walsh wrote:
Differentiating on which kind of container to create based off of the
--command == InteractiveContainer --unitfile == ServiceContainer
Resorted create args to be shown aphabetically except for the --command and --unitfile which I want to come at the end. --- bin/virt-sandbox-service | 139 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 99 insertions(+), 40 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index f4d0eff..b559cf5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -413,6 +413,45 @@ class Container: mount = LibvirtSandbox.ConfigMountRam.new(dest, size); self.config.add_mount(mount)
+class InteractiveContainer(Container): + def __init__(self, name=None, uri = "lxc:///", path = Container.DEFAULT_PATH, config=None, create=False): + Container.__init__(self, name, uri, path, config, create) + + if create: + self.config = LibvirtSandbox.ConfigInteractive.new(name) + + def _gen_filesystems(self): + Container._gen_filesystems(self) + self.add_bind_mount(self.dest, self.path) + + def _create(self): + # + # Create an InteractiveContainer + # + Container.create(self) + self._gen_filesystems() + + if self.image: + self._create_image() + self._umount() + sys.stdout.write(_("Created sandbox container image %s\n") % self.image) + else: + sys.stdout.write(_("Created sandbox container dir %s\n") % self.dest) + self.save_config() + + def create(self): + try: + self._create() + except Exception, e: + try: + self._delete() + except Exception, e2: + pass + raise e + + def set_command(self, command): + self.config.set_command(command) + class ServiceContainer(Container): IGNORE_DIRS = [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ] DEFAULT_DIRS = [ "/etc", "/var" ] @@ -836,19 +875,26 @@ MB = int(1000000)
def delete(args): config = read_config(args.name) - container = ServiceContainer(uri=args.uri, config = config) + if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): + container = InteractiveContainer(uri=args.uri, config = config) + else: + container = ServiceContainer(uri=args.uri, config = config) container.delete()
def create(args): - container = ServiceContainer(name = args.name, uri=args.uri, create = True) - container.set_copy(args.copy) + if args.command: + container = InteractiveContainer(name = args.name, uri=args.uri, create = True) + container.set_command(args.command.split())
This is bad because it does not take account of shell quoting rules so if you have a command
/usr/bin/foo "some bar"
You'll create
['/usr/bin/foo', 'some', 'bar']
@@ -1022,28 +1075,34 @@ def gen_create_args(subparser): parser = subparser.add_parser("create", help=_("create a sandbox container"))
+ parser.add_argument("-n", "--network", dest="network", + action=SetNet, default=[], + help=_("Specify the network configuration"))
- parser.add_argument("-N", "--network", dest="network", - action=SetNet, - help=_("Specify the network configuration"))
You've changed '-N' into '-n' - please don't - the use of '-N' was intentionale to match 'virt-sandbox' command line arg names.
- image = parser.add_argument_group("Create On Disk Image File") - - image.add_argument("-i", "--imagesize", dest="imagesize", default = None, - action=SizeAction, - help=_("create image of this many megabytes.")) - parser.add_argument("-C", "--copy", default=False, - action="store_true", - help=_("copy content from /etc and /var directories that will be mounted within the sandbox")) + ctype = parser.add_argument_group(_("Type of sandbox container to create")) + group = ctype.add_mutually_exclusive_group(required=True) + group.add_argument("-c", "--command", + dest="command", default=None, + help=_("Command to run within the sandbox container"))
IMHO it is better to make this work like virt-sandbox, where you can pass the command + its args as args on the end of the command line eg instead of
virt-sandbox-service create -c "/usr/bin/foo bar wizz"
We should allow
virt-sandbox-service create -c /usr/bin/foo bar wizz
virt-sandbox-service create -c -- /usr/bin/foo bar wizz
so we don't have todo parsing of the -c arg - we just get the string list straight from the sys.argv
Daniel
Shouldn't the syntax be
virt-sandbox-service create foo1 -- /usr/bin/foo bar wizz
And
virt-sandbox-service create -u httpd.service httpd1
Opps, yes, I missed out the sandbox name in my examples. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

So I am execing a virsh list command to show all of the running containers. --- bin/virt-sandbox-service | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index b559cf5..ceb05b3 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -907,22 +907,27 @@ def usage(parser, msg): sys.exit(1) def sandbox_list(args): + if args.running: + import libvirt + conn = libvirt.open("lxc:///") + running = conn.listAllDomains(0) + conn.close() + running.sort() + for d in running: + print d.name() + return + import glob g = glob.glob(CONFIG_PATH + "*.sandbox") g.sort() for gc in g: try: c = LibvirtSandbox.Config.load_from_path(gc) - if args.running: - if c.running(): - print c.get_name() - else: - print c.get_name() + print c.get_name() except OSError, e: print "Invalid container %s: %s", (gc, e) - def sandbox_reload(args): config = read_config(args.name) if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive): -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:30PM -0400, Dan Walsh wrote:
So I am execing a virsh list command to show all of the running containers. --- bin/virt-sandbox-service | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index b559cf5..ceb05b3 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -907,22 +907,27 @@ def usage(parser, msg): sys.exit(1)
def sandbox_list(args): + if args.running: + import libvirt + conn = libvirt.open("lxc:///") + running = conn.listAllDomains(0) + conn.close() + running.sort() + for d in running: + print d.name() + return
NACK to this - we should fix any performance problems in libvirt-glib Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- bin/virt-sandbox-service | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index ceb05b3..1cce6a5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -909,7 +909,7 @@ def usage(parser, msg): def sandbox_list(args): if args.running: import libvirt - conn = libvirt.open("lxc:///") + conn = libvirt.open(args.uri) running = conn.listAllDomains(0) conn.close() running.sort() @@ -971,7 +971,7 @@ def fullpath(cmd): return cmd def execute(args): - myexec = [ "virsh", "-c", "lxc:///", "lxc-enter-namespace" ] + myexec = [ "virsh", "-c", args.uri, "lxc-enter-namespace" ] # myexec = [ "virt-sandbox-service-util", "execute" ] if args.noseclabel: myexec.append("--noseclabel") -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:31PM -0400, Dan Walsh wrote:
--- bin/virt-sandbox-service | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index ceb05b3..1cce6a5 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -909,7 +909,7 @@ def usage(parser, msg): def sandbox_list(args): if args.running: import libvirt - conn = libvirt.open("lxc:///") + conn = libvirt.open(args.uri) running = conn.listAllDomains(0) conn.close() running.sort() @@ -971,7 +971,7 @@ def fullpath(cmd): return cmd
def execute(args): - myexec = [ "virsh", "-c", "lxc:///", "lxc-enter-namespace" ] + myexec = [ "virsh", "-c", args.uri, "lxc-enter-namespace" ] # myexec = [ "virt-sandbox-service-util", "execute" ] if args.noseclabel: myexec.append("--noseclabel")
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- bin/virt-sandbox-service | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 1cce6a5..fe659e2 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -971,6 +971,9 @@ def fullpath(cmd): return cmd def execute(args): + if args.uri != "lxc:///": + raise ValueError(_("Can only execute commands inside of linux containers.")) + myexec = [ "virsh", "-c", args.uri, "lxc-enter-namespace" ] # myexec = [ "virt-sandbox-service-util", "execute" ] if args.noseclabel: @@ -1120,7 +1123,7 @@ def gen_connect_args(subparser): def gen_execute_args(subparser): parser = subparser.add_parser("execute", - help=_("Execute a command within a sandbox container")) + help=_("Execute a command within a sandbox container. Only available for lxc:///")) parser.add_argument("-N", "--noseclabel", dest="noseclabel", default=False, action="store_true", help=_("do not modify the label of the executable process. By default all commands execute with the label of the sandbox")) -- 1.8.2

On Tue, Apr 02, 2013 at 06:11:32PM -0400, Dan Walsh wrote: Typo in commit message & it is also too long.
--- bin/virt-sandbox-service | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 1cce6a5..fe659e2 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -971,6 +971,9 @@ def fullpath(cmd): return cmd
def execute(args): + if args.uri != "lxc:///": + raise ValueError(_("Can only execute commands inside of linux containers.")) + myexec = [ "virsh", "-c", args.uri, "lxc-enter-namespace" ] # myexec = [ "virt-sandbox-service-util", "execute" ] if args.noseclabel: @@ -1120,7 +1123,7 @@ def gen_connect_args(subparser):
def gen_execute_args(subparser): parser = subparser.add_parser("execute", - help=_("Execute a command within a sandbox container")) + help=_("Execute a command within a sandbox container. Only available for lxc:///"))
ACK if commit message is fixed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 04/02/2013 04:11 PM, Dan Walsh wrote:
Most of them effect virt-sandbox-service, with the biggest change beeing the addition of InteractiveContainers.
The subject line on the cover letter needs to be relevant to the rest of the series; using "(no subject)" is generally wrong. Also, since your patch series is for libvirt-sandbox, I'd suggest doing: git config format.subjectprefix 'sandbox PATCH' so that 'git format-patch' (or 'git send-email'), coupled with the automatic list subject munging, will give you a subject-line of: [libvirt][sandbox PATCH 1/16] foobar and make it obvious which project the patch is for. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Dan Walsh
-
Daniel J Walsh
-
Daniel P. Berrange
-
Eric Blake