patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9792, mDNS: mdns_search_service...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9792: mDNS: mdns_search_service mdns_search_stop: define the request id as unsigned.

Submitter:  Douglas <ourairquality>
Submitted:  Fri 12 Apr 2019 01:22:20 AM UTC
   
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Tue 30 Apr 2019 12:54:31 PM UTC, comment #10: 

Applied, thanks!

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 30 Apr 2019 12:53:03 PM UTC, comment #9: 

Argh, sorry!!! I missed your change to mdns_search_service - everything is fine!!

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 30 Apr 2019 12:51:34 PM UTC, comment #8: 

Sorry I don't understand why you claim that mdns_search_service returns a s8_t as the patch is as follows:

 mdns_search_service(const char *name, const char *service, enum mdns_sd_proto proto,
                     struct netif *netif, search_result_fn_t result_fn, void *arg,
-                    s8_t *request_id)
+                    u8_t *request_id)

                     ^^^
See the patch changes it to return a u8_t.

Really sorry if I am missing something, would not be be first time.

Douglas <ourairquality>
Tue 30 Apr 2019 12:41:18 PM UTC, comment #7: 

One function (mdns_search_service) returns an s8_t (via pointer) and the other function (mdns_search_stop) needs this value as u8_t - so you have to cast in application code. My point is: Change BOTH functions to u8_t or leave both as they are now.

Do you see a problem in changing mdns_search_service to u8_t, too?

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 30 Apr 2019 12:27:49 PM UTC, comment #6: 

The patch changes it to accept a pointer to a u8_t. There are no uses of mdns_search_service() in the lwip source code to patch, so it should give no compilation errors?

Obviously people using this will need to update their code, and that was one objection to this change. However it seems a recently added function, and is an optional feature, and appears to have not been in a released version yet, so why not fix it now. It's just a suggestion.

Douglas <ourairquality>
Tue 30 Apr 2019 12:11:06 PM UTC, comment #5: 

Yes, but it's a pointer to s8_t instead of u8_t.

So code would look like this:

s8_t request_id;
mdns_search_service(..., &request_id);

[...]

mdns_search_stop(request_id);

which produces a warning for mdns_search_stop() because of passing s8_t instead of u8_t.

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 30 Apr 2019 12:08:34 PM UTC, comment #4: 

Thank you for taking a look. Could you please double check, as mdns_search_service() appears to return the request ID via a pointer argument, not as the result value. It is the request ID that is passed back to mdns_search_stop(), and the suggestion is to make that an u8_t. The return value from mdns_search_service() can remain an err_t.

Douglas <ourairquality>
Tue 30 Apr 2019 10:45:52 AM UTC, comment #3: 

The change looks inconsistent to me - it would be OK if mdns_search_service() is changed to return u8_t, too.

With you patch, mdns_search_service() returns s8_t and this value needs to be passed as u8_t to mdns_search_stop().

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 23 Apr 2019 10:48:02 AM UTC, comment #2: 

The request ID is an unsigned integer so declaring it as such seems most appropriate (to me anyway). The search service functions are relatively new additions, and also an option, so it might be best to change this now - if that is the consensus.

There are pros and cons to some of these decisions. Some might consider it a clever encoding of an integer to have negative values be the error codes and positive values the index, and then to have functions that accept that ID check for an error value passed to them. On the other hand some might consider it cleaner to use two separate values and not conflate these, so that both the error code value and the ID index value can have separate types and that is my own general preference for a range of reasons.

Fwiw I did a good deal of rewriting of the mdns code, to malloc dynamic objecst rather than using large fix stack allocations - for the benefit of limited memory devices. The patches submitted back here are just some suggestions and not necessary. If people wish to see the full changes, which will never be accepted here as lwip can not depend on malloc, see https://github.com/ourairquality/lwip/

Douglas <ourairquality>
Tue 23 Apr 2019 09:01:17 AM UTC, comment #1: 

Well, this one will change API and require change in application too to avoid compilation error:


error: invalid conversion from 's8_t* {aka signed char*}' to 'u8_t* {aka unsigned char*}'



So, why changing this type?

David GIRAULT <dgirault>
Fri 12 Apr 2019 01:22:20 AM UTC, original submission:  


Douglas <ourairquality>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by dgirault (Posted a comment)
  • -email is unavailable- added by ourairquality (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-30 dziegel StatusNone Done
        Open/ClosedOpen Closed
    2019-04-12 ourairquality Attached File- Added 0001-mdns-mdns_search_service-mdns_search_stop-define-the.patch, #46756

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code