| Message ID | 1416830049-21773-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
Petri needs to decide, but for consistency with other accessors, this should be odp_pktio_name(). On Mon, Nov 24, 2014 at 5:54 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/api/odp_packet_io.h | 10 ++++++++++ > platform/linux-generic/odp_packet_io.c | 11 +++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/platform/linux-generic/include/api/odp_packet_io.h > b/platform/linux-generic/include/api/odp_packet_io.h > index 667395c..b3b7ea6 100644 > --- a/platform/linux-generic/include/api/odp_packet_io.h > +++ b/platform/linux-generic/include/api/odp_packet_io.h > @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); > */ > int odp_pktio_mtu(odp_pktio_t id); > > +/* > + * Return the interface name from a pktio handle. This is the > + * name that was passed to the odp_pktio_open() call. > + * > + * @param[in] id ODP Packet IO handle. > + * > + * @return pointer to the iface name or NULL. > + */ > +const char *odp_pktio_get_name(odp_pktio_t id); > + > /** > * @} > */ > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index c523350..9ceb989 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) > > return ifr.ifr_mtu; > } > + > +const char *odp_pktio_get_name(odp_pktio_t id) > +{ > + pktio_entry_t *entry; > + > + entry = get_entry(id); > + if (entry == NULL) > + return NULL; > + > + return entry->s.name; > +} > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 24 November 2014 06:54, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/api/odp_packet_io.h | 10 ++++++++++ > platform/linux-generic/odp_packet_io.c | 11 +++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/platform/linux-generic/include/api/odp_packet_io.h > b/platform/linux-generic/include/api/odp_packet_io.h > index 667395c..b3b7ea6 100644 > --- a/platform/linux-generic/include/api/odp_packet_io.h > +++ b/platform/linux-generic/include/api/odp_packet_io.h > @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); > */ > int odp_pktio_mtu(odp_pktio_t id); > > +/* > + * Return the interface name from a pktio handle. This is the > + * name that was passed to the odp_pktio_open() call. > + * > + * @param[in] id ODP Packet IO handle. > + * > + * @return pointer to the iface name or NULL. > I think it adds value to describe what may cause the NULL, also presumably this will always return a terminated string can an application rely on that? @retval Pointer to the interface name. @retval NULL if packet handle is invalid > + */ > +const char *odp_pktio_get_name(odp_pktio_t id); > + > /** > * @} > */ > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index c523350..9ceb989 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) > > return ifr.ifr_mtu; > } > + > +const char *odp_pktio_get_name(odp_pktio_t id) > +{ > + pktio_entry_t *entry; > + > + entry = get_entry(id); > + if (entry == NULL) > + return NULL; > + > Ensuring it is a terminated string with a removable runtime assert might be valuable. Is there a max size that this should be check against in an assert? > + return entry->s.name; > +} > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
Returning a pointer to an internal structure should be discouraged. You have no idea what the caller will do with what you return so better that they pass in something that is filled from the internal struct. That way the call is independent of the implementation or any changes to it. On Mon, Nov 24, 2014 at 9:01 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 24 November 2014 06:54, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> platform/linux-generic/include/api/odp_packet_io.h | 10 ++++++++++ >> platform/linux-generic/odp_packet_io.c | 11 +++++++++++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/platform/linux-generic/include/api/odp_packet_io.h >> b/platform/linux-generic/include/api/odp_packet_io.h >> index 667395c..b3b7ea6 100644 >> --- a/platform/linux-generic/include/api/odp_packet_io.h >> +++ b/platform/linux-generic/include/api/odp_packet_io.h >> @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); >> */ >> int odp_pktio_mtu(odp_pktio_t id); >> >> +/* >> + * Return the interface name from a pktio handle. This is the >> + * name that was passed to the odp_pktio_open() call. >> + * >> + * @param[in] id ODP Packet IO handle. >> + * >> + * @return pointer to the iface name or NULL. >> > > I think it adds value to describe what may cause the NULL, also presumably > this will always return a terminated string can an > application rely on that? > > @retval Pointer to the interface name. > @retval NULL if packet handle is invalid > >> + */ >> +const char *odp_pktio_get_name(odp_pktio_t id); >> + >> /** >> * @} >> */ >> diff --git a/platform/linux-generic/odp_packet_io.c >> b/platform/linux-generic/odp_packet_io.c >> index c523350..9ceb989 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) >> >> return ifr.ifr_mtu; >> } >> + >> +const char *odp_pktio_get_name(odp_pktio_t id) >> +{ >> + pktio_entry_t *entry; >> + >> + entry = get_entry(id); >> + if (entry == NULL) >> + return NULL; >> + >> > > Ensuring it is a terminated string with a removable runtime assert might > be valuable. > Is there a max size that this should be check against in an assert? > > >> + return entry->s.name; >> +} >> -- >> 1.8.5.1.163.gd7aced9 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > >
On 11/24/2014 09:05 PM, Bill Fischofer wrote: > Returning a pointer to an internal structure should be discouraged. > You have no idea what the caller will do with what you return so > better that they pass in something that is filled from the internal > struct. That way the call is independent of the implementation or any > changes to it. > My point was that this name already stored somewhere. You don't need to allocate memory for it twice. And of course if you call odp_pktio_close() then you can not reference to any attributes of that packet io. That call also independent and each implementation can allocate memory for that call differently. Maxim. > On Mon, Nov 24, 2014 at 9:01 AM, Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> wrote: > > > > On 24 November 2014 06:54, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/include/api/odp_packet_io.h | 10 > ++++++++++ > platform/linux-generic/odp_packet_io.c | 11 +++++++++++ > 2 files changed, 21 insertions(+) > > diff --git > a/platform/linux-generic/include/api/odp_packet_io.h > b/platform/linux-generic/include/api/odp_packet_io.h > index 667395c..b3b7ea6 100644 > --- a/platform/linux-generic/include/api/odp_packet_io.h > +++ b/platform/linux-generic/include/api/odp_packet_io.h > @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int > mtu); > */ > int odp_pktio_mtu(odp_pktio_t id); > > +/* > + * Return the interface name from a pktio handle. This is the > + * name that was passed to the odp_pktio_open() call. > + * > + * @param[in] id ODP Packet IO handle. > + * > + * @return pointer to the iface name or NULL. > > > I think it adds value to describe what may cause the NULL, also > presumably this will always return a terminated string can an > application rely on that? > > @retval Pointer to the interface name. > @retval NULL if packet handle is invalid > > + */ > +const char *odp_pktio_get_name(odp_pktio_t id); > + > /** > * @} > */ > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index c523350..9ceb989 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) > > return ifr.ifr_mtu; > } > + > +const char *odp_pktio_get_name(odp_pktio_t id) > +{ > + pktio_entry_t *entry; > + > + entry = get_entry(id); > + if (entry == NULL) > + return NULL; > + > > > Ensuring it is a terminated string with a removable runtime assert > might be valuable. > Is there a max size that this should be check against in an assert? > > + return entry->s.name <http://s.name>; > +} > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > >
On 24 November 2014 at 20:08, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/24/2014 09:05 PM, Bill Fischofer wrote: >> >> Returning a pointer to an internal structure should be discouraged. You >> have no idea what the caller will do with what you return so better that >> they pass in something that is filled from the internal struct. That way >> the call is independent of the implementation or any changes to it. >> > > My point was that this name already stored somewhere. You don't need to As Bill pointed out, this is really unsafe behavior. You don't know for how long the application will sit on that pointer and possibly reference it after the pktio instance has been freed (including all memory). Actually there is a similar case when we pass pointers (e.g. strings often used for names) when creating objects. Can the implementation save that pointer (because the caller guarantees it will be valid until the object is freed) or should the implementation make a copy of such strings? strdup() is a simple way of creating that reliable copy but I don't think we are allowed to use strdup (and thus implicitly malloc) in ODP implementations (at least not linux-generic). This is really an architectural question. Do we favor simplicity or robustness? -- Ola > allocate memory for it twice. And of course if you call odp_pktio_close() > then > you can not reference to any attributes of that packet io. That call also > independent and each implementation can allocate memory for that call > differently. > > Maxim. > > >> On Mon, Nov 24, 2014 at 9:01 AM, Mike Holmes <mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org>> wrote: >> >> >> >> On 24 November 2014 06:54, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> >> --- >> platform/linux-generic/include/api/odp_packet_io.h | 10 >> ++++++++++ >> platform/linux-generic/odp_packet_io.c | 11 +++++++++++ >> 2 files changed, 21 insertions(+) >> >> diff --git >> a/platform/linux-generic/include/api/odp_packet_io.h >> b/platform/linux-generic/include/api/odp_packet_io.h >> index 667395c..b3b7ea6 100644 >> --- a/platform/linux-generic/include/api/odp_packet_io.h >> +++ b/platform/linux-generic/include/api/odp_packet_io.h >> @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int >> mtu); >> */ >> int odp_pktio_mtu(odp_pktio_t id); >> >> +/* >> + * Return the interface name from a pktio handle. This is the >> + * name that was passed to the odp_pktio_open() call. >> + * >> + * @param[in] id ODP Packet IO handle. >> + * >> + * @return pointer to the iface name or NULL. >> >> >> I think it adds value to describe what may cause the NULL, also >> presumably this will always return a terminated string can an >> application rely on that? >> >> @retval Pointer to the interface name. >> @retval NULL if packet handle is invalid >> >> + */ >> +const char *odp_pktio_get_name(odp_pktio_t id); >> + >> /** >> * @} >> */ >> diff --git a/platform/linux-generic/odp_packet_io.c >> b/platform/linux-generic/odp_packet_io.c >> index c523350..9ceb989 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) >> >> return ifr.ifr_mtu; >> } >> + >> +const char *odp_pktio_get_name(odp_pktio_t id) >> +{ >> + pktio_entry_t *entry; >> + >> + entry = get_entry(id); >> + if (entry == NULL) >> + return NULL; >> + >> >> >> Ensuring it is a terminated string with a removable runtime assert >> might be valuable. >> Is there a max size that this should be check against in an assert? >> >> + return entry->s.name <http://s.name>; >> +} >> -- >> 1.8.5.1.163.gd7aced9 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- *Mike Holmes* >> Linaro Sr Technical Manager >> LNG - ODP >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
odp_buffer_pool_create() uses strncpy() to copy the caller-supplied name into an internal struct and I believe most of the other routines that take names follow a similar strategy. Using that to copy names back for retrieval seems a consistent policy. The issue here isn't to make APIs bulletproof so much as it is to make application programming errors cause failure close to the point of error rather than to linger as latent time bombs. Bill On Mon, Nov 24, 2014 at 2:49 PM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 24 November 2014 at 20:08, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > > On 11/24/2014 09:05 PM, Bill Fischofer wrote: > >> > >> Returning a pointer to an internal structure should be discouraged. You > >> have no idea what the caller will do with what you return so better that > >> they pass in something that is filled from the internal struct. That > way > >> the call is independent of the implementation or any changes to it. > >> > > > > My point was that this name already stored somewhere. You don't need to > As Bill pointed out, this is really unsafe behavior. You don't know for > how long > the application will sit on that pointer and possibly reference it > after the pktio > instance has been freed (including all memory). > > Actually there is a similar case when we pass pointers (e.g. strings often > used > for names) when creating objects. Can the implementation save that pointer > (because the caller guarantees it will be valid until the object is > freed) or should > the implementation make a copy of such strings? strdup() is a simple way of > creating that reliable copy but I don't think we are allowed to use strdup > (and > thus implicitly malloc) in ODP implementations (at least not > linux-generic). > > This is really an architectural question. Do we favor simplicity or > robustness? > > -- Ola > > > allocate memory for it twice. And of course if you call odp_pktio_close() > > then > > you can not reference to any attributes of that packet io. That call also > > independent and each implementation can allocate memory for that call > > differently. > > > > Maxim. > > > > > >> On Mon, Nov 24, 2014 at 9:01 AM, Mike Holmes <mike.holmes@linaro.org > >> <mailto:mike.holmes@linaro.org>> wrote: > >> > >> > >> > >> On 24 November 2014 06:54, Maxim Uvarov <maxim.uvarov@linaro.org > >> <mailto:maxim.uvarov@linaro.org>> wrote: > >> > >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > >> <mailto:maxim.uvarov@linaro.org>> > >> > >> --- > >> platform/linux-generic/include/api/odp_packet_io.h | 10 > >> ++++++++++ > >> platform/linux-generic/odp_packet_io.c | 11 +++++++++++ > >> 2 files changed, 21 insertions(+) > >> > >> diff --git > >> a/platform/linux-generic/include/api/odp_packet_io.h > >> b/platform/linux-generic/include/api/odp_packet_io.h > >> index 667395c..b3b7ea6 100644 > >> --- a/platform/linux-generic/include/api/odp_packet_io.h > >> +++ b/platform/linux-generic/include/api/odp_packet_io.h > >> @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int > >> mtu); > >> */ > >> int odp_pktio_mtu(odp_pktio_t id); > >> > >> +/* > >> + * Return the interface name from a pktio handle. This is the > >> + * name that was passed to the odp_pktio_open() call. > >> + * > >> + * @param[in] id ODP Packet IO handle. > >> + * > >> + * @return pointer to the iface name or NULL. > >> > >> > >> I think it adds value to describe what may cause the NULL, also > >> presumably this will always return a terminated string can an > >> application rely on that? > >> > >> @retval Pointer to the interface name. > >> @retval NULL if packet handle is invalid > >> > >> + */ > >> +const char *odp_pktio_get_name(odp_pktio_t id); > >> + > >> /** > >> * @} > >> */ > >> diff --git a/platform/linux-generic/odp_packet_io.c > >> b/platform/linux-generic/odp_packet_io.c > >> index c523350..9ceb989 100644 > >> --- a/platform/linux-generic/odp_packet_io.c > >> +++ b/platform/linux-generic/odp_packet_io.c > >> @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) > >> > >> return ifr.ifr_mtu; > >> } > >> + > >> +const char *odp_pktio_get_name(odp_pktio_t id) > >> +{ > >> + pktio_entry_t *entry; > >> + > >> + entry = get_entry(id); > >> + if (entry == NULL) > >> + return NULL; > >> + > >> > >> > >> Ensuring it is a terminated string with a removable runtime assert > >> might be valuable. > >> Is there a max size that this should be check against in an assert? > >> > >> + return entry->s.name <http://s.name>; > >> +} > >> -- > >> 1.8.5.1.163.gd7aced9 > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > >> http://lists.linaro.org/mailman/listinfo/lng-odp > >> > >> > >> > >> > >> -- *Mike Holmes* > >> Linaro Sr Technical Manager > >> LNG - ODP > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > >> http://lists.linaro.org/mailman/listinfo/lng-odp > >> > >> > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 11/25/2014 12:17 AM, Bill Fischofer wrote: > odp_buffer_pool_create() uses strncpy() to copy the caller-supplied > name into an internal struct and I believe most of the other routines > that take names follow a similar strategy. Using that to copy names > back for retrieval seems a consistent policy. The issue here isn't to > make APIs bulletproof so much as it is to make application programming > errors cause failure close to the point of error rather than to linger > as latent time bombs. > > Bill > odp_pktio_entry structure has name of that pktio. And current approach will mostly segfault on not correct access. I.e. pktio = odp_pktio_open("eth0") char *name = odp_pktio_get_name(pktio); <here any access for name is valid> odp_pktio_close(pktio); < segfault on *name access> Now you want to print name for not existence pktio. Might be segfault, might be wrong name for newly created pktio. Depends how memory was allocated. From my point of view that situation is correct. We should never reference to dead objects. If app needs to print of old pktio objects name than this app should reference to it's own app memory, not for name of old object. I.e. app should have a local copy of that name to apps memory. API name is odp_pktio_get_name(), not odp_pktio_alloc_and_get_name(pktio). So I think it should be even clear that API does not allocate storage for that memory (no strdup). Maxim. > On Mon, Nov 24, 2014 at 2:49 PM, Ola Liljedahl > <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>> wrote: > > On 24 November 2014 at 20:08, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: > > On 11/24/2014 09:05 PM, Bill Fischofer wrote: > >> > >> Returning a pointer to an internal structure should be > discouraged. You > >> have no idea what the caller will do with what you return so > better that > >> they pass in something that is filled from the internal > struct. That way > >> the call is independent of the implementation or any changes to it. > >> > > > > My point was that this name already stored somewhere. You don't > need to > As Bill pointed out, this is really unsafe behavior. You don't > know for how long > the application will sit on that pointer and possibly reference it > after the pktio > instance has been freed (including all memory). > > Actually there is a similar case when we pass pointers (e.g. > strings often used > for names) when creating objects. Can the implementation save that > pointer > (because the caller guarantees it will be valid until the object is > freed) or should > the implementation make a copy of such strings? strdup() is a > simple way of > creating that reliable copy but I don't think we are allowed to > use strdup (and > thus implicitly malloc) in ODP implementations (at least not > linux-generic). > > This is really an architectural question. Do we favor simplicity > or robustness? > > -- Ola > > > allocate memory for it twice. And of course if you call > odp_pktio_close() > > then > > you can not reference to any attributes of that packet io. That > call also > > independent and each implementation can allocate memory for that > call > > differently. > > > > Maxim. > > > > > >> On Mon, Nov 24, 2014 at 9:01 AM, Mike Holmes > <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org> > >> <mailto:mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>>> wrote: > >> > >> > >> > >> On 24 November 2014 06:54, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > >> <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > >> > >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > >> <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > >> > >> --- > >> platform/linux-generic/include/api/odp_packet_io.h | 10 > >> ++++++++++ > >> platform/linux-generic/odp_packet_io.c | 11 +++++++++++ > >> 2 files changed, 21 insertions(+) > >> > >> diff --git > >> a/platform/linux-generic/include/api/odp_packet_io.h > >> b/platform/linux-generic/include/api/odp_packet_io.h > >> index 667395c..b3b7ea6 100644 > >> --- a/platform/linux-generic/include/api/odp_packet_io.h > >> +++ b/platform/linux-generic/include/api/odp_packet_io.h > >> @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t > id, int > >> mtu); > >> */ > >> int odp_pktio_mtu(odp_pktio_t id); > >> > >> +/* > >> + * Return the interface name from a pktio handle. This > is the > >> + * name that was passed to the odp_pktio_open() call. > >> + * > >> + * @param[in] id ODP Packet IO handle. > >> + * > >> + * @return pointer to the iface name or NULL. > >> > >> > >> I think it adds value to describe what may cause the NULL, also > >> presumably this will always return a terminated string can an > >> application rely on that? > >> > >> @retval Pointer to the interface name. > >> @retval NULL if packet handle is invalid > >> > >> + */ > >> +const char *odp_pktio_get_name(odp_pktio_t id); > >> + > >> /** > >> * @} > >> */ > >> diff --git a/platform/linux-generic/odp_packet_io.c > >> b/platform/linux-generic/odp_packet_io.c > >> index c523350..9ceb989 100644 > >> --- a/platform/linux-generic/odp_packet_io.c > >> +++ b/platform/linux-generic/odp_packet_io.c > >> @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) > >> > >> return ifr.ifr_mtu; > >> } > >> + > >> +const char *odp_pktio_get_name(odp_pktio_t id) > >> +{ > >> + pktio_entry_t *entry; > >> + > >> + entry = get_entry(id); > >> + if (entry == NULL) > >> + return NULL; > >> + > >> > >> > >> Ensuring it is a terminated string with a removable runtime > assert > >> might be valuable. > >> Is there a max size that this should be check against in an > assert? > >> > >> + return entry->s.name <http://s.name> > <http://s.name>; > >> +} > >> -- > >> 1.8.5.1.163.gd7aced9 > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>> > >> http://lists.linaro.org/mailman/listinfo/lng-odp > >> > >> > >> > >> > >> -- *Mike Holmes* > >> Linaro Sr Technical Manager > >> LNG - ODP > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>> > >> http://lists.linaro.org/mailman/listinfo/lng-odp > >> > >> > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > > http://lists.linaro.org/mailman/listinfo/lng-odp > >
Looks like I'm guilty of the same offense since Petri has defined odp_buffer_pool_info_t to return the pool name as const char *name rather than char name[ODP_MAX_NAME_LEN]. So as of now we've architected the return of a pointer to an implementation-internal struct. On Mon, Nov 24, 2014 at 3:36 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/25/2014 12:17 AM, Bill Fischofer wrote: > >> odp_buffer_pool_create() uses strncpy() to copy the caller-supplied name >> into an internal struct and I believe most of the other routines that take >> names follow a similar strategy. Using that to copy names back for >> retrieval seems a consistent policy. The issue here isn't to make APIs >> bulletproof so much as it is to make application programming errors cause >> failure close to the point of error rather than to linger as latent time >> bombs. >> >> Bill >> >> > odp_pktio_entry structure has name of that pktio. And current approach > will mostly segfault on not correct access. > I.e. > > pktio = odp_pktio_open("eth0") > char *name = odp_pktio_get_name(pktio); > <here any access for name is valid> > odp_pktio_close(pktio); > < segfault on *name access> > > Now you want to print name for not existence pktio. Might be segfault, > might be wrong name for newly created pktio. Depends how memory was > allocated. > > From my point of view that situation is correct. We should never reference > to dead objects. If app needs to print of old pktio objects name than this > app should reference to it's own app memory, not for name of old object. > I.e. app should have a local copy of that name to apps memory. API name is > odp_pktio_get_name(), > not odp_pktio_alloc_and_get_name(pktio). So I think it should be even > clear that API does not allocate storage for that memory (no strdup). > > Maxim. > > On Mon, Nov 24, 2014 at 2:49 PM, Ola Liljedahl <ola.liljedahl@linaro.org >> <mailto:ola.liljedahl@linaro.org>> wrote: >> >> On 24 November 2014 at 20:08, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >> > On 11/24/2014 09:05 PM, Bill Fischofer wrote: >> >> >> >> Returning a pointer to an internal structure should be >> discouraged. You >> >> have no idea what the caller will do with what you return so >> better that >> >> they pass in something that is filled from the internal >> struct. That way >> >> the call is independent of the implementation or any changes to it. >> >> >> > >> > My point was that this name already stored somewhere. You don't >> need to >> As Bill pointed out, this is really unsafe behavior. You don't >> know for how long >> the application will sit on that pointer and possibly reference it >> after the pktio >> instance has been freed (including all memory). >> >> Actually there is a similar case when we pass pointers (e.g. >> strings often used >> for names) when creating objects. Can the implementation save that >> pointer >> (because the caller guarantees it will be valid until the object is >> freed) or should >> the implementation make a copy of such strings? strdup() is a >> simple way of >> creating that reliable copy but I don't think we are allowed to >> use strdup (and >> thus implicitly malloc) in ODP implementations (at least not >> linux-generic). >> >> This is really an architectural question. Do we favor simplicity >> or robustness? >> >> -- Ola >> >> > allocate memory for it twice. And of course if you call >> odp_pktio_close() >> > then >> > you can not reference to any attributes of that packet io. That >> call also >> > independent and each implementation can allocate memory for that >> call >> > differently. >> > >> > Maxim. >> > >> > >> >> On Mon, Nov 24, 2014 at 9:01 AM, Mike Holmes >> <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org> >> >> <mailto:mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org>>> wrote: >> >> >> >> >> >> >> >> On 24 November 2014 06:54, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> >> <mailto:maxim.uvarov@linaro.org >> >> <mailto:maxim.uvarov@linaro.org>>> >> >> >> >> --- >> >> platform/linux-generic/include/api/odp_packet_io.h | 10 >> >> ++++++++++ >> >> platform/linux-generic/odp_packet_io.c | 11 +++++++++++ >> >> 2 files changed, 21 insertions(+) >> >> >> >> diff --git >> >> a/platform/linux-generic/include/api/odp_packet_io.h >> >> b/platform/linux-generic/include/api/odp_packet_io.h >> >> index 667395c..b3b7ea6 100644 >> >> --- a/platform/linux-generic/include/api/odp_packet_io.h >> >> +++ b/platform/linux-generic/include/api/odp_packet_io.h >> >> @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t >> id, int >> >> mtu); >> >> */ >> >> int odp_pktio_mtu(odp_pktio_t id); >> >> >> >> +/* >> >> + * Return the interface name from a pktio handle. This >> is the >> >> + * name that was passed to the odp_pktio_open() call. >> >> + * >> >> + * @param[in] id ODP Packet IO handle. >> >> + * >> >> + * @return pointer to the iface name or NULL. >> >> >> >> >> >> I think it adds value to describe what may cause the NULL, also >> >> presumably this will always return a terminated string can an >> >> application rely on that? >> >> >> >> @retval Pointer to the interface name. >> >> @retval NULL if packet handle is invalid >> >> >> >> + */ >> >> +const char *odp_pktio_get_name(odp_pktio_t id); >> >> + >> >> /** >> >> * @} >> >> */ >> >> diff --git a/platform/linux-generic/odp_packet_io.c >> >> b/platform/linux-generic/odp_packet_io.c >> >> index c523350..9ceb989 100644 >> >> --- a/platform/linux-generic/odp_packet_io.c >> >> +++ b/platform/linux-generic/odp_packet_io.c >> >> @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) >> >> >> >> return ifr.ifr_mtu; >> >> } >> >> + >> >> +const char *odp_pktio_get_name(odp_pktio_t id) >> >> +{ >> >> + pktio_entry_t *entry; >> >> + >> >> + entry = get_entry(id); >> >> + if (entry == NULL) >> >> + return NULL; >> >> + >> >> >> >> >> >> Ensuring it is a terminated string with a removable runtime >> assert >> >> might be valuable. >> >> Is there a max size that this should be check against in an >> assert? >> >> >> >> + return entry->s.name <http://s.name> >> <http://s.name>; >> >> +} >> >> -- >> >> 1.8.5.1.163.gd7aced9 >> >> >> >> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>> >> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> >> >> >> >> >> -- *Mike Holmes* >> >> Linaro Sr Technical Manager >> >> LNG - ODP >> >> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>> >> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h index 667395c..b3b7ea6 100644 --- a/platform/linux-generic/include/api/odp_packet_io.h +++ b/platform/linux-generic/include/api/odp_packet_io.h @@ -148,6 +148,16 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); */ int odp_pktio_mtu(odp_pktio_t id); +/* + * Return the interface name from a pktio handle. This is the + * name that was passed to the odp_pktio_open() call. + * + * @param[in] id ODP Packet IO handle. + * + * @return pointer to the iface name or NULL. + */ +const char *odp_pktio_get_name(odp_pktio_t id); + /** * @} */ diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c index c523350..9ceb989 100644 --- a/platform/linux-generic/odp_packet_io.c +++ b/platform/linux-generic/odp_packet_io.c @@ -542,3 +542,14 @@ int odp_pktio_mtu(odp_pktio_t id) return ifr.ifr_mtu; } + +const char *odp_pktio_get_name(odp_pktio_t id) +{ + pktio_entry_t *entry; + + entry = get_entry(id); + if (entry == NULL) + return NULL; + + return entry->s.name; +}
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/include/api/odp_packet_io.h | 10 ++++++++++ platform/linux-generic/odp_packet_io.c | 11 +++++++++++ 2 files changed, 21 insertions(+)