Re: [Qemu-devel] [qemu-s390x] [PATCH v7 04/12] s390-ccw: update libc

[Collin L. Walling]

On 02/19/2018 11:19 AM, Collin L. Walling wrote:
> On 02/19/2018 11:00 AM, Thomas Huth wrote:
> > On 19.02.2018 16:40, Collin L. Walling wrote:
> > > On 02/17/2018 02:48 AM, Thomas Huth wrote:
> > > > On 16.02.2018 23:07, Collin L. Walling wrote:
> > > > [...]
> > > > > +/**
> > > > > + * uitoa:
> > > > > + * @num: an integer (base 10) to be converted.
> > > > > + * @str: a pointer to a string to store the conversion.
> > > > > + * @len: the length of the passed string.
> > > > > + *
> > > > > + * Given an integer @num, convert it to a string. The string @str
> > > > > must be
> > > > > + * allocated beforehand. The resulting string will be null
> > > > > terminated and
> > > > > + * returned. This function only handles numbers between 0 and
> > > > > UINT64_MAX
> > > > > + * inclusive.
> > > > > + *
> > > > > + * Returns: the string @str of the converted integer @num
> > > > > + */
> > > > > +char *uitoa(uint64_t num, char *str, size_t len)
> > > > > +{
> > > > > +    size_t num_idx = 0;
> > > > > +    uint64_t tmp = num;
> > > > > +
> > > > > +    IPL_assert(str != NULL, "uitoa: no space allocated to store
> > > > > string");
> > > > > +
> > > > > +    /* Get index to ones place */
> > > > > +    while ((tmp /= 10) != 0) {
> > > > > +        num_idx++;
> > > > > +    }
> > > > > +
> > > > > +    /* Check if we have enough space for num and null */
> > > > > +    IPL_assert(len > num_idx, "uitoa: array too small for 
> > > > > conversion");
> > > > Well, in v5 of this patch you've had "len >= num_idx + 1" where we
> > > > agreed that it was wrong. Now you have "len > num_idx" which is pretty
> > > > much the same. WTF?
> > > > I still think you need "len > num_idx + 1" here to properly take the
> > > > trailing NUL-byte into account properly. Please fix it!
> > > >
> > > >    Thomas
> > > You're correct, and my apologies for not correcting the true problem 
> > > here:
> > > I messed up the value of num_idx.  It is off by one, but 
> > > initializing the
> > > value to 1 instead of 0 should fix this. I must've accounted for 
> > > this in
> > > my test file but forgot to update it in the actual source code.
> > Are you sure that initializing it to 1 is right? Unless you also change
> > the final while loop in this function, this will put the character into
> > the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
> > that only consists of one digit ... the digit will be placed in str[1]
> > which sounds wrong to me...?
> >
> >   Thomas
> There's that, which we can solve by decrementing num_idx at the start 
> of the loop.
> We also have to change the line str[num_idx + 1] = '\0'; to no longer 
> add 1.
> It all boils down to "which way reads better", which I often struggle and
> bounce back-and-forth with (way) too much...
>
> Maybe I should also rename num_idx to just "idx" and let the comments 
> explain
> everything?
How is this for a compromise?

    - start num_idx at 1, provide comment as for why
    - change while loop comment to explain we are "counting the 
_indices_ _of_ _num_"
    - str[num_idx] is assigned \0, and we also decrement num_idx in one 
line
    - in conversion loop, post decrement num_idx as it is used

char *uitoa(int num, char *str, int len)
{
  int num_idx = 1; /* account for NULL */
  int tmp = num;

  assert(str != NULL, "uitoa: no space allocated to store string");

  /* Count indices of num */
  while ((tmp /= 10) != 0)
    num_idx++;

  /* Check if we have enough space for num and null */
  assert(len > num_idx, "uitoa: array too small for conversion");

  str[num_idx--] = '\0';

  /* Convert int to string */
  while (num_idx >= 0) {
    str[num_idx--] = num % 10 + '0';
    num /= 10;
  }

  return str;
}


--
- Collin L Walling