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