Re: [PATCH v3 1/2] gdbserver: widen the stat fields returned by the vFile packets

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

Abhay Kandpal <abhay@linux.ibm.com> wrote:

>The field widths cannot simply be changed, as that would alter the
wire
>layout and break communication between different versions of GDB and
>gdbserver.  Add a new 'stat64' qSupported feature instead.  When both
>sides report it, the vFile stat packets return struct fio_stat_wide,
in
>which the six too-narrow fields are 8 bytes; otherwise the existing
>struct fio_stat is used, exactly as before.

This looks all generally reasonable to me.  A couple of minor comments.

>+static ULONGEST
>+remote_fileio_to_host_ulong8 (const char *fnum)
>+{
>+  return extract_unsigned_integer ((const gdb_byte *) fnum, 8,
>+				   BFD_ENDIAN_BIG);
>+}

Why do we need this?  The existing remote_fileio_to_host_ulong
should do the right thing already ...

>-  host_to_fileio_stat (&st, &fst);
>-
>-  bytes_sent = hostio_reply_with_data (own_buf,
>-				       (char *) &fst, sizeof (fst),
>-				       new_packet_len);
>+  struct fio_stat_wide wide;
>+  host_to_fileio_stat_wide (&st, &wide);
> 
>-  /* If the response does not fit into a single packet, do not
>attempt
>-     to return a partial response, but simply fail.  */
>-  if (bytes_sent < sizeof (fst))
>-    write_enn (own_buf);
>+  /* Send the wide form if GDB understands it, otherwise narrow the
>+     result first.  */
>+  if (cs.stat64_feature)
>+    {
>+      bytes_sent = hostio_reply_with_data (own_buf, (char *) &wide,
>+					   sizeof (wide),
>new_packet_len);
>+      /* If the response does not fit into a single packet, do not
>attempt
>+	 to return a partial response, but simply fail.  */
>+      if (bytes_sent < sizeof (wide))
>+	write_enn (own_buf);
>+    }
>+  else
>+    {
>+      struct fio_stat narrow;
>+      fileio_stat_wide_to_narrow (&wide, &narrow);

Do we really need the wide_to_narrow conversion routine?
I think it would be preferable to convert from the host struct
directly to the fio_stat_wide in the if path and to fio_stat
in the else path.  Otherwise we end up converting twice.

If we want to emit a warning, maybe that should be done
in host_to_fileio_uint ?

>+      bytes_sent = hostio_reply_with_data (own_buf, (char *) &narrow,
>+					   sizeof (narrow),
>new_packet_len);
>+      /* If the response does not fit into a single packet, do not
>attempt
>+	 to return a partial response, but simply fail.  */
>+      if (bytes_sent < sizeof (narrow))
>+	write_enn (own_buf);
>+    }

This whole block is now duplicated three times, so it might be
worth a helper routine.

>+void
>+host_to_fileio_stat_wide (struct stat *st, struct fio_stat_wide *fst)
>+{
>+  LONGEST blksize;
>+
>+  host_to_bigendian ((LONGEST) st->st_dev, (char *) fst->fst_dev, 8);
>+  host_to_bigendian ((LONGEST) st->st_ino, (char *) fst->fst_ino, 8);

Again, we should be using the existing host_to_fileio_ulong.

Bye,
Ulrich