Mykyta Iziumtsev(MykytaI) replied on github web page:

platform/linux-generic/include/odp_pcapng.h
line 1
@@ -0,0 +1,56 @@
+/* Copyright (c) 2018, Linaro Limited


Comment:
Maybe consider moving code to separate .c file then if it has nothing to do 
with packet io ?

> Ilias Apalodimas(apalos) wrote:
> too much unrelated info to the packet io. I'd prefer this reside on a 
> different file


>> Mykyta Iziumtsev(MykytaI) wrote:
>> 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_r167198029
updated_at 2018-02-09 10:57:37

Reply via email to