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.
