Bug 102061 – [EFL] Fix build warning in NetworkStateNotifierEfl.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[KyungTae Kim]

To fix the below build warning:
 Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110:130: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

The second argument for NLMSG_OK need to be unsigned because it is compared with 'unsigned int'.

http://lxr.free-electrons.com/source/include/linux/netlink.h#L86
 41 struct nlmsghdr {
 42         __u32           nlmsg_len;      /* Length of message including header */
 43         __u16           nlmsg_type;     /* Message content */
 44         __u16           nlmsg_flags;    /* Additional flags */
 45         __u32           nlmsg_seq;      /* Sequence number */
 46         __u32           nlmsg_pid;      /* Sending process port ID */
 47 };

 86 #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
 87                            (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
 88                            (nlh)->nlmsg_len <= (len))

[KyungTae Kim]

Created attachment 173850 [details]
Patch

[Kangil Han]

Comment on attachment 173850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173850&action=review

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110
> +        while ((NLMSG_OK(nlh, (unsigned) len)) && (nlh->nlmsg_type != NLMSG_DONE)) {

len is ssize_t type and it includes -1 in its value range.
Why don't you insert some ASSERT stuff before here to make sure '-1' case is managed.

[KyungTae Kim]

Comment on attachment 173850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173850&action=review

>> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110
>> +        while ((NLMSG_OK(nlh, (unsigned) len)) && (nlh->nlmsg_type != NLMSG_DONE)) {
> 
> len is ssize_t type and it includes -1 in its value range.
> Why don't you insert some ASSERT stuff before here to make sure '-1' case is managed.

In here, len always >0 because the outer while check for that.
So, I didn't add additional code to check that for not making redundant code.

[Kangil Han]

Comment on attachment 173850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173850&action=review

>>> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110
>>> +        while ((NLMSG_OK(nlh, (unsigned) len)) && (nlh->nlmsg_type != NLMSG_DONE)) {
>> 
>> len is ssize_t type and it includes -1 in its value range.
>> Why don't you insert some ASSERT stuff before here to make sure '-1' case is managed.
> 
> In here, len always >0 because the outer while check for that.
> So, I didn't add additional code to check that for not making redundant code.

Ah, right!
LGTM, thanks!

[Laszlo Gombos]

Comment on attachment 173850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173850&action=review

> Source/WebCore/ChangeLog:8
> +        The second argument for NLMSG_OK need to be unsigned for fixing the -Wsign-compare warning.

need -> needs
for fixing --> to avoid

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110
> +        while ((NLMSG_OK(nlh, (unsigned) len)) && (nlh->nlmsg_type != NLMSG_DONE)) {

static_cast<unsigned>(len)

[KyungTae Kim]

Created attachment 174048 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174048 [details]
Patch

Clearing flags on attachment: 174048

Committed r134537: <http://trac.webkit.org/changeset/134537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 174048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174048&action=review

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110
> +        while ((NLMSG_OK(nlh, static_cast<unsigned>(len))) && (nlh->nlmsg_type != NLMSG_DONE)) {

If I check "man netlink" on my machine, I get:
int NLMSG_OK(struct nlmsghdr *nlh, int len);

It says that the second argument is signed.

[Chris Dumez]

(In reply to comment #9)
> (From update of attachment 174048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174048&action=review
> 
> > Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:110
> > +        while ((NLMSG_OK(nlh, static_cast<unsigned>(len))) && (nlh->nlmsg_type != NLMSG_DONE)) {
> 
> If I check "man netlink" on my machine, I get:
> int NLMSG_OK(struct nlmsghdr *nlh, int len);
> 
> It says that the second argument is signed.

Oh, I have just seen your first comment. the "man" page is misleading then. Clearly an issue with that macro but anyways.