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

[Oren Laadan <orenl@cs.columbia.edu>]

MinChan Kim wrote:
> On Tue, Sep 9, 2008 at 4:42 PM, Oren Laadan <orenl@cs.columbia.edu> wrote:

[...]

>> +#define CR_DEFAULT_FDTABLE  256                /* an initial guess */
>> +
>> +/**
>> + * cr_scan_fds - scan file table and construct array of open fds
>> + * @files: files_struct pointer
>> + * @fdtable: (output) array of open fds
>> + * @return: the number of open fds found
>> + *
>> + * Allocates the file descriptors array (*fdtable), caller should free
>> + */
>> +int cr_scan_fds(struct files_struct *files, int **fdtable)
>> +{
>> +       struct fdtable *fdt;
>> +       int *fdlist;
>> +       int i, n, max;
>> +
>> +       n = 0;
>> +       max = CR_DEFAULT_FDTABLE;
> 
> max is read-only variable so that you don't need to declare local variable.
> You can use macro.

It's actually used below - track size in case of krealloc()

> 
>> +       fdlist = kmalloc(max * sizeof(*fdlist), GFP_KERNEL);
>> +       if (!fdlist)
>> +               return -ENOMEM;
>> +
>> +       spin_lock(&files->file_lock);
>> +       fdt = files_fdtable(files);
>> +       for (i = 0; i < fdt->max_fds; i++) {
>> +               if (!fcheck_files(files, i))
>> +                       continue;
>> +               if (n == max) {
>> +                       /* fcheck_files() is safe with drop/re-acquire
>> +                        * of the lock, as it tests:  fd < max_fds */
>> +                       spin_unlock(&files->file_lock);
>> +                       max *= 2;
>> +                       if (max < 0) {  /* overflow ? */
>> +                               n = -EMFILE;
>> +                               goto out;
>> +                       }
>> +                       fdlist = krealloc(fdlist, max, GFP_KERNEL);
>> +                       if (!fdlist) {
>> +                               n = -ENOMEM;
>> +                               goto out;
>> +                       }
>> +                       spin_lock(&files->file_lock);
>> +               }
>> +               fdlist[n++] = i;
>> +       }
>> +       spin_unlock(&files->file_lock);
>> +
>> +       *fdtable = fdlist;
>> + out:
>> +       return n;
>> +}

[...]

>> +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;
>> +               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);
> 
> I think your intention is not 'n' but 'fdtable[n]' in argument.

Oops ... yes. Thanks.

[...]

Oren.