[CRIU] [PATCH v3 1/4] IPC: dump shared memory

[Kir Kolyshkin]

On 02/09/2012 12:13 PM, Kinsbursky Stanislav wrote:
> 08.02.2012 22:48, Kir Kolyshkin пишет:
>> On 02/08/2012 09:26 PM, Kinsbursky Stanislav wrote:
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky at parallels.com>
>>>
>>> ---
>>>     crtools.c         |    6 +++
>>>     include/crtools.h |    5 ++-
>>>     include/image.h   |   17 +++++++++
>>>     ipc_ns.c          |  106 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>>>     4 files changed, 123 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/crtools.c b/crtools.c
>>> index 6f6c254..c43eeab 100644
>>> --- a/crtools.c
>>> +++ b/crtools.c
>>> @@ -116,6 +116,12 @@ struct cr_fd_desc_tmpl fdset_template[CR_FD_MAX] = {
>>>     		.fmt	= FMT_FNAME_IPCNS_VAR,
>>>     		.magic	= IPCNS_VAR_MAGIC,
>>>     	},
>>> +
>>> +	/* IPC namespace shared memory */
>>> +	[CR_FD_IPCNS_SHM] = {
>>> +		.fmt	= FMT_FNAME_IPCNS_SHM,
>>> +		.magic	= IPCNS_SHM_MAGIC,
>>> +	},
>>>     };
>>>
>>>     static struct cr_fdset *alloc_cr_fdset(void)
>>> diff --git a/include/crtools.h b/include/crtools.h
>>> index 868ab38..cf7a46b 100644
>>> --- a/include/crtools.h
>>> +++ b/include/crtools.h
>>> @@ -38,6 +38,7 @@ enum {
>>>     	CR_FD_PSTREE,
>>>     	CR_FD_UTSNS,
>>>     	CR_FD_IPCNS_VAR,
>>> +	CR_FD_IPCNS_SHM,
>>>
>>>     	CR_FD_MAX
>>>     };
>>> @@ -80,6 +81,7 @@ extern struct cr_fd_desc_tmpl fdset_template[CR_FD_MAX];
>>>     #define FMT_FNAME_CREDS		"creds-%d.img"
>>>     #define FMT_FNAME_UTSNS		"utsns-%d.img"
>>>     #define FMT_FNAME_IPCNS_VAR	"ipcns-var-%d.img"
>>> +#define FMT_FNAME_IPCNS_SHM	"ipcns-shm-%d.img"
>>>
>>>     extern int get_image_path(char *path, int size, const char *fmt, int pid);
>>>
>>> @@ -111,7 +113,8 @@ struct cr_fdset {
>>>     	CR_FD_DESC_USE(CR_FD_CREDS)		)
>>>     #define CR_FD_DESC_NS				(\
>>>     	CR_FD_DESC_USE(CR_FD_UTSNS)		|\
>>> -	CR_FD_DESC_USE(CR_FD_IPCNS_VAR)		)
>>> +	CR_FD_DESC_USE(CR_FD_IPCNS_VAR)		|\
>>> +	CR_FD_DESC_USE(CR_FD_IPCNS_SHM)		)
>> If you move the ending bracket ) to a separate line, your next patches
>> will be 1 line shorter and easier to read.
> It's not so hard as is. But I'll take it into account.
>
>>>     #define CR_FD_DESC_NONE			(0)
>>>
>>>     int cr_dump_tasks(pid_t pid, struct cr_options *opts);
>>> diff --git a/include/image.h b/include/image.h
>>> index 84712a9..01a3b86 100644
>>> --- a/include/image.h
>>> +++ b/include/image.h
>>> @@ -22,6 +22,7 @@
>>>     #define UTSNS_MAGIC	0x54473203 /* Smolensk */
>>>     #define CREDS_MAGIC	0x54023547 /* Kozelsk */
>>>     #define IPCNS_VAR_MAGIC	0x53115007 /* Samara */
>>> +#define IPCNS_SHM_MAGIC	0x46283044 /* Odessa */
>>>
>>>     #define PIPEFS_MAGIC	0x50495045
>>>
>>> @@ -125,6 +126,22 @@ struct ipc_var_entry {
>>>     	u32	mq_msgsize_max;
>>>     } __packed;
>>>
>>> +struct ipc_seg {
>>> +	u32	key;
>>> +	u32	uid;
>>> +	u32	gid;
>>> +	u32	cuid;
>>> +	u32	cgid;
>>> +	u32	mode;
>>> +	u32	id;
>>> +	u8	pad[4];
>>> +} __packed;
>>> +
>>> +struct ipc_shm_entry {
>>> +	struct ipc_seg seg;
>>> +	u64	size;
>>> +} __packed;
>>> +
>>>     #define VMA_AREA_NONE		(0<<     0)
>>>     #define VMA_AREA_REGULAR	(1<<     0)	/* Dumpable area */
>>>     #define VMA_AREA_STACK		(1<<     1)
>>> diff --git a/ipc_ns.c b/ipc_ns.c
>>> index de51715..877c16d 100644
>>> --- a/ipc_ns.c
>>> +++ b/ipc_ns.c
>>> @@ -13,6 +13,36 @@
>>>     #include "namespaces.h"
>>>     #include "sysctl.h"
>>>
>>> +static void print_ipc_seg(const struct ipc_seg *seg)
>>> +{
>>> +	pr_info("id: %-10d key: 0x%08x ", seg->id, seg->key);
>>> +	pr_info("uid: %-10d gid: %-10d ", seg->uid, seg->gid);
>>> +	pr_info("cuid: %-10d cgid: %-10d ", seg->cuid, seg->cgid);
>>> +	pr_info ("mode: %-10o ", seg->mode);
>> An extra space
> Yep. Thanks.
>
>>> +}
>>> +
>>> +static void fill_ipc_seg(int id, struct ipc_seg *seg, const struct ipc_perm *ipcp)
>>> +{
>>> +#if defined (__GLIBC__)&&    __GLIBC__>= 2
>>> +#define KEY __key
>>> +#else
>>> +#define KEY key
>>> +#endif
>> I would add a comment here saying something like (if I get it right) we
>> compile this file with and without glibc headers, and that struct
>> ipc_perm is different for kernel and glibc; in particular, kernel
>> version has item named 'key' while in glibc it is '__key', thus the define.
> Kir, it's redundant. Really.
>
>>> +	seg->id = id;
>>> +	seg->key = ipcp->KEY;
>>> +	seg->uid = ipcp->uid;
>>> +	seg->gid = ipcp->gid;
>>> +	seg->cuid = ipcp->cuid;
>>> +	seg->cgid = ipcp->cgid;
>>> +	seg->mode = ipcp->mode;
>>> +}
>> Add
>> #undef KEY
> What for?

C'mon, can't you see it yourself? Because it's a good thing to clean up 
after yourself.

You are introducing a new define with a pretty generic name (KEY) inside 
the function, i.e. in the middle of the file. You intend do it just for 
this function, but in fact you do it for the half of the whole source 
file, which is very wrong and confusing, making the scope of this define 
unclear.

So you should either
1 #define it at the beginning of the file, say right after all #include 
directives. Scope is clear (this file).
2 #undef it at the end of the function, so it's clear that this was just 
for one function. Scope is clear (this function).

I mean, the possibility of someone using this KEY lexeme at the second 
half of this source file is low, but it's still possible (you have used 
it, why could not others do the same?).