Re: [RFC v14-rc2][PATCH 21/29] Dump anonymous- and file-mapped- shared memory

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> Thanks for the review ...
> 
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> >> We now handle anonymous and file-mapped shared memory. Support for IPC
> >> shared memory requires support for IPC first. We extend cr_write_vma()
> >> to detect shared memory VMAs and handle it separately than private
> >> memory.
> 
> [...]
> 
> >> +	switch (vma_type) {
> >> +	case CR_VMA_SHM_FILE:
> >> +		/* no need for contents that are stored in the file system */
> >> +		ret = vfs_fsync(vma->vm_file, vma->vm_file->f_path.dentry, 0);
> >> +		break;
> >> +	case CR_VMA_SHM_ANON:
> >> +		/* save the contents of this resgion */
> >> +		inode = vma->vm_file->f_dentry->d_inode;
> >> +		ret = cr_write_shmem_contents(ctx, inode);
> >> +		break;
> >> +	case CR_VMA_SHM_ANON_SKIP:
> >> +	case CR_VMA_SHM_FILE_SKIP:
> >> +		/* already saved before .. skip now */
> >> +		break;
> >> +	default:
> >> +		BUG();
> > 
> > Well, no - since the user can feed in whatever crap they want,
> > this isn't a *bug*, right?
> 
> ... this is during checkpoint

Oh, heh. never mind then.

> , no user input; it makes sure we don't
> add a new type of VMA that we don't handle. On restart we complain
> with -EINVAL.
> 
> [...]
> 
> > 
> >>  struct cr_hdr_vma {
> >>  	__u32 vma_type;
> >> -	__u32 vma_objref;	/* for vma->vm_file */
> >> +	__s32 vma_objref;	/* objref of backing file */
> >> +	__s32 shm_objref;	/* objref of shared segment */
> > 
> > You're going to upset Alexey again with the signeds, aren't you?
> 
> Yes, I put a comment about signed values somewhere. I cleaned up most of
> the unsigned instances following Alexey's comment, but I think in some
> cases it makes sense.
> 
> In particular, assume I take a pid, or an objref, which is an 'int' in
> the code, and save it with __u32. During restart I need to test for a
> valid value before blindly converting back to (signed) int, like:
> 	
> 	ret = -EINVAL;
> 	if (hh->pid > INT_MAX)
> 		goto out;
> 
> in that case, I can just as well leave it signed and test
> 
> 	ret = -EINVAL;
> 	if (hh->pid < 0)
> 		goto out;
> 
> which is much more readable, and less error-prone if sometime later
> we change objref type from (signed) int to (signed) long and forget
> to change INT_MAX to LONG_MAX everywhere ...
> 
> Oren.

Makes sense.  Just wanted to make sure it didn't accidentally slip
in.

thanks,
-serge