[bugzilla-daemon@xxxxxxxxxxxxx]

Michal Labedzki changed bug 8279

What	Removed	Added
Attachment #9907 is obsolete		1
Attachment #9907 Flags	review_for_checkin?
Attachment #9962 Flags		review_for_checkin?

Comment # 3 on bug 8279 from Michal Labedzki

Created attachment 9962 [details]
[PATCH] Wiretap: Add support for Android Logcat

Hello Jakub,


(In reply to comment #2)
> Hi Michał,
>
> Great! logcat seems to be interesting format, can you link to some
> specification?

Specification... Never seen... I am starting on reverse engineering, but later
I read source code of Android.


> Quick review:
>
> 1/ wiretap/logcat.c
>
> 48 static gchar get_priority(gchar *priority) {
>
> 51 if (*priority >= (gchar) sizeof(priorities))
>
> priority is signed char, so *priority can be < 0.

Ok. So now *priority is guint8.


> 2/ wiretap/wtap.h
>
> I'm not sure exactly how it works, but it seems only WTAP_ENCAP_LOGCAT is
> used.

That's right. But see wtap.c "encap_table_base" <-- number of mentioned defines
must be the same that number of supported written (Save as) formats. So we
support for "open" only "WTAP_ENCAP_LOGCAT", but for "save":
#define WTAP_ENCAP_LOGCAT                       152
#define WTAP_ENCAP_LOGCAT_BRIEF                 153
#define WTAP_ENCAP_LOGCAT_PROCESS               154
#define WTAP_ENCAP_LOGCAT_TAG                   155
#define WTAP_ENCAP_LOGCAT_THREAD                156
#define WTAP_ENCAP_LOGCAT_TIME                  157
#define WTAP_ENCAP_LOGCAT_THREADTIME            158
#define WTAP_ENCAP_LOGCAT_LONG                  159

Maybe last 7 text-formats will be written later (if needed, because people
often save logcat logs as text files...)


> 3/ wiretap/logcat.c
>
> 208 wth->phdr.ts.secs  = GINT32_FROM_LE(*((gint32 *) &buf[3 * 4]));
> 209 wth->phdr.ts.nsecs = GINT32_FROM_LE(*((gint32 *) &buf[4 * 4]));
>
> Please check wiretap/wtap-int.h but I believe pletohl() should be used.

pletohl seems to be for unsigned, but there is needed signed variables.


> 4/ wiretap/logcat.c
>
> 180 if (file_seek(wth->fh, *data_offset, SEEK_SET, err) == -1)
> 181   return FALSE;
>
> ...
>
> 193 if (file_seek(wth->fh, *data_offset, SEEK_SET, err) == -1)
> 194   return FALSE;
>
> not *data_offset nor file offset seems to be changed.
> Also I'd rather reconstruct this first two bytes from payload_length and not
> seek file, but premature optimization is the root of all evil, so let's
> leave it :)


Ok, 193, 194 are removed.


> 5/ wiretap/logcat.c
>
> Speaking about payload_length:
>
> 172 bytes_read = file_read(&payload_length, 2, wth->fh);
>
> Please read it to some payload_tmp[2] and use pletohs().
>

Done, thanks.

> I don't have time for full review (sorry),
> Kuba.

Ok, your review is great help.

---

      You are receiving this mail because:

• You are watching all bug changes.