Ilias Apalodimas(apalos) replied on github web page: platform/linux-generic/include/odp_pcapng.h line 1 @@ -0,0 +1,56 @@ +/* Copyright (c) 2018, Linaro Limited
Comment: 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_r167196200 updated_at 2018-02-09 10:49:49
