Quoting Oren Laadan ([email protected]):
>
> Thanks for the review ...
>
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan ([email protected]):
> >> 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
_______________________________________________
Containers mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel
