Re: [Qemu-devel] [PATCH v5 13/20] hw/acpi/aml-build: Add ToUUID macro

[Igor Mammedov]

On Mon, 04 May 2015 17:30:20 +0800
Shannon Zhao <address@hidden> wrote:

> 
> 
> On 2015/5/4 17:22, Igor Mammedov wrote:
> > On Wed, 29 Apr 2015 21:41:39 +0800
> > Shannon Zhao <address@hidden> wrote:
> > 
> >>
> >>
> >> On 2015/4/28 17:48, Shannon Zhao wrote:
> >>> On 2015/4/28 17:35, Igor Mammedov wrote:
> >>>> On Tue, 28 Apr 2015 16:52:19 +0800
> >>>> Shannon Zhao <address@hidden> wrote:
> >>>>
> >>>>> On 2015/4/28 16:15, Igor Mammedov wrote:
> >>>>>>>> btw:
> >>>>>>>>>> whole thing might be simpler if an intermediate conversion is 
> >>>>>>>>>> avoided,
> >>>>>>>>>> just pack buffer as in spec byte by byte:
> >>>>>>>>>>
> >>>>>>>>>> /* format: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp */
> >>>>>>>>>> assert(strlen(uuid) == ...);
> >>>>>>>>>> build_append_byte(var->buf, HEX2BYTE(uuid[3]); /* dd */
> >>>>>>>>
> >>>>>>>> use build_append_byte(var->buf, HEX2BYTE(uuid + 7); ?
> >>>>>>>>
> >>>>>>>>>> build_append_byte(var->buf, HEX2BYTE(uuid[2]); /* cc */
> >>>>>>>>
> >>>>>>>> use build_append_byte(var->buf, HEX2BYTE(uuid + 5); ?
> >>>>>> if you mean hyphens /-/ then they are not encoded,
> >>>>>> but you surely can add checks for them to make sure that
> >>>>>> UUID format is as expected.
> >>>>>>
> >>>>>
> >>>>> I mean uuid[3] points to b not dd. Maybe use following way:
> >>>>>
> >>>>> static uint8_t Hex2Byte(char *src)
> >>>> or even better:
> >>>> Hex2Byte(char *src, byte_idx)
> >>>>    and do pointer arithmetic inside
> >>>>
> >>>> [...]
> >>>>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
> >>>> build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
> >>>> build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
> >>>> ...
> >>>>
> >>> Yes, it's better to first four bytes. But there are hyphens /-/, for
> >>> offset 04, 05 and etc it doesn't work. We can't use following expression:
> >>>
> >>> build_append_byte(var->buf, Hex2Byte(uuid, 5)); /* ff - at offset 04 */
> >>> build_append_byte(var->buf, Hex2Byte(uuid, 4)); /* ee - at offset 05 */
> >>> ...
> >>>
> >>>
> >>
> >> So about the implementation of this macro, I think I'd like to use 
> >> following approach. This lets Hex2Byte do what it should only do and 
> >> still has a clear look of UUID. What do you think about?
> >>
> >> static uint8_t Hex2Byte(char *src)
> >> {
> >>      int hi = Hex2Digit(*src++);
> >>      int lo = Hex2Digit(*src);
> >>
> >>      if ((hi < 0) || (lo < 0))
> >>          return -1;
> > just make Hex2Digit() assert on wrong input.
> > 
> 
> Ok.
> 
> >>
> >>      return (hi << 4) | lo;
> >> }
> >>
> >> g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
> >>            && (uuid[18] == '-') && (uuid[23] == '-'));
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
> >                                                ^^^^^
> > I'd make it one number, instead of forcing reader to do math
> > every time he/she looks at this code.
> > 
> 
> Ok, will do this. And about other patches in this series could you offer
> your comments?
the rest of AML patches looks good, I'll give them RB on next respin.

> 
> Thanks,
> Shannon
> 
> > 
> >> build_append_byte(var->buf, Hex2Byte(uuid + (2 * 2))); /* cc */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (1 * 2))); /* bb */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (0 * 2))); /* aa */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (5 * 2 + 1))); /* ff */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (4 * 2 + 1))); /* ee */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (7 * 2 + 2))); /* hh */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (6 * 2 + 2))); /* gg */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (8 * 2 + 3))); /* ii */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (9 * 2 + 3))); /* jj */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (10 * 2 + 4))); /* kk */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (11 * 2 + 4))); /* ll */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (12 * 2 + 4))); /* mm */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (13 * 2 + 4))); /* nn */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (14 * 2 + 4))); /* oo */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (15 * 2 + 4))); /* pp */
> > 
>