On Wed, Jun 14, 2017 at 10:42:19AM +0200, Jiri Slaby wrote:
> On 06/01/2017, 07:44 AM, Josh Poimboeuf wrote:
> ...
> > index 3fb0747..1ca5d9a 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> ...
> > +int elf_write_to_file(struct elf *elf, char *outfile)
> > +{
> > + int fd;
> > + struct section *sec;
> > + Elf *elfout;
> > + GElf_Ehdr eh, ehout;
> > + Elf_Scn *scn;
> > + Elf_Data *data;
> > + GElf_Shdr sh;
> > +
> > + fd = creat(outfile, 0777);
>
> 0755 even though it is umasked?
>
> > + if (fd == -1) {
> > + perror("creat");
> > + return -1;
> > + }
> > +
> > + elfout = elf_begin(fd, ELF_C_WRITE, NULL);
> > + if (!elfout) {
> > + perror("elf_begin");
> > + return -1;
> > + }
> > +
> > + if (!gelf_newehdr(elfout, gelf_getclass(elf->elf))) {
> > + perror("gelf_newehdr");
> > + return -1;
> > + }
> > +
> > + if (!gelf_getehdr(elfout, &ehout)) {
>
> This does not make much sense to do. You memset(0) it below.
>
> > + perror("gelf_getehdr");
> > + return -1;
> > + }
> > +
> > + if (!gelf_getehdr(elf->elf, &eh)) {
> > + perror("gelf_getehdr");
> > + return -1;
> > + }
> > +
> > + memset(&ehout, 0, sizeof(ehout));
> > + ehout.e_ident[EI_DATA] = eh.e_ident[EI_DATA];
> > + ehout.e_machine = eh.e_machine;
> > + ehout.e_type = eh.e_type;
> > + ehout.e_version = EV_CURRENT;
> > + ehout.e_shstrndx = find_section_by_name(elf, ".shstrtab")->idx;
> > +
> > + list_for_each_entry(sec, &elf->sections, list) {
> > + if (sec->idx == 0)
> > + continue;
> > +
> > + scn = elf_newscn(elfout);
> > + if (!scn) {
> > + perror("elf_newscn");
> > + return -1;
> > + }
> > +
> > + data = elf_newdata(scn);
> > + if (!data) {
> > + perror("elf_newdata");
> > + return -1;
> > + }
> > +
> > + if (!elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY)) {
> > + perror("elf_flagdata");
> > + return -1;
> > + }
>
> There is not much point setting DIRTY flag here. elf_newdata does so.
>
> > + data->d_type = sec->data->d_type;
> > + data->d_buf = sec->data->d_buf;
> > + data->d_size = sec->data->d_size;
> > +
> > + if(!gelf_getshdr(scn, &sh)) {
> > + perror("gelf_getshdr");
> > + return -1;
> > + }
>
> This does not make much sense to do again. You overwrite the content
> right away:
>
> > + sh = sec->sh;
> > +
> > + if (!gelf_update_shdr(scn, &sh)) {
> > + perror("gelf_update_shdr");
> > + return -1;
> > + }
> > + }
> > +
> > + if (!gelf_update_ehdr(elfout, &ehout)) {
> > + perror("gelf_update_ehdr");
> > + return -1;
> > + }
> > +
> > + if (elf_update(elfout, ELF_C_WRITE) < 0) {
> > + perror("elf_update");
> > + return -1;
> > + }
>
> elf_end() + close() ?
>
> > +
> > + return 0;
> > +}
> > +
> > void elf_close(struct elf *elf)
> > {
> > struct section *sec, *tmpsec;
>
> ...
>
> > --- /dev/null
> > +++ b/tools/objtool/undwarf.c
> > @@ -0,0 +1,308 @@
> ...
> > +int undwarf_dump(const char *_objname)
> > +{
> > + struct elf *elf;
> > + struct section *sec;
> > + struct rela *rela;
> > + struct undwarf *undwarf;
> > + int nr, i;
> > +
> > + objname = _objname;
> > +
> > + elf = elf_open(objname);
> > + if (!elf) {
> > + WARN("error reading elf file %s\n", objname);
> > + return 1;
> > + }
> > +
> > + sec = find_section_by_name(elf, ".undwarf");
> > + if (!sec || !sec->rela)
> > + return 0;
> > +
> > + nr = sec->len / sizeof(*undwarf);
> > + for (i = 0; i < nr; i++) {
> ...
> > + }
>
> elf_close() ?
>
> > +
> > + return 0;
> > +}
I agree with all your comments, will fix them all. Thanks for the
review.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
