Mykyta Iziumtsev(MykytaI) replied on github web page: platform/linux-generic/include/odp_pcapng.h line 14 @@ -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>
Comment: 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_r166566828 updated_at 2018-02-07 10:31:08
