Mykyta Iziumtsev(MykytaI) replied on github web page:

platform/linux-generic/include/odp_pcapng.h
line 23
@@ -0,0 +1,56 @@
+/* Copyright (c) 2018, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier:     BSD-3-Clause
+ */
+
+#include <protocols/eth.h>
+#include <protocols/ip.h>
+#include <odp/api/plat/packet_inlines.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/fcntl.h>
+#include <unistd.h>
+#include <pthread.h>
+
+#define PCAPNG_BLOCK_TYPE_EPB 0x00000006UL
+#define PCAPNG_BLOCK_TYPE_SHB 0x0A0D0D0AUL
+#define PCAPNG_BLOCK_TYPE_IDB 0x00000001UL
+#define PCAPNG_ENDIAN_MAGIC 0x1A2B3C4DUL
+#define PCAP_DATA_ALIGN (4)
+
+/* inotify */
+#define INOTIFY_EVENT_SIZE  (sizeof(struct inotify_event))


Comment:
I believe sizeof(struct) directly in code is more informative.

> Mykyta Iziumtsev(MykytaI) wrote:
> I'm not sure event aggregation has any value here, but even if it does -- we 
> don't need buffer for 1024 events. 


>> Ilias Apalodimas(apalos) wrote:
>> The + 16 is just a reasonable guess. The event inotify returns might or 
>> might not have an event->name of dynamic size. If the buffer is too small, 
>> the system call returns zero. Since it allows event slurping i am trying to 
>> read multiple events at once.
>> 
>> i could change this and use FIONREAD ioctl to get the exact number of bytes 
>> i can read from the inotify fd


>>> Ilias Apalodimas(apalos) wrote:
>>> will fix


>>>> Ilias Apalodimas(apalos) wrote:
>>>> will fix


>>>>> Mykyta Iziumtsev(MykytaI) wrote:
>>>>> Any justification for +16 and such a big buffer on a stack ? Do we really 
>>>>> need 1024 events at once ?


>>>>>> Mykyta Iziumtsev(MykytaI) wrote:
>>>>>> Shall be _t


>>>>>>> Mykyta Iziumtsev(MykytaI) wrote:
>>>>>>> In fact we do care. We can't tolerate partial packet writes, otherwise 
>>>>>>> pcapng file will be unreadable. The proper way to address that is to 
>>>>>>> see how much space is left in the pipe (buf_size - TIOCOUTQ) and append 
>>>>>>> packets to iovec as long as they fit in the calculated 'budget'.
>>>>>>> 
>>>>>>> Another thing to consider -- is increasing pipe buffer size. The linux 
>>>>>>> default size is 64K, so 'dd' command you used doesn't make much sense. 
>>>>>>> Please consider setting it with fcntl to max size (1M according to 
>>>>>>> /proc/sys/fs/pipe-max-size).


>>>>>>>> Mykyta Iziumtsev(MykytaI) wrote:
>>>>>>>> I guess only <stdint.h> and <sys/inotify.h> are needed here.


>>>>>>>>> Mykyta Iziumtsev(MykytaI) wrote:
>>>>>>>>> Is there any reason to put all of this into separate header file ? 
>>>>>>>>> From what I see only one .c file is using it, so why not move it 
>>>>>>>>> there ?


>>>>>>>>>> Mykyta Iziumtsev(MykytaI) wrote:
>>>>>>>>>> IMHO, only `int pcapng_fds[PKTIO_MAX_QUEUES]` belong here, the rest 
>>>>>>>>>> is superfluous or belongs to global context. Why not initialize 
>>>>>>>>>> pcapng_fds[0 .. ARRAY_SIZE - 1] = -1, open the pipes from inotify 
>>>>>>>>>> thread and assign fds when the header is already written out ?


>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>> @apalos No, start and stop are good times for this as well. An 
>>>>>>>>>>> application can stop a device, reconfigure it, and then restart it, 
>>>>>>>>>>> so having that be the trigger for capture file cleanup and close 
>>>>>>>>>>> makes sense.


>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>> wireshark re-defines them in every c file they want to use them. 
>>>>>>>>>>>> Keeping in mind this is the pcapng file format specification, 
>>>>>>>>>>>> these are high unlikely to change.
>>>>>>>>>>>> 
>>>>>>>>>>>> The only relevant library i found is 
>>>>>>>>>>>> ntar(https://www.winpcap.org/ntar/changelog.htm) but it seems 
>>>>>>>>>>>> abandoned for many many years.


>>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>>> @Bill-Fischofer-Linaro pcapng_destroy() is called from 
>>>>>>>>>>>>> _pktio_stop() which is called from odp_pktio_term_global().
>>>>>>>>>>>>> pcapng_prepare() on the other hand is called from 
>>>>>>>>>>>>> odp_pktio_start(). Want me to move it in odp_pktio_init_global()?


>>>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>>>> This essentially "reads" the inotify triggers for 
>>>>>>>>>>>>>> opening/closing the fifos and sets the appropriate variables for 
>>>>>>>>>>>>>> starting/stopping the pcapng dump on fifos. Will try to make it 
>>>>>>>>>>>>>> easier to read on the final PR


>>>>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>>>>> will fix


>>>>>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>>>>>> will fix


>>>>>>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>>>>>>> I'll check and update for the final PR


>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>> General cleanup should be part of `odp_term_global()` 
>>>>>>>>>>>>>>>>>> processing. Presumably `odp_pktio_close()` should also do 
>>>>>>>>>>>>>>>>>> cleanup for specific pktios.


>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>> A few comments about what this routine is doing would be 
>>>>>>>>>>>>>>>>>>> very helpful here. This is not easy code to follow. 
>>>>>>>>>>>>>>>>>>> Modularizing it into some  `static inline` helpers might 
>>>>>>>>>>>>>>>>>>> also make the logic simpler.


>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>> This is redundant. If `len > 0` then by definition `len != 
>>>>>>>>>>>>>>>>>>>> -1`.


>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>> This fails for multi-segment packets. 
>>>>>>>>>>>>>>>>>>>>> `odp_packet_data(pkt)` is shorthand for 
>>>>>>>>>>>>>>>>>>>>> `odp_packet_offset(pkt, 0, NULL, NULL)` so you don't know 
>>>>>>>>>>>>>>>>>>>>> how long the first segment really is. First cut would be 
>>>>>>>>>>>>>>>>>>>>> to only dump the first segment, so correct code is:
>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>> uint32_t seg_len;
>>>>>>>>>>>>>>>>>>>>> char *buf = (char *)odp_packet_offset(packets[i], 0, 
>>>>>>>>>>>>>>>>>>>>> &seg_len, NULL);
>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>> And use `seg_len` rather than `pkt_len` in this routine.


>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>> Does pcapng not supply a `.h` file for these "magic 
>>>>>>>>>>>>>>>>>>>>>> numbers" and associated structs?


>>>>>>>>>>>>>>>>>>>>>>> Ilias Apalodimas(apalos) wrote:
>>>>>>>>>>>>>>>>>>>>>>> Will fix. This will also remove the requirement of 
>>>>>>>>>>>>>>>>>>>>>>> manually deleting the fifos


>>>>>>>>>>>>>>>>>>>>>>>> muvarov wrote
>>>>>>>>>>>>>>>>>>>>>>>> odp_global_data_s->main_pid needs to be here.


https://github.com/Linaro/odp/pull/435#discussion_r166596354
updated_at 2018-02-07 11:57:25

Reply via email to