On Tue, Jul 26, 2022 at 4:55 PM Janosch Frank <fran...@linux.ibm.com> wrote:
> On 7/26/22 13:25, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <fran...@linux.ibm.com>
> wrote:
> >>
> >> As sections don't have a type like the notes do we need another way to
> >> determine their contents. The string table allows us to assign each
> >> section an identification string which architectures can then use to
> >> tag their sections with.
> >>
> >> There will be no string table if the architecture doesn't add custom
> >> sections which are introduced in a following patch.
> >>
> >> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>
> >> ---
> >> dump/dump.c | 81 ++++++++++++++++++++++++++++++++++++++++++-
> >> include/sysemu/dump.h | 1 +
> >> 2 files changed, 81 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index 3cf846d0a0..298a1e923f 100644
> >> --- a/dump/dump.c
> >> +++ b/dump/dump.c
> >> @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s)
> >> close(s->fd);
> >> g_free(s->guest_note);
> >> g_free(s->elf_header);
> >> + g_array_unref(s->string_table_buf);
> >> s->guest_note = NULL;
> >> if (s->resume) {
> >> if (s->detached) {
> >> @@ -357,21 +358,72 @@ static size_t
> prepare_elf_section_hdr_zero(DumpState *s)
> >> return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> >> }
> >>
> >> +static void write_elf_section_hdr_string(DumpState *s, void *buff)
> >> +{
> >
> > Mildly annoyed that we use "write_" prefix for actually writing to the
> > fd, and sometimes to pre-fill (or "prepare_" structures). Would you
> > mind cleaning that up?
> >
>
> Yes, absolutely
>
> >> + Elf32_Shdr shdr32;
> >> + Elf64_Shdr shdr64;
> >> + int shdr_size;
> >> + void *shdr = buff;
> >
> > Why assign here?
>
> Great question
>
:)
>
> >
> >> +
> >> + if (dump_is_64bit(s)) {
> >> + shdr_size = sizeof(Elf64_Shdr);
> >> + memset(&shdr64, 0, shdr_size);
> >> + shdr64.sh_type = SHT_STRTAB;
> >> + shdr64.sh_offset = s->section_offset +
> s->elf_section_data_size;
> >> + shdr64.sh_name = s->string_table_buf->len;
> >> + g_array_append_vals(s->string_table_buf, ".strtab",
> sizeof(".strtab"));
> >> + shdr64.sh_size = s->string_table_buf->len;
> >> + shdr = &shdr64;
> >> + } else {
> >> + shdr_size = sizeof(Elf32_Shdr);
> >> + memset(&shdr32, 0, shdr_size);
> >> + shdr32.sh_type = SHT_STRTAB;
> >> + shdr32.sh_offset = s->section_offset +
> s->elf_section_data_size;
> >> + shdr32.sh_name = s->string_table_buf->len;
> >> + g_array_append_vals(s->string_table_buf, ".strtab",
> sizeof(".strtab"));
> >> + shdr32.sh_size = s->string_table_buf->len;
> >> + shdr = &shdr32;
> >> + }
> >> +
> >> + memcpy(buff, shdr, shdr_size);
> >> +}
> >> +
> >> static void prepare_elf_section_hdrs(DumpState *s)
> >> {
> >> size_t len, sizeof_shdr;
> >> + Elf64_Ehdr *hdr64 = s->elf_header;
> >> + Elf32_Ehdr *hdr32 = s->elf_header;
> >> + void *buff_hdr;
> >>
> >> /*
> >> * Section ordering:
> >> * - HDR zero (if needed)
> >> + * - String table hdr
> >> */
> >> sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) :
> sizeof(Elf32_Shdr);
> >> len = sizeof_shdr * s->shdr_num;
> >> s->elf_section_hdrs = g_malloc0(len);
> >> + buff_hdr = s->elf_section_hdrs;
> >>
> >> /* Write special section first */
> >> if (s->phdr_num == PN_XNUM) {
> >> prepare_elf_section_hdr_zero(s);
> >> + buff_hdr += sizeof_shdr;
> >> + }
> >> +
> >> + if (s->shdr_num < 2) {
> >> + return;
> >> + }
> >> +
> >> + /*
> >> + * String table needs to be last section since strings are added
> >> + * via arch_sections_write_hdr().
> >> + */
> >
> > This comment isn't exactly relevant yet, since that code isn't there,
> but ok.
>
> What about something like this, it's a bit more precise and I'll add the
> arch_sections_write_hdr() reference in the next patch?
>
> /*
> * String table needs to be the last section since it will be populated
> when adding other elf structures.
> */
>
>
ok
> [..]
> >> s->length = length;
> >> + /* First index is 0, it's the special null name */
> >> + s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> >> + /*
> >> + * Allocate the null name, due to the clearing option set to true
> >> + * it will be 0.
> >> + */
> >> + g_array_set_size(s->string_table_buf, 1);
> >
> > I wonder if GByteArray wouldn't be more appropriate, even if it
> > doesn't have the clearing option. If it's just for one byte, ...
>
> I don't really care but I need a decision on it to change it :)
>
I haven't tried, but I think it would be a better fit.
--
Marc-André Lureau
