Hi Stéphane,
On Wed, Aug 20, 2014 at 05:19:14PM -0500, Stéphane Graber wrote:
> On Wed, Aug 20, 2014 at 02:31:05PM -0500, Tycho Andersen wrote:
> >
> > +# criu
> > +AC_ARG_ENABLE([criu],
> > + [AC_HELP_STRING([--enable-criu], [enable checkpoint/restore support
> > [default=auto]])],
> > + [], [enable_criu=auto])
> > +
> > +if test "x$enable_criu" = "xauto" ; then
> > + AC_CHECK_PROG([CRIU_CHECK], [criu], [yes], [no],
> > "$PATH:/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin")
> > + if test "x$CRIU_CHECK" = "xyes" ; then
> > + enable_criu=yes
> > + fi
> > +fi
> > +
> > +AM_CONDITIONAL([ENABLE_CRIU], [test "x$enable_criu" = "xyes"])
> > +AC_PATH_TOOL([CRIU_PATH], [criu], [no])
> > +if test "x$enable_criu" = "xyes" ; then
> > + if test "x$CRIU_PATH" = "xno" ; then
> > + AC_MSG_ERROR([Could not find criu])
> > + fi
> > + AC_DEFINE_UNQUOTED([CRIU_PATH], "$CRIU_PATH", [Criu path])
> > +fi
> > +
>
> So why are you doing this?
>
> To have this check pass, a packager would have to build-depend on criu
> even though the build process doesn't use criu at any point.
>
>
> Also, I know we may have been setting a bad example in the past and I'd
> love to see this corrected in the future, but in general, it's a really
> bad practice to hardcode path to binaries at build time.
>
> One simple example of why that's bad is that distros which still do care
> about /usr separation, like Ubuntu, regularly move binaries between
> /usr/bin and /bin depending on what needs to be available for early
> boot.
>
> Obviously, this is unlikely to affect criu, but say that for whatever
> reaosn, criu was considered critical for early boot, we'd then move it
> from /usr/sbin to /sbin which would break LXC until someone uploads an
> (otherwise pointless) no change rebuild to have the hardcode path
> updated.
Mostly because hallyn asked me to :). I guess it is to prevent path
attacks, but I can remove it if you guys want.
> > # cgmanager
> > AC_ARG_ENABLE([cgmanager],
> > [AC_HELP_STRING([--enable-cgmanager], [enable cgmanager support
> > [default=auto]])],
> > @@ -652,6 +673,7 @@ AC_CONFIG_FILES([
> > doc/lxc-autostart.sgml
> > doc/lxc-cgroup.sgml
> > doc/lxc-checkconfig.sgml
> > + doc/lxc-checkpoint.sgml
> > doc/lxc-clone.sgml
> > doc/lxc-config.sgml
> > doc/lxc-console.sgml
> > @@ -780,6 +802,7 @@ Environment:
> > - GnuTLS: $enable_gnutls
> > - Bash integration: $enable_bash
> > - Openvswitch: $enable_ovs
> > + - CRIU: $CRIU_PATH
>
> I guess with the above comment, this one can go too?
>
> Or if criu is just turned into an enable/disable kind of parameter
> (defaulting to disable as not all distros have it), then we probably
> would want to show a simple yes/no at this point.
Yep, agreed. Right now it is the path to criu or "no".
> > +static int my_checker(const struct lxc_arguments *args)
> > +{
> > + if (do_restore && stop) {
> > + lxc_error(args, "-s not compatible with -r.");
> > + return -1;
> > +
> > + } else if (daemonize_set) {
> > + lxc_error(args, "-d/-F not compatible with -r.");
>
> Don't you mean do_restore && daemonize_set here?
Ah, actually I mean !do_restore && daemonize_set, but yes, that logic
is wrong.
I will make the rest of the changes and re-post once we decide what to
do about the autoconf bit.
Tycho
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
