Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()

[Greg Kurz]

On Fri, 7 Oct 2016 10:36:58 +0100
Peter Maydell <address@hidden> wrote:

> On 7 October 2016 at 08:31, Greg Kurz <address@hidden> wrote:
> > On Fri, 7 Oct 2016 09:10:14 +0200
> > Laurent Vivier <address@hidden> wrote:
> >  
> >> On 06/10/2016 22:45, Peter Maydell wrote:  
> >> > On 6 October 2016 at 11:56, Laurent Vivier <address@hidden> 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