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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
URL:
  <https://savannah.nongnu.org/patch/?9792>

                 Summary: mDNS: mdns_search_service mdns_search_stop: define
the request id as unsigned.
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: ourairquality
            Submitted on: Fri 12 Apr 2019 01:22:20 AM UTC
                Category: apps
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:





    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Fri 12 Apr 2019 01:22:20 AM UTC  Name:
0001-mdns-mdns_search_service-mdns_search_stop-define-the.patch  Size: 2KiB  
By: ourairquality

<http://savannah.nongnu.org/patch/download.php?file_id=46756>

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #1, patch #9792 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #2, patch #9792 (project lwip):

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/

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #3, patch #9792 (project lwip):

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().

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #4, patch #9792 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #5, patch #9792 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #6, patch #9792 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #7, patch #9792 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #8, patch #9792 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Follow-up Comment #9, patch #9792 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Wilfred
Update of patch #9792 (project lwip):

                  Status:                    None => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #10:

Applied, thanks!

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9792>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel