[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 2/7] Add rpm extraction routines (use librpm and libarchive)



> > +    /* read all files in cpio archive */
> > +    while (rc==ARCHIVE_OK && (rc = archive_read_next_header(cpio,
> &cpio_entry)) == ARCHIVE_OK){
> 
> This should be more like:
> 
> if (rc != ARCHIVE_OK)
>     return rc;
> while ((rc = archive_read_next_header(cpio, &cpio_entry)) ==
> ARCHIVE_OK) {
> 
> As it is now, if archive_read_open() fails, you're calling
> archive_read_finish()
> when the open has failed.

That is intended. libarchive doesn't open anything, because the stream was already opened by librpm. (open handler is NULL in archive_open). So we have to close the stream even if we read nothing.

But I will change it so it is more readable.
 
> > +        const struct stat *fstat;
> > +        int64_t fsize;
> > +        const char* filename;
> > +        int needskip = 1; /* do we need to read the data to get to
> the next header? */
> > +        int offset = 0;
> > +        int towrite = 0;
> > +
> > +        filename = archive_entry_pathname(cpio_entry);
> > +        fstat = archive_entry_stat(cpio_entry);
> > +        fsize = archive_entry_size(cpio_entry);
> > +
> > +        /* Strip leading slashes */
> > +        while (filename[offset] == '/') offset+=1;
> > +        /* Strip leading ./ */
> > +        while (filename[offset] == '.' && filename[offset+1] ==
> '/') offset+=2;
> 
> 1) please don't collapse code onto the same line as a while() or if()
> 2) if "/" or "./" or "." makes it into the cpio ball here, which would
> be
>    unfortunate but is possible, then this code segfaults.

No it doesn't. You forgot that it won'ลง match \0 and skip the second check. It will return empty string in the case / or ./ make it into cpio.


> > +                free((void*)filename);
> 
> (void*) shouldn't be here.

For some reason, gcc complained so this needs to be here. It won't build otherwise. (I tried it on F12, not sure about rawhide)

> > +            if (symlink(buffer, filename+offset)) {
> > +                /* TODO: error handling */
> 
> Obviously this needs to be written before this goes in.

There is really nothing we can do.. If we are only missing symlinks in the DD, the .ko files should still work. logMessage should be here though.

> > +   Author:    msivak
> > +
> > +   Copyright (C) 2009 msivak
> 
> I think this should say "Copyright 2009 ."

Oh no, my boilerplate just leaked :) I'll fix this of course

Martin


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]