Re: [RFC v4][PATCH 8/9] File descriprtors (dump)

["Vegard Nossum" <vegard.nossum@gmail.com>]

Hi,

Below are some concerns, I would be grateful for explanations (or
pointers if I missed them before).

On Tue, Sep 9, 2008 at 9:42 AM, Oren Laadan <orenl@cs.columbia.edu> wrote:
> +/* cr_write_fd_data - dump the state of a given file pointer */
> +static int cr_write_fd_data(struct cr_ctx *ctx, struct file *file, int parent)
> +{
> +       struct cr_hdr h;
> +       struct cr_hdr_fd_data *hh = cr_hbuf_get(ctx, sizeof(*hh));
> +       struct dentry *dent = file->f_dentry;
> +       struct inode *inode = dent->d_inode;
> +       enum fd_type fd_type;
> +       int ret;
> +
> +       h.type = CR_HDR_FD_DATA;
> +       h.len = sizeof(*hh);
> +       h.parent = parent;
> +
> +       hh->f_flags = file->f_flags;
> +       hh->f_mode = file->f_mode;
> +       hh->f_pos = file->f_pos;
> +       hh->f_uid = file->f_uid;
> +       hh->f_gid = file->f_gid;
> +       hh->f_version = file->f_version;
> +       /* FIX: need also file->f_owner */
> +
> +       switch (inode->i_mode & S_IFMT) {
> +       case S_IFREG:
> +               fd_type = CR_FD_FILE;
> +               break;
> +       case S_IFDIR:
> +               fd_type = CR_FD_DIR;
> +               break;
> +       case S_IFLNK:
> +               fd_type = CR_FD_LINK;
> +               break;
> +       default:
> +               return -EBADF;
> +       }

Should cr_hbuf_put() come before the return here?

As far as I've understood, "leaking" the buffer size/data isn't
critical (1. because it's just some extra space, and/or 2. the buffer
is discarded on error anyway). The code looks really unbalanced
without it, though. I guess it should at least be documented?

> +
> +       /* FIX: check if the file/dir/link is unlinked */
> +       hh->fd_type = fd_type;
> +
> +       ret = cr_write_obj(ctx, &h, hh);
> +       cr_hbuf_put(ctx, sizeof(*hh));
> +       if (ret < 0)
> +               return ret;
> +
> +       return cr_write_fname(ctx, &file->f_path, ctx->vfsroot);
> +}
> +
> +/**
> + * cr_write_fd_ent - dump the state of a given file descriptor
> + * @ctx: checkpoint context
> + * @files: files_struct pointer
> + * @fd: file descriptor
> + *
> + * Save the state of the file descriptor; look up the actual file pointer
> + * in the hash table, and if found save the matching objref, otherwise call
> + * cr_write_fd_data to dump the file pointer too.
> + */
> +static int
> +cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd)
> +{
> +       struct cr_hdr h;
> +       struct cr_hdr_fd_ent *hh = cr_hbuf_get(ctx, sizeof(*hh));
> +       struct file *file = NULL;
> +       struct fdtable *fdt;
> +       int coe, objref, new, ret;
> +
> +       rcu_read_lock();
> +       fdt = files_fdtable(files);
> +       file = fcheck_files(files, fd);
> +       if (file) {
> +               coe = FD_ISSET(fd, fdt->close_on_exec);
> +               get_file(file);
> +       }
> +       rcu_read_unlock();
> +
> +       /* sanity check (although this shouldn't happen) */
> +       if (!file)
> +               return -EBADF;
> +
> +       new = cr_obj_add_ptr(ctx, (void *) file, &objref, CR_OBJ_FILE, 0);
> +       cr_debug("fd %d objref %d file %p c-o-e %d)\n", fd, objref, file, coe);
> +
> +       if (new < 0)
> +               return new;

fput() and/or cr_hbuf_put()?

> +
> +       h.type = CR_HDR_FD_ENT;
> +       h.len = sizeof(*hh);
> +       h.parent = 0;
> +
> +       hh->objref = objref;
> +       hh->fd = fd;
> +       hh->close_on_exec = coe;
> +
> +       ret = cr_write_obj(ctx, &h, hh);
> +       cr_hbuf_put(ctx, sizeof(*hh));
> +       if (ret < 0)
> +               return ret;
> +
> +       /* new==1 if-and-only-if file was newly added to hash */
> +       if (new)
> +               ret = cr_write_fd_data(ctx, file, objref);
> +
> +       fput(file);
> +       return ret;
> +}
> +
> +int cr_write_files(struct cr_ctx *ctx, struct task_struct *t)
> +{
> +       struct cr_hdr h;
> +       struct cr_hdr_files *hh = cr_hbuf_get(ctx, sizeof(*hh));
> +       struct files_struct *files;
> +       int *fdtable;
> +       int nfds, n, ret;
> +
> +       h.type = CR_HDR_FILES;
> +       h.len = sizeof(*hh);
> +       h.parent = task_pid_vnr(t);
> +
> +       files = get_files_struct(t);
> +
> +       hh->objref = 0; /* will be meaningful with multiple processes */
> +
> +       nfds = cr_scan_fds(files, &fdtable);
> +       if (nfds < 0) {
> +               ret = nfds;

cr_hbuf_put()?

> +               goto out;
> +       }
> +
> +       hh->nfds = nfds;
> +
> +       ret = cr_write_obj(ctx, &h, hh);
> +       cr_hbuf_put(ctx, sizeof(*hh));
> +       if (ret < 0)
> +               goto clean;
> +
> +       cr_debug("nfds %d\n", nfds);
> +       for (n = 0; n < nfds; n++) {
> +               ret = cr_write_fd_ent(ctx, files, n);
> +               if (ret < 0)
> +                       break;
> +       }
> +
> + clean:
> +       kfree(fdtable);
> + out:
> +       put_files_struct(files);
> +
> +       return ret;
> +}


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036