lkml.org 
[lkml]   [2012]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Extend core dump note section to contain file names of mapped files
Date
On Wednesday 11 July 2012 17:15, Oleg Nesterov wrote:
> On 07/11, Denys Vlasenko wrote:
> >
> > I propose to save this information in core dump, as a new note
> > in note segment.
>
> Denys, I am in no position to discuss whether we need this change or not,
> format, etc. I'll only try to comment the code.
>
> And please do not use the attachments ;)
>
> > +static void fill_files_note(struct memelfnote *note)
> > +{
> > + struct vm_area_struct *vma;
> > + struct file *file;
> > + unsigned count, word_count, size, remaining;
> > + long *data;
> > + long *start_end_ofs;
> > + char *name;
> > +
> > + count = 0;
> > + for (vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
> > + file = vma->vm_file;
> > + if (!file)
> > + continue;
> > + count++;
> > + if (count >= MAX_FILE_NOTE_SIZE / 64) /* paranoia check */
> > + goto err;
>
> Why this check? If count is huge, then...
>
> > + size = count * 64;
> > + word_count = 2 + 3 * count;
> > + alloc:
> > + if (size >= MAX_FILE_NOTE_SIZE) /* paranoia check */
> > + goto err;
>
> we should detect this case before the first alloc?

Unless count * 64 overflows an int :)
As I said in the comment: paranoia.

Perhaps that's TOO MUCH of paranoia. Removing.

> > + size = (size + PAGE_SIZE - 1) & (-PAGE_SIZE);
>
> Well, I'd suggest PAGE_MASK instead of -PAGE_SIZE. Better yet,
>
> size = round_up(size, PAGE_SIZE);
>
> > + if (remaining == 0) {
> > + try_new_size:
> > + vfree(data);
> > + size = size * 5 / 4;
> > + goto alloc;
> > + }
> > + filename = d_path(&file->f_path, name, remaining);
> > + if (IS_ERR(filename)) {
> > + if (PTR_ERR(filename) == -ENAMETOOLONG)
> > + goto try_new_size;
>
> This looks like unnecessary complication to me, or I missed something.
> d_path(..., buflen) should handle the "buflen == 0" case correctly, so
> afacics you can remove the "if (remaining == 0)" block and move this
> free-and-goto-alloc code under the -ENAMETOOLONG check.
>
> > + while ((remaining--, *name++ = *filename++) != '\0')
> > + continue;
>
> Well, perhaps this is just me... but this looks a bit too complex
> to me ;) I won't insist, but
>
> do
> remaining--;
> while ((*name++ = *filename++));
>
> looks more understandable, imho.

Okay.


> Or even
>
> /* d_path() fills the end of the buffer */
> remaining = name - filename;
> strcpy(name, filename);

This does not advance "name" pointer... oh...
it's actually clever! But it'll fail if we took

/* continue; -- WRONG, we must have COUNT elements */
filename = "";
}

branch just above... I will use an open-coded loop for now.

Sending v2 in a moment.

--
vda


\
 
 \ /
  Last update: 2012-07-12 22:21    [W:0.106 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site