Skip to contentSkip to conversation

: Rename/reorg usercr code

RFC PATCH In review Sukadev Bhattiprolu 2 participants 12 messages Apr 14, 2010 – Jun 14, 2010 4 messages not in the archive Kbuild

lore
Share & tools
lore

Sukadev Bhattiprolu

author
#0 lore raw

cover : Rename/reorg usercr code

Change the prefix of the USERCR apis to 'cr_' and reorg the code to avoid duplication and reduce code size.

[PATCH 1/6] Change API prefix to cr_
[PATCH 2/6] Remove flags parameter to cr_checkpoint() [PATCH 3/6] Minor reorg of restart.c
[PATCH 4/6] Move checkpoint() into restart.c [PATCH 5/6] Rename restart.c to cr_checkpoint.c [PATCH 6/6] Rename common.h to cr_log.h

With this change, USERCR for now "exports" just the following two files:

cr_checkpoint.o
cr_checkpoint.h

But this patchset does cause some churn, let me know if you think any of it is unnecessary/noise. I have tested for now along with the patch that removes most exits() from cr_restart(). If the reorg makes sense, will run all tests once more and resubmit.

TODO:
cr_checkpoint.c has several forward declarations - most of these can be removed by reordering the code in the file. This would also reduce maintenance overhead when the prototype of the functions change. But changing it would cause more noise in the history.

author
#2 lore raw

RFC PATCH 1/6 Change API prefix to cr_

In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 13 Apr 2010 12:26:03 -0700
Subject: [RFC][PATCH 1/6][usercr] Change API prefix to cr_

Change the "app_" prefix to "cr_" in function names and data structures.

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 Makefile          |    2 +-
 app-checkpoint.h  |   45 ---------------------------------------------
 checkpoint-main.c |    8 ++++----
 checkpoint.c      |    6 +++---
 cr_checkpoint.h   |   45 +++++++++++++++++++++++++++++++++++++++++++++
 restart-main.c    |    8 ++++----
 restart.c         |    8 ++++----
 7 files changed, 61 insertions(+), 61 deletions(-)
 delete mode 100644 app-checkpoint.h
 create mode 100644 cr_checkpoint.h
Makefile +1 -1
index 6c9ff93..afe144f 100644
@@ -52,7 +52,7 @@ $(LIB_ECLONE):
 # restart needs to be thread-safe
 restart: CFLAGS += -D__REENTRANT -pthread
 
-$(CR_OBJS): common.h app-checkpoint.h
+$(CR_OBJS): common.h cr_checkpoint.h
 
 restart: restart.o restart-main.o
app-checkpoint.h deleted +0 -45

45 lines not on this page · raw message

checkpoint-main.c +4 -4
index bac65cc..48c54f6 100644
@@ -9,7 +9,7 @@
 
 #include <linux/checkpoint.h>
 
-#include "app-checkpoint.h"
+#include "cr_checkpoint.h"
 #include "common.h"
 
 static int global_uerrfd = -1;

@@ -51,7 +51,7 @@ static int str2num(char *str)
 	return num;
 }
 
-static void parse_args(struct app_checkpoint_args *args, int argc, char *argv[])
+static void parse_args(struct cr_checkpoint_args *args, int argc, char *argv[])
 {
 	static struct option opts[] = {
 		{ "help",	no_argument,		NULL, 'h' },

@@ -146,7 +146,7 @@ static void parse_args(struct app_checkpoint_args *args, int argc, char *argv[])
 int main(int argc, char *argv[])
 {
 	int ret;
-	struct app_checkpoint_args args;
+	struct cr_checkpoint_args args;
 	unsigned long flags = 0;
 	pid_t pid;
 

@@ -168,6 +168,6 @@ int main(int argc, char *argv[])
 	if (!args.container)
 		flags |= CHECKPOINT_SUBTREE;
 
-	ret = app_checkpoint(pid, flags, &args);
+	ret = cr_checkpoint(pid, flags, &args);
 	return (ret > 0) ? 0 : 1;
 }
checkpoint.c +3 -3
index e0290c9..5472bfe 100644
@@ -21,7 +21,7 @@
 
 #include <linux/checkpoint.h>
 
-#include "app-checkpoint.h"
+#include "cr_checkpoint.h"
 #include "common.h"
 
 static int global_uerrfd = -1;

@@ -31,8 +31,8 @@ inline static int checkpoint(pid_t pid, int fd, unsigned long flags, int logfd)
 	return syscall(__NR_checkpoint, pid, fd, flags, logfd);
 }
 
-int app_checkpoint(int pid, unsigned long flags,
-				struct app_checkpoint_args *args)
+int cr_checkpoint(int pid, unsigned long flags,
+				struct cr_checkpoint_args *args)
 {
 	int ret;
cr_checkpoint.h new file +45 -0

45 lines not on this page · raw message

restart-main.c +4 -4
index 656d49f..1f1a749 100644
@@ -8,7 +8,7 @@
 #include <limits.h>
 #include <getopt.h>
 
-#include "app-checkpoint.h"
+#include "cr_checkpoint.h"
 #include "common.h"
 
 static int global_ulogfd;

@@ -105,7 +105,7 @@ static int str2sig(char *str)
 	return -1;
 }
 
-static void parse_args(struct app_restart_args *args, int argc, char *argv[])
+static void parse_args(struct cr_restart_args *args, int argc, char *argv[])
 {
 	static struct option opts[] = {
 		{ "help",	no_argument,		NULL, 'h' },

@@ -299,7 +299,7 @@ static void parse_args(struct app_restart_args *args, int argc, char *argv[])
 int main(int argc, char *argv[])
 {
 	int ret;
-	struct app_restart_args args;
+	struct cr_restart_args args;
 
 	/*
 	 * Initialize the log/error fds early so even parse_args() errors

@@ -312,7 +312,7 @@ int main(int argc, char *argv[])
 
 	parse_args(&args, argc, argv);
 
-	ret = app_restart(&args);
+	ret = cr_restart(&args);
 	if (ret > 0)
 		ret = 0;
restart.c +4 -4
index 3224335..c98d377 100644
@@ -40,7 +40,7 @@
 #include "eclone.h"
 #include "genstack.h"
 #include "compat.h"
-#include "app-checkpoint.h"
+#include "cr_checkpoint.h"
 #include "common.h"
 
 /*

@@ -162,7 +162,7 @@ struct ckpt_ctx {
 	char tree[BUFSIZE];
 	char vpids[BUFSIZE];
 	char buf[BUFSIZE];
-	struct app_restart_args *args;
+	struct cr_restart_args *args;
 
 	char *freezer;
 };

@@ -400,7 +400,7 @@ static int freezer_register(struct ckpt_ctx *ctx, pid_t pid)
  * Validate the specified arguments and initialize globals based on the
  * arguments. Return 0 on success.
  */
-int process_args(struct app_restart_args *args)
+int process_args(struct cr_restart_args *args)
 {
 	global_debug = args->debug;
 	global_verbose = args->verbose;

@@ -463,7 +463,7 @@ int process_args(struct app_restart_args *args)
 	return 0;
 }
 
-int app_restart(struct app_restart_args *args)
+int cr_restart(struct cr_restart_args *args)
 {
 	struct ckpt_ctx ctx;
 	int ret;
Signature · 1 line
1.6.6.1
author
#3 lore raw

RFC PATCH 2/6 Remove flags parameter to cr_checkpoint()

In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 13 Apr 2010 15:28:55 -0700
Subject: [RFC][PATCH 2/6][usercr] Remove flags parameter to cr_checkpoint()

For consistency with cr_restart(), remove 'flags' parameter to cr_checkpoint(). Given that we pass in the 'struct cr_checkpoint_args' the flags parameter is anyway redundant.

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 checkpoint-main.c |    6 +-----
 checkpoint.c      |    7 +++++--
 cr_checkpoint.h   |    4 +---
 3 files changed, 7 insertions(+), 10 deletions(-)
checkpoint-main.c +1 -5
index 48c54f6..b8f96a7 100644
@@ -147,7 +147,6 @@ int main(int argc, char *argv[])
 {
 	int ret;
 	struct cr_checkpoint_args args;
-	unsigned long flags = 0;
 	pid_t pid;
 
 	global_uerrfd = fileno(stderr);

@@ -165,9 +164,6 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	if (!args.container)
-		flags |= CHECKPOINT_SUBTREE;
-
-	ret = cr_checkpoint(pid, flags, &args);
+	ret = cr_checkpoint(pid, &args);
 	return (ret > 0) ? 0 : 1;
 }
checkpoint.c +5 -2
index 5472bfe..90d4d35 100644
@@ -31,10 +31,10 @@ inline static int checkpoint(pid_t pid, int fd, unsigned long flags, int logfd)
 	return syscall(__NR_checkpoint, pid, fd, flags, logfd);
 }
 
-int cr_checkpoint(int pid, unsigned long flags,
-				struct cr_checkpoint_args *args)
+int cr_checkpoint(int pid, struct cr_checkpoint_args *args)
 {
 	int ret;
+	unsigned long flags;
 
 	global_uerrfd = args->uerrfd;
 

@@ -46,6 +46,9 @@ int cr_checkpoint(int pid, unsigned long flags,
 	if (args->logfd < 0)
 		args->logfd = CHECKPOINT_FD_NONE;
 
+	if (!args->container)
+		flags |= CHECKPOINT_SUBTREE;
+
 	ret = checkpoint(pid, args->outfd, flags, args->logfd);
 
 	if (ret < 0) {
cr_checkpoint.h +1 -3
index 81fa2c2..37f9f2a 100644
@@ -39,7 +39,5 @@ struct cr_restart_args {
  */
 extern int global_send_sigint;
 
-extern int cr_checkpoint(int pid, unsigned long flags,
-				struct cr_checkpoint_args *args);
-
+extern int cr_checkpoint(int pid, struct cr_checkpoint_args *args);
 extern int cr_restart(struct cr_restart_args *args);
Signature · 1 line
1.6.6.1
author
#4 lore raw

RFC PATCH 3/6 Minor reorg of restart.c

In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 13 Apr 2010 12:34:28 -0700
Subject: [RFC][PATCH 3/6][usercr] Minor reorg of restart.c

Just a minor reorg to group global variables, declarations, system-call wrappers and helper functions together. A follow-on patch will combine the checkpoint and restart functionality into one file and this reorg would help keep related code together.

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 restart.c |   43 +++++++++++++++++++++----------------------
 1 files changed, 21 insertions(+), 22 deletions(-)
restart.c +21 -22
index c98d377..11628e0 100644
@@ -68,24 +68,6 @@
  * of the checkpoint image stream.
  */
 
-static struct signal_array signal_array[] = INIT_SIGNAL_ARRAY;
-
-static char *sig2str(int sig)
-{
-	int i = 0;
-
-	do {
-		if (signal_array[i].signum == sig)
-			return signal_array[i].sigstr;
-	} while (signal_array[++i].signum >= 0);
-	return "UNKNOWN SIGNAL";
-}
-
-inline static int restart(pid_t pid, int fd, unsigned long flags, int klogfd)
-{
-	return syscall(__NR_restart, pid, fd, flags, klogfd);
-}
-
 struct hashent {
 	long key;
 	void *data;

@@ -167,6 +149,11 @@ struct ckpt_ctx {
 	char *freezer;
 };
 
+struct pid_swap {
+	pid_t old;
+	pid_t new;
+};
+
 /*
  * TODO: Do we need to direct user-space restart messages to two different
  * 	 fds (like stdout and stderr) or can we just use one ?

@@ -179,6 +166,7 @@ static pid_t global_child_pid;
 static int global_child_status;
 static int global_child_collected;
 static int global_sent_sigint;
+static struct signal_array signal_array[] = INIT_SIGNAL_ARRAY;
 
 /*
  * TODO: Implement an API to let callers choose if/how an interrupt be sent

@@ -244,10 +232,10 @@ static inline pid_t _getpid(void)
 	return syscall(__NR_getpid);
 }
 
-struct pid_swap {
-	pid_t old;
-	pid_t new;
-};
+static inline int restart(pid_t pid, int fd, unsigned long flags, int klogfd)
+{
+	return syscall(__NR_restart, pid, fd, flags, klogfd);
+}
 
 static inline int ckpt_cond_warn(struct ckpt_ctx *ctx, long mask)
 {

@@ -276,6 +264,17 @@ static void report_exit_status(int status, char *str, int debug)
 		ckpt_err("%s\n", msg);
 }
 
+static char *sig2str(int sig)
+{
+	int i = 0;
+
+	do {
+		if (signal_array[i].signum == sig)
+			return signal_array[i].sigstr;
+	} while (signal_array[++i].signum >= 0);
+	return "UNKNOWN SIGNAL";
+}
+
 static void sigchld_handler(int sig)
 {
 	int collected = 0;
Signature · 1 line
1.6.6.1
author
#5 lore raw

RFC PATCH 4/6 Move checkpoint() into restart.c

In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 13 Apr 2010 14:35:32 -0700
Subject: [RFC][PATCH 4/6][usercr] Move checkpoint() into restart.c

checkpoint() function is pretty small and the file checkpoint.c mostly has includes/declarations that are common to restart.c. We could just fold both checkpoint() and restart() into a single file.

To keep the diff simple, this patch just does the move and the next patch renames 'restart.c' to 'cr_checkpoint.c'.

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 Makefile     |   10 ++++++--
 checkpoint.c |   62 ----------------------------------------------------------
 restart.c    |   35 ++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 65 deletions(-)
 delete mode 100644 checkpoint.c
Makefile +7 -3
index afe144f..674b112 100644
@@ -13,7 +13,7 @@ CKPT_HEADERS = include/linux/checkpoint.h \
 		include/linux/checkpoint_hdr.h \
 		include/asm/checkpoint_hdr.h
 
-CR_OBJS = checkpoint.o checkpoint-main.o restart.o restart-main.o
+CR_OBJS = restart.o checkpoint-main.o restart-main.o
 
 # detect architecture (for eclone)
 SUBARCH ?= $(patsubst i%86,x86_32,$(shell uname -m))

@@ -32,7 +32,7 @@ PREFIX ?= /usr/local
 BIN_INSTALL_DIR := $(PREFIX)/bin
 LIB_INSTALL_DIR := $(PREFIX)/lib
 
-ECLONE_PROGS = restart nsexec
+ECLONE_PROGS = checkpoint restart nsexec
 PROGS =	checkpoint ckptinfo $(ECLONE_PROGS)
 LIB_ECLONE = libeclone.a
 

@@ -55,8 +55,10 @@ restart: CFLAGS += -D__REENTRANT -pthread
 $(CR_OBJS): common.h cr_checkpoint.h
 
 restart: restart.o restart-main.o
+	$(CC) -o $@ $^
 
-checkpoint: checkpoint.o checkpoint-main.o
+checkpoint: restart.o checkpoint-main.o
+	$(CC) -o $@ $^
 
 # eclone() is architecture specific
 ifneq ($(SUBARCH),)

@@ -77,6 +79,8 @@ ASFLAGS += -m64
 $(LIB_ECLONE): clone_$(SUBARCH)_.o
 endif
 
+restart.o: $(LIB_ECLONE)
+
 # ckptinfo dependencies
 ckptinfo: ckptinfo_types.o
checkpoint.c deleted +0 -62

62 lines not on this page · raw message

restart.c +35 -0
index 11628e0..e01c08e 100644
@@ -232,6 +232,11 @@ static inline pid_t _getpid(void)
 	return syscall(__NR_getpid);
 }
 
+inline static int checkpoint(pid_t pid, int fd, unsigned long flags, int logfd)
+{
+	return syscall(__NR_checkpoint, pid, fd, flags, logfd);
+}
+
 static inline int restart(pid_t pid, int fd, unsigned long flags, int klogfd)
 {
 	return syscall(__NR_restart, pid, fd, flags, klogfd);

@@ -2582,3 +2587,33 @@ static void *hash_lookup(struct ckpt_ctx *ctx, long key)
 	}
 	return NULL;
 }
+
+int cr_checkpoint(int pid, struct cr_checkpoint_args *args)
+{
+	int ret;
+	unsigned long flags;
+
+	global_uerrfd = args->uerrfd;
+
+	/* output file descriptor (default: stdout) */
+	if (args->outfd < 0)
+		args->outfd = STDOUT_FILENO;
+
+	/* log file descriptor (default: none) */
+	if (args->logfd < 0)
+		args->logfd = CHECKPOINT_FD_NONE;
+
+	if (!args->container)
+		flags |= CHECKPOINT_SUBTREE;
+
+	ret = checkpoint(pid, args->outfd, flags, args->logfd);
+
+	if (ret < 0) {
+		ckpt_perror("checkpoint");
+		ckpt_err("(you may use 'ckptinfo -e' for more info)\n"); 
+	} else if (args->verbose) {
+		ckpt_err("checkpoint id %d\n", ret);
+	}
+
+	return ret;
+}
Signature · 1 line
1.6.6.1
author
#6 lore raw

RFC PATCH 5/6 Rename restart.c to cr_checkpoint.c

In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 13 Apr 2010 17:01:40 -0700
Subject: [RFC][PATCH 5/6][usercr] Rename restart.c to cr_checkpoint.c

restart.c now contains both the checkpoint and restart functionality. Kernel files that are common to both checkpoint and restart have "checkpoint" in their name, To follow the convention, rename 'restart.c' to 'cr_checkpoint.c'

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 Makefile        |    8 +-
 cr_checkpoint.c | 2619 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 restart.c       | 2619 -------------------------------------------------------
 3 files changed, 2623 insertions(+), 2623 deletions(-)
 create mode 100644 cr_checkpoint.c
 delete mode 100644 restart.c
Makefile +4 -4
index 674b112..205320a 100644
@@ -13,7 +13,7 @@ CKPT_HEADERS = include/linux/checkpoint.h \
 		include/linux/checkpoint_hdr.h \
 		include/asm/checkpoint_hdr.h
 
-CR_OBJS = restart.o checkpoint-main.o restart-main.o
+CR_OBJS = cr_checkpoint.o checkpoint-main.o restart-main.o
 
 # detect architecture (for eclone)
 SUBARCH ?= $(patsubst i%86,x86_32,$(shell uname -m))

@@ -54,10 +54,10 @@ restart: CFLAGS += -D__REENTRANT -pthread
 
 $(CR_OBJS): common.h cr_checkpoint.h
 
-restart: restart.o restart-main.o
+restart: cr_checkpoint.o restart-main.o
 	$(CC) -o $@ $^
 
-checkpoint: restart.o checkpoint-main.o
+checkpoint: cr_checkpoint.o checkpoint-main.o
 	$(CC) -o $@ $^
 
 # eclone() is architecture specific

@@ -79,7 +79,7 @@ ASFLAGS += -m64
 $(LIB_ECLONE): clone_$(SUBARCH)_.o
 endif
 
-restart.o: $(LIB_ECLONE)
+cr_checkpoint.o: $(LIB_ECLONE)
 
 # ckptinfo dependencies
 ckptinfo: ckptinfo_types.o
cr_checkpoint.c new file +2619 -0

2,619 lines not on this page · raw message

restart.c deleted +0 -2619

353 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
1.6.6.1

Part of the diff is not shown here. View the raw message.

author
#7 lore raw

RFC PATCH 6/6 Rename common.h to cr_log.h

In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 13 Apr 2010 17:12:04 -0700
Subject: [RFC][PATCH 6/6][usercr] Rename common.h to cr_log.h

common.h used to be shared between checkpoint.c and restart.c. Since all the code is now in one file (cr_checkpoint.c), the name 'common.h' does not make much sense.

The definitions in the file relate to logging, so rename to cr_log.h.

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 Makefile          |    2 +-
 checkpoint-main.c |    2 +-
 common.h          |  112 -----------------------------------------------------
 cr_checkpoint.c   |    2 +-
 cr_log.h          |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 restart-main.c    |    2 +-
 6 files changed, 114 insertions(+), 116 deletions(-)
 delete mode 100644 common.h
 create mode 100644 cr_log.h
Makefile +1 -1
index 205320a..f2c6b18 100644
@@ -52,7 +52,7 @@ $(LIB_ECLONE):
 # restart needs to be thread-safe
 restart: CFLAGS += -D__REENTRANT -pthread
 
-$(CR_OBJS): common.h cr_checkpoint.h
+$(CR_OBJS): cr_log.h cr_checkpoint.h
 
 restart: cr_checkpoint.o restart-main.o
 	$(CC) -o $@ $^
checkpoint-main.c +1 -1
index b8f96a7..34eb827 100644
@@ -10,7 +10,7 @@
 #include <linux/checkpoint.h>
 
 #include "cr_checkpoint.h"
-#include "common.h"
+#include "cr_log.h"
 
 static int global_uerrfd = -1;
common.h deleted +0 -112

112 lines not on this page · raw message

cr_checkpoint.c +1 -1
index e01c08e..272c54b 100644
@@ -41,7 +41,7 @@
 #include "genstack.h"
 #include "compat.h"
 #include "cr_checkpoint.h"
-#include "common.h"
+#include "cr_log.h"
 
 /*
  * By default, 'restart' creates a new pid namespace in which the
cr_log.h new file +110 -0

110 lines not on this page · raw message

restart-main.c +1 -1
index 1f1a749..c3100ac 100644
@@ -9,7 +9,7 @@
 #include <getopt.h>
 
 #include "cr_checkpoint.h"
-#include "common.h"
+#include "cr_log.h"
 
 static int global_ulogfd;
 static int global_uerrfd;
Signature · 1 line
1.6.6.1
author
#8 lore raw
In reply to a message not in the archive

Sukadev Bhattiprolu [[email protected]] wrote:

| 
| Change the prefix of the USERCR apis to 'cr_' and reorg the code to avoid
| duplication and reduce code size.
| 
| 	[PATCH 1/6] Change API prefix to cr_
| 	[PATCH 2/6] Remove flags parameter to cr_checkpoint()
| 	[PATCH 3/6] Minor reorg of restart.c
| 	[PATCH 4/6] Move checkpoint() into restart.c
| 	[PATCH 5/6] Rename restart.c to cr_checkpoint.c
| 	[PATCH 6/6] Rename common.h to cr_log.h
| 
| With this change, USERCR for now "exports" just the following two files:
| 
| 	cr_checkpoint.o
| 	cr_checkpoint.h
| 
| But this patchset does cause some churn, let me know if you think any of it
| is unnecessary/noise.  I have tested for now along with the patch that removes
| most exits() from cr_restart(). If the reorg makes sense, will run all tests
| once more and resubmit.

All tests in cr-tests pass with these two patchsets (on usercr ckpt-v20-dev and linux ckpt-v21-rc2).

Pls let me know if there are comments about the patchsets.

Thanks,

Sukadev

#10 lore raw
In reply to a message not in the archive

Hi Suka,

Thanks for the patchset. A couple of comments:

* I prefer the header exported to users to be checkpoint.h - this is consistent with kernel headers, and with future name of a c/r library if we opt libcheckpoint.a

* I also prefer to leave checkpoint.c as a separate file, as is. It may gain more functionality in the future. If the goal was to only export a single .o file, then the solution IMHO is to instead export a single library: libcheckpoint.a

Unless you have a strong opinion against the above, I'll go ahead and pull patches 1-3, leave out 4-6, and update the Makefile to create a libcheckpoint.a library.

Oren.

Quoted 31 lines · Sukadev Bhattiprolu

Sukadev Bhattiprolu [[email protected]] wrote:

| 
| Change the prefix of the USERCR apis to 'cr_' and reorg the code to avoid
| duplication and reduce code size.
| 
| 	[PATCH 1/6] Change API prefix to cr_
| 	[PATCH 2/6] Remove flags parameter to cr_checkpoint()
| 	[PATCH 3/6] Minor reorg of restart.c
| 	[PATCH 4/6] Move checkpoint() into restart.c
| 	[PATCH 5/6] Rename restart.c to cr_checkpoint.c
| 	[PATCH 6/6] Rename common.h to cr_log.h
| 
| With this change, USERCR for now "exports" just the following two files:
| 
| 	cr_checkpoint.o
| 	cr_checkpoint.h
| 
| But this patchset does cause some churn, let me know if you think any of it
| is unnecessary/noise.  I have tested for now along with the patch that removes
| most exits() from cr_restart(). If the reorg makes sense, will run all tests
| once more and resubmit.

All tests in cr-tests pass with these two patchsets (on usercr ckpt-v20-dev and linux ckpt-v21-rc2).

Pls let me know if there are comments about the patchsets.

Thanks,

Sukadev

author
#12 lore raw
In reply to a message not in the archive
Quoted 7 lines · Oren Laadan [[email protected]] wrote:

Hi Suka,

Thanks for the patchset. A couple of comments:

* I prefer the header exported to users to be checkpoint.h - this is consistent with kernel headers, and with future name of a c/r library if we opt libcheckpoint.a

That is fine. Will this checkpoint.h include <linux/checkpoint.h> ?

Quoted 5 lines

* I also prefer to leave checkpoint.c as a separate file, as is. It may gain more functionality in the future. If the goal was to only export a single .o file, then the solution IMHO is to instead export a single library: libcheckpoint.a

A single .o was not really a goal. Sure, if we are ready for libcheckpoint.a then thats a better way.

Quoted 4 lines

Unless you have a strong opinion against the above, I'll go ahead and pull patches 1-3, leave out 4-6, and update the Makefile to create a libcheckpoint.a library.

How about patch 6, renaming 'common.h' to 'cr_log.h' (file internal to usercr) ? Let me know if you want me to resend the patches dropping 4 and 5.

Sukadev

#14 lore raw
In reply to a message not in the archive
Quoted 10 lines · Sukadev Bhattiprolu
Quoted 7 lines · Oren Laadan [[email protected]] wrote:

Hi Suka,

Thanks for the patchset. A couple of comments:

* I prefer the header exported to users to be checkpoint.h - this is consistent with kernel headers, and with future name of a c/r library if we opt libcheckpoint.a

That is fine. Will this checkpoint.h include <linux/checkpoint.h> ?

Definitely.

Quoted 9 lines
Quoted 5 lines

* I also prefer to leave checkpoint.c as a separate file, as is. It may gain more functionality in the future. If the goal was to only export a single .o file, then the solution IMHO is to instead export a single library: libcheckpoint.a

A single .o was not really a goal. Sure, if we are ready for libcheckpoint.a then thats a better way.

Quoted 6 lines
Quoted 3 lines

Unless you have a strong opinion against the above, I'll go ahead and pull patches 1-3, leave out 4-6, and update the Makefile to create a libcheckpoint.a library.

How about patch 6, renaming 'common.h' to 'cr_log.h' (file internal to usercr) ? Let me know if you want me to resend the patches dropping 4 and 5.

It's not strictly limited to logging affairs, and it may have more in the future, so I'd rather keep it as is.

No need to resend. I'll just pull and edit.

Thanks!

Oren.

#15 lore raw
In reply to a message not in the archive

Patches 1-3 applied (and will be pushed shortly).

Oren.

Quoted 57 lines · Oren Laadan

Hi Suka,

Thanks for the patchset. A couple of comments:

* I prefer the header exported to users to be checkpoint.h - this is consistent with kernel headers, and with future name of a c/r library if we opt libcheckpoint.a

* I also prefer to leave checkpoint.c as a separate file, as is. It may gain more functionality in the future. If the goal was to only export a single .o file, then the solution IMHO is to instead export a single library: libcheckpoint.a

Unless you have a strong opinion against the above, I'll go ahead and pull patches 1-3, leave out 4-6, and update the Makefile to create a libcheckpoint.a library.

Oren.

Quoted 22 lines · Sukadev Bhattiprolu

Sukadev Bhattiprolu [[email protected]] wrote:

| 
| Change the prefix of the USERCR apis to 'cr_' and reorg the code to avoid
| duplication and reduce code size.
| 
| 	[PATCH 1/6] Change API prefix to cr_
| 	[PATCH 2/6] Remove flags parameter to cr_checkpoint()
| 	[PATCH 3/6] Minor reorg of restart.c
| 	[PATCH 4/6] Move checkpoint() into restart.c
| 	[PATCH 5/6] Rename restart.c to cr_checkpoint.c
| 	[PATCH 6/6] Rename common.h to cr_log.h
| 
| With this change, USERCR for now "exports" just the following two files:
| 
| 	cr_checkpoint.o
| 	cr_checkpoint.h
| 
| But this patchset does cause some churn, let me know if you think any of it
| is unnecessary/noise.  I have tested for now along with the patch that removes
| most exits() from cr_restart(). If the reorg makes sense, will run all tests
| once more and resubmit.
Quoted 9 lines

All tests in cr-tests pass with these two patchsets (on usercr ckpt-v20-dev and linux ckpt-v21-rc2).

Pls let me know if there are comments about the patchsets.

Thanks,

Sukadev

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers