From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: [RFC v14-rc2][PATCH 21/29] Dump anonymous- and file-mapped- shared memory Date: Wed, 1 Apr 2009 18:32:48 -0500 Message-ID: <20090401233248.GA31598@us.ibm.com> References: <1238477349-11029-1-git-send-email-orenl@cs.columbia.edu> <1238477349-11029-22-git-send-email-orenl@cs.columbia.edu> <20090401230657.GB27725@us.ibm.com> <49D3F636.1070303@cs.columbia.edu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <49D3F636.1070303-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Oren Laadan Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Dave Hansen List-Id: containers.vger.kernel.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