[Devel] [PATCH 1/2] vzctl: synchronize CRIU with vzctl

Kir Kolyshkin kir at parallels.com
Fri May 31 11:16:29 PDT 2013


On 05/31/2013 11:15 AM, Kir Kolyshkin wrote:
> On 05/31/2013 02:56 AM, Andrey Vagin wrote:
>> vzctl provides two descriptors signalfd and waitfd, it's used for apply
>> host-side configuration after creating environment.
>> A signal is send to signalfd after creating environment and an answer is
>> recieved from waitfd.
>
> Looks awesome overall. See some notes below.
>
>> Signed-off-by: Andrey Vagin <avagin at openvz.org>
>> ---
>>   scripts/Makefile.am    |  3 ++-
>>   scripts/vps-rst-env.in | 44 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   scripts/vps-rst.in     |  4 ++++
>>   src/lib/hooks_ct.c     | 33 +++++++++++++++++++++------------
>>   4 files changed, 71 insertions(+), 13 deletions(-)
>>   create mode 100755 scripts/vps-rst-env.in
>>
>> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
>> index 05381f8..e77f19d 100644
>> --- a/scripts/Makefile.am
>> +++ b/scripts/Makefile.am
>> @@ -29,7 +29,8 @@ script_SCRIPTS = \
>>       vzevent-reboot \
>>       vps-pci    \
>>       vps-cpt \
>> -    vps-rst
>> +    vps-rst \
>> +    vps-rst-env
>>     EXTRA_DIST = \
>>       $(script_SCRIPTS:%=%.in)
>> diff --git a/scripts/vps-rst-env.in b/scripts/vps-rst-env.in
>> new file mode 100755
>> index 0000000..a0ec898
>> --- /dev/null
>> +++ b/scripts/vps-rst-env.in
>> @@ -0,0 +1,44 @@
>> +#!/bin/sh
>> +#  Copyright (C) 2013, Parallels, Inc. All rights reserved.
>> +#
>> +#  This program is free software; you can redistribute it and/or modify
>> +#  it under the terms of the GNU General Public License as published by
>> +#  the Free Software Foundation; either version 2 of the License, or
>> +#  (at your option) any later version.
>> +#
>> +#  This program is distributed in the hope that it will be useful,
>> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +#  GNU General Public License for more details.
>> +#
>> +#  You should have received a copy of the GNU General Public License
>> +#  along with this program; if not, write to the Free Software
>> +#  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 
>> 02111-1307  USA
>> +#
>> +# This script is called by CRIU (http://criu.org) after creating 
>> namespaces.
>> +#
>> +# Parameters are passed in environment variables.
>> +# Required parameters:
>> +#   VZCTL_PID      - pid of vzctl
>
> whitespace at EOL
>
>> +#   STATUSFD      - file descriptor for sending signal to vzctl
>> +#   WAITFD      - file descriptor for receiving signal from vzctl
>> +#   VE_STATE_FILE - file with PID of container init process
>> +#   VE_NETNS_FILE - file, which used by ip netns
>
> There's also CRTOOLS_SCRIPT_ACTION which is set by criu.
>
>> +
>> +[[ "setup-namespaces" == "$CRTOOLS_SCRIPT_ACTION" ]] || exit 0
>
> bashism
>
> it should be something like
>
> [ "x$CRTOOLS_SCRIPT_ACTION" = "xsetup-namespaces" ] || exit 0
>
>> +
>> +exec 1>&2
>
> why?
>
>> +. @SCRIPTDIR@/vps-functions
>> +
>> +vzcheckvar VZCTL_PID
>> +vzcheckvar STATUSFD
>> +vzcheckvar WAITFD
>> +vzcheckvar VE_NETNS_FILE
>> +vzcheckvar VE_STATE_FILE
>> +
>
> maybe you should do 'set -e' here, since you are running some commands
> below that may fail, and you don't check their exit codes.
>
>> +pid=$(cat $VE_STATE_FILE)
>> +ln -sf /proc/$pid/ns/net $VE_NETNS_FILE
>> +
>> +echo -ne '\0\0\0\0' > /proc/$VZCTL_PID/fd/$STATUSFD

printf '\0\0\0\0'

echo -e is non-portable

In general, just use checkbashism before sending scripts like this one

>> +ret=$(cat /proc/$VZCTL_PID/fd/$WAITFD | xxd -p -l 4)
>
> Oh well, xxd comes from vim. I doubt we want to have vim as a dependency.
>
> Maybe hexdump -e '"%d"'?
>> +[ "$ret" = "00000000" ] && exit 0 || exit 1
>
> You don't have to do that, just
>
>     [ "$ret" = "00000000" ]
>     exit
>
> and "exit" is optional if it's last line of the file.
>
>
>> diff --git a/scripts/vps-rst.in b/scripts/vps-rst.in
>> index 91080b3..7cfe658 100755
>> --- a/scripts/vps-rst.in
>> +++ b/scripts/vps-rst.in
>> @@ -26,6 +26,8 @@
>>   #   VE_DUMP_DIR   - directory for saving dump files
>>   #   VE_STATE_FILE - file to write CT init PID to
>>   #   VE_VETH_DEVS  - pair of veth names (CT=HW\n)
>> +#   VE_NS_SCRIPT  - script, which will be executed after creating 
>> namespaces
>
> 1 "script to execute after creating namespaces"
>
> 2 Can we just hardcode it to vps-rst-env?
>
>> +#
>>     exec 1>&2
>>   . @SCRIPTDIR@/vps-functions
>> @@ -34,6 +36,7 @@ vzcheckvar VE_ROOT
>>   vzcheckvar VE_STATE_FILE
>>   vzcheckvar VE_DUMP_DIR
>>   vzcheckvar VE_VETH_DEVS
>> +vzcheckvar VE_NS_SCRIPT
>>     veth_args=""
>>   for dev in $VE_VETH_DEVS; do
>> @@ -46,6 +49,7 @@ criu restore    --file-locks        \
>>           --link-remap        \
>>           --root $VE_ROOT        \
>>           --restore-detached    \
>> +        --action-script $VE_NS_SCRIPT \
>>           -D $VE_DUMP_DIR        \
>>           -o restore.log        \
>>           -vvvv            \
>> diff --git a/src/lib/hooks_ct.c b/src/lib/hooks_ct.c
>> index 18650e0..bf20779 100644
>> --- a/src/lib/hooks_ct.c
>> +++ b/src/lib/hooks_ct.c
>> @@ -391,6 +391,7 @@ static int ct_env_create_real(struct arg_start *arg)
>>       int userns_p[2];
>>       int ret, fd;
>>       char pidpath[STR_SIZE];
>> +    char ctpath[STR_SIZE];
>>         stack_size = get_pagesize();
>>       if (stack_size < 0)
>> @@ -478,13 +479,20 @@ static int ct_env_create_real(struct arg_start 
>> *arg)
>>           close(userns_p[1]);
>>       }
>>   +    snprintf(ctpath, STR_SIZE, "%s/%d", NETNS_RUN_DIR, arg->veid);
>> +    snprintf(pidpath, STR_SIZE, "/proc/%d/ns/net", ret);
>> +    if (symlink(pidpath, ctpath)) {
>> +        logger(-1, errno, "Can't symlink into netns file %s", ctpath);
>> +        destroy_container(arg->veid);
>> +        return -VZ_RESOURCE_ERROR;
>> +    }
>> +
>>       return ret;
>>   }
>>     int ct_env_create(struct arg_start *arg)
>>   {
>>       int ret;
>> -    char procpath[STR_SIZE];
>>       char ctpath[STR_SIZE];
>>         /* non-fatal */
>> @@ -519,14 +527,6 @@ int ct_env_create(struct arg_start *arg)
>>       if (ret < 0)
>>           return -ret;
>>   -    snprintf(procpath, STR_SIZE, "/proc/%d/ns/net", ret);
>> -    ret = symlink(procpath, ctpath);
>> -    if (ret) {
>> -        logger(-1, errno, "Can't symlink into netns file %s", ctpath);
>> -        destroy_container(arg->veid);
>> -        return VZ_RESOURCE_ERROR;
>> -    }
>> -
>>       return 0;
>>   }
>>   @@ -924,7 +924,7 @@ static int ct_chkpnt(vps_handler *h, envid_t veid,
>>   static int ct_restore_fn(vps_handler *h, envid_t veid, const 
>> vps_res *res,
>>                 int wait_p, int old_wait_p, int err_p, void *data)
>>   {
>> -    char *argv[2], *env[5];
>> +    char *argv[2], *env[10];
>>       const char *dumpfile = NULL;
>>       const char *statefile = NULL;
>>       cpt_param *param = data;
>> @@ -957,8 +957,17 @@ static int ct_restore_fn(vps_handler *h, envid_t 
>> veid, const vps_res *res,
>>                   "%s=%s\n", veth->dev_name_ve, veth->dev_name);
>>       }
>>       env[3] = strdup(buf);
>> -
>> -    env[4] = NULL;
>> +    snprintf(buf, sizeof(buf), "VE_NS_SCRIPT=%s", SCRIPTDIR 
>> "/vps-rst-env");
>> +    env[4] = strdup(buf);
>> +    snprintf(buf, sizeof(buf), "VZCTL_PID=%d", getpid());
>> +    env[5] = strdup(buf);
>> +    snprintf(buf, sizeof(buf), "STATUSFD=%d", STDIN_FILENO);
>> +    env[6] = strdup(buf);
>> +    snprintf(buf, sizeof(buf), "WAITFD=%d", wait_p);
>> +    env[7] = strdup(buf);
>> +    snprintf(buf, sizeof(buf), "VE_NETNS_FILE=%s/%d", NETNS_RUN_DIR, 
>> veid);
>> +    env[8] = strdup(buf);
>> +    env[9] = NULL;
>>         ret = run_script(argv[0], argv, env, 0);
>>       free_arg(env);
>




More information about the Devel mailing list