Received-SPF: pass (google.com: domain of linux-wireless+bounces-4377-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <5c5be485dcfceb44fc731e47758d6be3.legale.legale@gmail.com>
 <12f91b008d6f2b4e6241794d2852c1c84fe6c12c.camel@sipsolutions.net>
 <CZLQMMSSUWBF.1NPJJYDRIXGIR@gmail.com> <37398c88130c487be656eed4e378d6b047a3d612.camel@sipsolutions.net>
In-Reply-To: <37398c88130c487be656eed4e378d6b047a3d612.camel@sipsolutions.net>
From: Isaev Ruslan <legale.legale@gmail.com>
Date: Tue, 5 Mar 2024 14:32:07 +0300
Message-ID: <CACDmYycV9qP2TUMMz5ij75A18NEaVpke6t70r7ELNgV=8+sCRw@mail.gmail.com>
Subject: Re: [PATCH v5] Add JSON output options to 'iw' for scan results
To: Johannes Berg <johannes@sipsolutions.net>
Cc: Nicolas Escande <nico.escande@gmail.com>, linux-wireless@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Tue, 5 Mar 2024 at 13:31, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Tue, 2024-03-05 at 11:17 +0100, Nicolas Escande wrote:
>
> > For what it's worth jansson has been very good to me. It has printf() like
> > object creation which usually integrates well with c programs.
>
> Nice :)
>
> > With MIT license https://github.com/akheron/jansson/blob/master/LICENSE
>
> If we link it as a library rather than copying it, that wouldn't even
> matter (well unless it was GPL too, but even then you could still make a
> build without it if you don't want the result to become GPL.)
>
> > For me, having a "good" json representation means, as you pointed out, using the
> > right json underlying type and most of the time I'm afraid that means a complete
> > different code path because of the underlying type/container. It's always a
> > blurry line obviously but duplicating in a complete seperate function that only
> > does the right thing for json output may end up being cleaner codewise.
>
> Yeah, depends, I guess. I'd rather see less duplicating, and in the
> cases like in this patch it seems like it wouldn't even matter so much.
> But agree it depends on the objects.

I think i could make a universal printf() like function with
additional args for json representation.

Something like this:
before:
printf("\t * Manufacturer: %.*s\n", sublen, data + 4);

after:
printf_json("\t * Manufacturer", "%.*s", sublen, data + 4);


void printf_json(const char* key, const char* fmt, ...) {
    char buf[1024];
    va_list args;
    va_start(args, format);

    if (iw_json) {
        vsnprintf(buf, sizeof(buf), format, args);
        print_string(PRINT_JSON, key, "%s", buf);
    } else {
        printf("\t * %s: ", key);
        vprintf(fmt, args);
        printf("\n");
    }

    va_end(args);
}


>
> Just things like
>
>         iw_json ? print_string(PRINT_JSON, "manufacturer", "%.*s", sublen, data + 4) : printf("\t * Manufacturer: %.*s\n", sublen, data + 4);
>
> seem like the worst of both worlds... More obviously perhaps where it's
> not even different:
>
>         iw_json ? print_string(PRINT_JSON, NULL, "FT/IEEE 802.1X") : printf("FT/IEEE 802.1X");
>
> or
>
>         iw_json ? print_string(PRINT_JSON, NULL, "%.02x-%.02x-%.02x:%d", data[0], data[1] ,data[2], data[3]) : printf("%.02x-%.02x-%.02x:%d", data[0], data[1] ,data[2], data[3]);
>
>
> Whereas with something like
>
>         iw_json ? print_string(PRINT_JSON, "response_type", "%d%s", val, val == 3 ? " (AP)" : "") : printf("\t * Response Type: %d%s\n", val, val == 3 ? " (AP)" : "");
>
> (ugh, I don't like those long lines)
>
> you probably don't even want to have the " (AP)" thing in the JSON, but
> make it a real integer type rather than a string with print_string()!
>
> But we could make that
>
>         output_int("response_type", "Response Type", val);
>         if (val == 3)
>                 hint("(AP)");
>
> and the hint() just doesn't do anything in JSON?
>
> And maybe even "response_type" is translated to "Response Type" for the
> human output so you don't need to add the other "Response Type" there.
> Dunno. I really wouldn't mind if in some places that meant we'd now
> output "Response Type" instead of "response type" or "Response type" or
> something.
>
> Or perhaps do it the other way around, so "Response Type" is lower-cased
> and illegal chars like " " are replaced by "_", although that makes the
> JSON representation less obvious in the code.
>
> Picking one arbitrarily, we'd have something
>
>         output_int("Response Type", val);
>         if (val == 3)
>                 hint("(AP)");
>
> which seems far nicer than
>
>         iw_json ? print_string(PRINT_JSON, "response_type", "%d%s", val, val == 3 ? " (AP)" : "") : printf("\t * Response Type: %d%s\n", val, val == 3 ? " (AP)" : "");
>
> in both code and JSON representation.
>
> Yes we'll have to do something about the \n and indentation and I know
> I'm handwaving about that, but it seems like with open_object() and
> close_object() etc. that need to keep a state stack anyway, that should
> be solvable?
>
> I'd argue that we should try to get somewhere around 80-90% of the cases
> unified in this matter, and then have special things for the rest that
> are special kinds of objects, etc.
>
> Right now it's pretty much 100% special cases.
>
> johannes