On 29 January 2015 at 14:04, Savolainen, Petri (NSN - FI/Espoo)
<[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: [email protected] [mailto:lng-odp-
>> [email protected]] On Behalf Of ext Ola Liljedahl
>> Sent: Wednesday, January 28, 2015 7:06 PM
>> To: LNG ODP Mailman List
>> Subject: [lng-odp] API questions - sizes and numelems as inputs and
>> outputs
>>
>> Talking about API's that take some type of element count or buffer
>> size as input and return a value related to that. There seems to be a
>> number of different models.
>>
>> /**
>> * Generate random byte string
>> *
>> * @param buf Pointer to store result
>> * @param len Pointer to input length value as well as return
>> value
>> I assume the "return value" is the number of result bytes stored. But
>> this is not very clear.
>>
>> * @param use_entropy Use entropy
>> *
>> * @todo Define the implication of the use_entropy parameter
>> *
>> * @retval 0 on success
>> Not the same "return value" as described above.
>>
>> * @retval <0 on failure
>> */
>> int odp_hw_random_get(uint8_t *buf, size_t *len, bool use_entropy);
>> Here we return the actual size written (I assume) through the len
>> parameter which is consequently passed by reference.
>
> How use full the function is if it does not fill the whole buffer with rand
> data? I think in this case should be success vs. failure. In success the
> whole buffer is written.
I don't know, are we ruling out all potential cases where you get less
bytes of data than requested still being useful? Is it the
responsibility of ODP to decide this? I think this high order decision
belongs to the application and not the ODP implementation. The
application can always retry the operation until it has as much random
data that as it needs.
>
>>
>> /**
>> * Write CPU mask as a string of hexadecimal digits
>> *
>> * @param mask CPU mask to write
>> * @param buf Output buffer
>> * @param bufsz Size of output buffer
>> *
>> * @return >0 on success. Number of characters written (including the
>> * terminating null char).
>> * @return 0 on failure (output buffer too small).
>> */
>> size_t odp_cpumask_to_str(const odp_cpumask_t *mask, char *buf, size_t
>> bufsz);
>> The new definition of odp_cpumask_to_str() as recently agreed. Number
>> of characters written is returned. It can't be 0 so 0 can be used as
>> an error indicator. In other formatting-type of functions, 0 might be
>> a valid return value (it primarily depends on whether the terminating
>> null char should be included in the count).
Change per delta doc.
>>
>> * @param[out] mac_addr Storage for MAC address of the packet IO
>> interface.
>> * @param addr_size Storage size for the address
>> *
>> * @retval Number of bytes written on success, 0 on failure.
>> */
>> size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr, size_t
>> addr_size);
>> This function returns 0 for failure (but this is incidental and
>> prevents us to support pktio types that have zero-length MAC address
>> (i.e. does not have any MAC address)).
>
> What is a successful copy of a zero length MAC address?
I don't see this special case. I see the case "what is a successful
copy of a N-length MAC address?". The answer is obvious. Why is
different for N==0
Change per delta doc.
>I think it's OK to return failure (==0), if user asks for a copy of MAC
>address from a interface that do not have one.
In some API's 0 means success and in other it means failure. This is
confusing. I don't see why we cannot unify the API's and have 0 always
mean success.
There is nothing intrinsic to odp_pktio_mac_addr() which makes 0 the
most suitable or obvious failure indication.
odp_pktio_mac_addr() needs to return the number of bytes written, i.e.
the size of the MAC address because this is not a universal constant.
>
>>
>> /**
>> * Copy data from packet
>> *
>> * Copy 'len' bytes of data from the packet level offset to the
>> destination
>> * address.
>> *
>> * @param pkt Packet handle
>> * @param offset Byte offset into the packet
>> * @param len Number of bytes to copy
>> * @param dst Destination address
>> *
>> * @retval 0 on success
>> * @retval <0 on failure
>> Not documented what constitutes a failure.
>> */
>> int odp_packet_copydata_out(odp_packet_t pkt, uint32_t offset,
>> uint32_t len, void *dst);
>> What if less than len bytes could be copied (because reaching end of
>> packet)? How is this reported? Should we use "uint32_t *len" and
>> update it on return with the actual number of bytes copied? Or is this
>> situation considered an error and reported with a negative return
>> value? Seems excessive. Better to just specify len as the size of the
>> output buffer and in some way be notified about the actual number of
>> bytes copied.
>
> It's better to keep this simple: success or failure. In success "len" bytes
> was copied. It's not important how many bytes were copied in failure.
So partial success (only N bytes left to copy in packet) should be
considered a failure. How does this affect the application code that
uses this call? Anyway the definition needs to be updated because the
semantics are not clear.
I think this call should follow the common model that you attempt to
copy/read N bytes (often the size of your output buffer) and you get
as many bytes as there are left (<= N).
>
> Failures:
> offset > odp_packet_len()
> offset + len > odp_packet_len()
Simpler if the implementation truncates the number of bytes copied
when there is not room enough (it has to check anyway). Return the
actual number of bytes copied.
>
>
>>
>> /**
>> * Copy data into packet
>> *
>> * Copy 'len' bytes of data from the source address into the packet
>> level
>> * offset. Maximum number of bytes to copy is packet data length minus the
>> * offset. Packet is not modified on an error.
>> *
>> * @param pkt Packet handle
>> * @param offset Byte offset into the packet
>> * @param len Number of bytes to copy
>> * @param src Source address
>> *
>> * @retval 0 on success
>> * @retval <0 on failure
>> Not documented what constitutes a failure.
>> */
>> int odp_packet_copydata_in(odp_packet_t pkt, uint32_t offset,
>> uint32_t len, const void *src);
>> Similar for this function. Is it an error if 'len' bytes does not fit
>> into the current packet size? Is this good semantics?
>
> Failures:
> offset > odp_packet_len()
> offset + len > odp_packet_len()
>
>>
>> /**
>> * Receive packets
>> *
>> * @param id ODP packet IO handle
>> * @param pkt_table[] Storage for received packets (filled by function)
>> * @param len Length of pkt_table[], i.e. max number of pkts to
>> receive
>> *
>> * @return Number of packets received
>> * @retval <0 on failure
>> */
>> int odp_pktio_recv(odp_pktio_t id, odp_packet_t pkt_table[], unsigned
>> len);
>> int odp_pktio_send(odp_pktio_t id, odp_packet_t pkt_table[], unsigned
>> len);
>> Here is a classic. Attempt to receive up to N items. Return actual
>> number of items received or <0 on failure. 0 is a valid success code
>> (not a failure). Why can't this be our model?
>> The prototype uses unsigned and signed ints. How is different from
>> using size_t and ssize_t?
>
>
> This is why we are going through the APIs and check/harmonize return values
> ... the function definition is from Nov 13.
>
> Int is good return type here. Should we use "int len" param to match the
> return value?
OK with me. The range of unsigned int is not needed.
>
> int i = 0;
> int len = 10;
>
> while (len) {
> int sent = odp_pktio_send(id, &pkt[i], len);
>
> if (sent < 0)
> ABORT();
>
> i += sent;
> len -= sent;
> }
>
>
> This would generate compiler warning if mixing unsigned and int:
Yes that's why we should use int (or potentially ssize_t in same
cases) for both the input parameters (e.g. buffer size) and return
type. No problem implicitly casting a size_t value to an ssize_t
parameter.
>
> unsigned len = 10;
>
> int sent = odp_pktio_send(id, &pkt[i], len);
>
> if (sent != len)
> LOG("could not send everything at once");
>
>
>>
>> /**
>> * Enqueue multiple events to a queue
>> *
>> * @param queue Queue handle
>> * @param ev Event handles
>> * @param num Number of event handles
>> *
>> * @retval 0 on success
>> * @retval <0 on failure
>> */
>> int odp_queue_enq_multi(odp_queue_t queue, odp_event_t ev[], int num);
>> Here we use "int" for the number of items, at least this matches with
>> the return type. But there is no indication that the function returns
>> the number of events that were enqueued. What happens if we are trying
>> to enqueue events to a fixed-size queue (e.g. ring buffer) and all
>> items do not fit? Is this considered a failure? It could be
>> problematic to have to retry this call until it is possible to enqueue
>> all items in the same go. Incremental success should be allowed for
>> unless there is a good explanation why this "atomic" semantics is
>> desired. The application can decide what to do with those events that
>> are not enqueued.
>>
>> Same for this:
>> int odp_queue_deq_multi(odp_queue_t queue, odp_event_t events[], int num);
>
> True. It's reasonable to allow incremental usage.
>
> @return Number of enqueued events on success.
Updated delta doc.
>
>
>>
>> /**
>> * Schedule multiple events
>> *
>> * Like odp_schedule(), but returns multiple events from a queue.
>> *
>> * @param from Output parameter for the source queue (where the event
>> was
>> * dequeued from). Ignored if NULL.
>> * @param wait Minimum time to wait for an event. Waits infinitely, if
>> set to
>> * ODP_SCHED_WAIT. Does not wait, if set to
>> ODP_SCHED_NO_WAIT.
>> * Use odp_schedule_wait_time() to convert time to other
>> wait
>> * values.
>> * @param events Event array for output
>> * @param num Maximum number of events to output
>> *
>> * @return Number of events outputed (0 ... num)
>> Number of events scheduled and stored in the events array?
>> */
>> int odp_schedule_multi(odp_queue_t *from, uint64_t wait, odp_event_t
>> events[],
>> unsigned int num);
>> Unsigned int for number of items, int used as return type (number of
>> items processed) but no errors are supposed to occur so negative range
>> is not used. Why then use int and not unsigned int for return type?
>
> True. Currently no failure specified, so it could return unsigned. However,
> to harmonize and to enable forward-compatible failure case: return int / int
> num?
Agree. Added to delta doc.
>
>
>>
>> So a lot of different models for basically two cases.
>> Case 1: Function writes data (size possibly not known in advance) to a
>> buffer. Caller must specify buffer size. Function must return number
>> of bytes actually written. And be able to return 1) output buffer too
>> small and 2) minimum required size of output buffer for successful
>> operation.
>
> I think, this 2) should be avoided for now and specify CONFIG_XXX_MAX macros
> instead. That avoids "int *len" output parameter or complex return value
> ranges (len for failure).
Agree. This concerns odp_cpumask_to_str() and odp_pktio_mac_addr()
(but not odp_hw_random_get() which falls into the class below). Added
to delta doc.
>
>>
>> Case 2: Caller specifies number of items for function to consume or
>> produce. Possibly that many items cannot be consumed (e.g. not room
>> enough in some ring buffer) or cannot be produced (not available at
>> source). Not all items have to be consumed or produced for success
>> (incremental success is allowed), indeed even 0 items consumed or
>> produced is a success, no error has occurred. Function must be able to
>> return number of items consumed/produced. Some functions must also be
>> able to return errors.
>
> This has two sub classes:
> 1) 0 consumed/produced is an error
> - Resource is full. User have to do something about it before a re-try may
> succeed.
Then the call can return a proper error code (e.g. -1 or some other
negative value). Not reuse a value which signify success in many other
places.
> 2) 0 is acceptable
> - Resource is currently busy, but re-try can succeed later on (without user
> doing anything else in between)
>
>
>>
>> For #1 I suggest:
>> ssize_t func(void *buf, ssize_t *bufsize);
>> @return >= 0 on success, number of bytes written (0 is OK when applicable)
>> @return <= on failure, if buffer too small (need specific return code
>> to indicate this?) then "*bufsize" is updated with the required size
>
> "Required size" not needed feature => output param not needed.
>
> ssize_t is problematic with sizeof().
Is is OK to pass "sizeof(something)" to a parameter of type ssize_t.
At least I was not able to get any warnings or errors using GCC or
clang (using -W and -Wall). The compiler might warn if the actual
value (which is a constant when using sizeof) does not fit into an
ssize_t variable. This is really unlikely when using sizeof().
>
>
> # if cannot fail or zero can indicate failure
> size_t func(void* buf, size_t len)
Yes but I don't want zero to indicate failure in some places and
success in other.
>
> # if can fail and zero cannot be the failure
> ssize_t func(void* buf, size_t len)
Or use ssize_t for both return type and input length.
>
>
>>
>> Alternatively if the function is not working with bytes or chars but
>> some other type of object, use "int" instead of "ssize_t".
>>
>> For #2 I suggest:
>> int func(odp_handle_t hdl, /const/ odp_object_t array[], int
>> num_elems_in_array);
>> @return >= 0 on success, number of elements consumed or produced (0 is
>> normally OK)
>> @return <0 on failure (when applicable, some functions may not
>> actually need to report failures but we can keep this for consistency)
>
> # This is OK. We just have to spec if 0 is success or failure.
IMO a return value of 0 always indicates success(at least from point
of the call itself).
For those calls were 0 "items" were processed and this actually means
failure, then the call should return <0 (e.g. -1) to indicate failure.
> int func(hdl, array[], int num_elems_in_array)
>
> -Petri
>
>
>>
>> Could there be exceptions to these design rules? Some function that
>> might need to operate on >=half the memory space as a consecutive
>> area? Not going to happen for 64-bit architectures. Plausible for
>> 32-bit architectures but how likely in practice?
>>
>> _______________________________________________
>> lng-odp mailing list
>> [email protected]
>> http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
