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
<[email protected] <mailto:[email protected]>> wrote:
On 24 November 2014 at 20:08, Maxim Uvarov
<[email protected] <mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>
>> <mailto:[email protected]
<mailto:[email protected]>>> wrote:
>>
>>
>>
>> On 24 November 2014 06:54, Maxim Uvarov
<[email protected] <mailto:[email protected]>
>> <mailto:[email protected]
<mailto:[email protected]>>> wrote:
>>
>> Signed-off-by: Maxim Uvarov <[email protected]
<mailto:[email protected]>
>> <mailto:[email protected]
<mailto:[email protected]>>>
>>
>> ---
>> 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
>> [email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>>
>> -- *Mike Holmes*
>> Linaro Sr Technical Manager
>> LNG - ODP
>>
>> _______________________________________________
>> lng-odp mailing list
>> [email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
>
> _______________________________________________
> lng-odp mailing list
> [email protected] <mailto:[email protected]>
> http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
