On Fri, 7 Oct 2016 10:36:58 +0100 Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 7 October 2016 at 08:31, Greg Kurz <gr...@kaod.org> wrote: > > On Fri, 7 Oct 2016 09:10:14 +0200 > > Laurent Vivier <lviv...@redhat.com> wrote: > > > >> On 06/10/2016 22:45, Peter Maydell wrote: > >> > On 6 October 2016 at 11:56, Laurent Vivier <lviv...@redhat.com> wrote: > >> >> + /* ask endianness of the target */ > >> >> + > >> >> + qtest_sendf(s, "endianness\n"); > >> >> + args = qtest_rsp(s, 1); > >> >> + g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") > >> >> == 0); > >> >> + s->big_endian = strcmp(args[1], "big") == 0; > >> >> + g_strfreev(args); > >> > > >> > This would be better in its own utility function, I think. > >> > >> Yes, I agree, but my wondering was how to name it :P , > >> qtest_big_endian() and target_big_endian() are already in use, and as it > >> is a 6 lines function, used once, I guessed we could inline it here. > >> > > > > This is TARGET_WORDS_BIGENDIAN which is constant for a single QEMU > > run... why moving it to a function ? Unless there are plans to > > have dynamic target endianness in QEMU, I guess it makes more > > sense to open code as you did. > > I thought it would be better in its own function simply > because "query the QEMU process for the value of > TARGET_WORDS_BIGENDIAN" is a simple well defined and > self contained operation, which isn't very tightly > coupled to the init-the-connection stuff that qtest_init() > does. qtest_init() is already a fairly long function and > so I think it makes for more maintainable code to have > a (static, local) qtest_query_target_endianness() function > rather than inlining it. > > thanks > -- PMM It's a matter of taste, but your point makes sense. I'd say let the maintainer decide, but... $ ./scripts/get_maintainer.pl -f tests/libqtest.c get_maintainer.pl: No maintainers found, printing recent contributors. get_maintainer.pl: Do not blindly cc: them on patches! Use common sense. Cheers. -- Greg