[Libguestfs] [libnbd PATCH v3 02/22] internal: Refactor layout of replies in sbuf

[Laszlo Ersek]

On 5/26/23 23:06, Eric Blake wrote:

> I should indeed try harder to follow your useful example of generating
> specific patches with more than the default 3 lines of context, when
> it would make review easier.  Alas, 'git format-patch' doesn't seem to
> have an easy way to pick a different context size on a per-patch
> basis, so this basically implies writing (or finding and reusing
> existing) wrapper tooling to automate that.

If it's of any help, please see my script attached. It lets me put
"context:-W" and "context:-U5" style hints in the Notes sections of the
patches (with git-notes).

>>> diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
>>> index 96182222..6f96945a 100644
>>> --- a/generator/states-reply-structured.c
>>> +++ b/generator/states-reply-structured.c
>>> @@ -49,9 +49,9 @@  REPLY.STRUCTURED_REPLY.START:
>>>     * so read the remaining part.
>>>     */
>>>    h->rbuf = &h->sbuf;
>>> -  h->rbuf = (char *)h->rbuf + sizeof h->sbuf.simple_reply;
>>> -  h->rlen = sizeof h->sbuf.sr.structured_reply;
>>> -  h->rlen -= sizeof h->sbuf.simple_reply;
>>> +  h->rbuf = (char *)h->rbuf + sizeof h->sbuf.reply.hdr.simple;
>>> +  h->rlen = sizeof h->sbuf.reply.hdr.structured;
>>> +  h->rlen -= sizeof h->sbuf.reply.hdr.simple;
>>>    SET_NEXT_STATE (%RECV_REMAINING);
>>>    return 0;
>>>
>>
>> Here I disagree with the mechanical approach.
>>
>> I even take issue with the pre-patch code. Pre-patch, we fill in
>> "h->sbuf.simple_reply" (in "generator/states-reply.c"), i.e., one member
>> of the top-level "sbuf" union, but then continue filling a member of a
>> *different* member (i.e., "sr.structured_reply") of the "sbuf" union
>> here. This looks undefined by the C standard, but even if it's not
>> undefined, it's supremely confusing.
> 
> It happens to work, but I agree with you that we are probably
> stretching aliasing rules about memory effective types that it is
> shaky ground to begin with.  Even adding a STATIC_ASSERT that
> offsetof(struct simple_reply, handle) == offsetof(struct
> structured_reply, handle) would be helpful to show that we depend on
> that.

After I sent my comments last week, I kept pondering this topic. I now
believe my approach to unions in this instance was too rigid. The code
is not trivial to read for sure, but there are code comments that help
with that. If you can introduce that STATIC_ASSERT, IMO that will
suffice, for sticking with this patch.

(

Given that we already depend on (non-standard) packing, and depend on
the accesses to those packed fields not to fault, it's mostly irrelevant
how exactly we calculate the byte offsets.

It's really difficult to use unions effectively, if one doesn't go
beyond what the standard guarantees. :/

)

Thanks,
Laszlo
-------------- next part --------------
#!/bin/bash
set -e -u -C

# Assume all arguments except the last one are git-format-patch options.
options=("${@:1:($#-1)}")

# Assume the last argument is the one operand for git-format-patch: the revision
# set.
revisions=${@:(-1)}

# Fetch the array of revisions (with abbreviated hashes) in increasing order.
rev_list=($(git log --reverse --format=tformat:%h "$revisions"))

# Create temporary directory.
tmpd=$(mktemp -d)
trap 'rm -r -- "$tmpd"' EXIT

# If the Notes section of a patch contains "context:-U3", "context:-U8" etc, or
# "context:-W" lines, return those lines. The "git notes show" output is
# expected on stdin.
filter_context_opts()
{
  sed -n -r -e 's/^context:(-U[0-9]+|-W)$/\1/p'
}

# In the directory given by $1, expand * into an array, then print the element
# with subscript $2 to stdout. The printed element will be qualified with the
# containing directory $1.
get_nth_file()
{
  local dir=$1
  local ofs=$2
  local flist=("$dir"/*)

  printf '%s\n' ${flist["$ofs"]}
}

# Collect all the context options used over the series into an array. The
# default "-U3" context option will be forcibly generated.
all_context_opts=($(
  (
    for rev in "${rev_list[@]}"; do
      git notes show "$rev" 2>/dev/null || true
    done \
    | filter_context_opts

    printf '%s\n' -U3
  ) \
  | sort -u
))

# For each found context option, format the entire series, into a dedicated
# subdirectory, using the caller-specified options and revision set. (Format a
# cover letter as well.) Unfortunately, this is necessary because the "/m" part
# in "[PATCH v3 n/m]" depends on *all* patches being formatted in one go.
for context_opt in "${all_context_opts[@]}"; do
  mkdir -- "$tmpd/context$context_opt"
  git format-patch --notes --cover-letter --numbered --stat=1000 \
      --stat-graph-width=20 --output-directory "$tmpd/context$context_opt" \
      "$context_opt" "${options[@]}" "$revisions" >/dev/null
done

# Iterate over the cover letter and all the revisions. For each, determine the
# context option. (For the cover letter, and for revisions without a
# "context:..." line in the Notes section, assume -U3. If a revision has
# multiple "context:..." lines in the Notes section, pick the first one.) Once
# the context option has been determined, *positionally* look up the formatted
# patch email that corresponds to the cover letter or the revision, in the
# subdirectory that corresponds to the context option.
patchnr=0
for rev in cover_letter "${rev_list[@]}"; do
  if test cover_letter = "$rev"; then
    context_opt=
  else
    context_opt=$(
      git notes show "$rev" 2>/dev/null \
      | filter_context_opts \
      | head -1
    )
  fi
  if test -z "$context_opt"; then
    context_opt=-U3
  fi
  file=$(get_nth_file "$tmpd/context$context_opt" "$patchnr")
  cat -- "$file"
  if test $patchnr -gt 0 && test $patchnr -lt ${#rev_list[@]}; then
    printf '\n'
  fi
  patchnr=$(($patchnr + 1))
done