Ilias Apalodimas(apalos) replied on github web page:

line 1
@@ -0,0 +1,56 @@
+/* Copyright (c) 2018, Linaro Limited

too much unrelated info to the packet io. I'd prefer this reside on a different 

> 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( 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.
updated_at 2018-02-09 10:49:49

Reply via email to