102061 2012-11-13 02:01:47 -0800 [EFL] Fix build warning in NetworkStateNotifierEfl.cpp 2012-11-13 22:12:22 -0800 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 101761 1 ktf.kim ktf.kim cdumez gyuyoung.kim lucas.de.marchi webkit.review.bot oldest_to_newest 765400 0 ktf.kim 2012-11-13 02:01:47 -0800 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)) 765404 1 173850 ktf.kim 2012-11-13 02:06:54 -0800 Created attachment 173850 Patch 765434 2 173850 kangil.han 2012-11-13 02:56:43 -0800 Comment on attachment 173850 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. 765436 3 173850 ktf.kim 2012-11-13 03:02:44 -0800 Comment on attachment 173850 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. 765439 4 173850 kangil.han 2012-11-13 03:08:03 -0800 Comment on attachment 173850 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! 766267 5 173850 laszlo.gombos 2012-11-13 18:07:39 -0800 Comment on attachment 173850 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) 766288 6 174048 ktf.kim 2012-11-13 18:32:27 -0800 Created attachment 174048 Patch 766391 7 174048 webkit.review.bot 2012-11-13 21:24:16 -0800 Comment on attachment 174048 Patch Clearing flags on attachment: 174048 Committed r134537: <http://trac.webkit.org/changeset/134537> 766392 8 webkit.review.bot 2012-11-13 21:24:20 -0800 All reviewed patches have been landed. Closing bug. 766426 9 174048 cdumez 2012-11-13 22:10:25 -0800 Comment on attachment 174048 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. 766427 10 cdumez 2012-11-13 22:12:22 -0800 (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. 173850 2012-11-13 02:06:54 -0800 2012-11-13 18:32:22 -0800 Patch bug-102061-20121113190455.patch text/plain 1780 ktf.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0MzY1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjEzMzY5NjIwNmI1MzFk Mjk1MThmNTE2YjQwOTA2MWM4YzZkZjdlOC4uNzViNDhkYTU5MTZkZDBhMjQwMGE2OThhZWFjMGNh MWI1ODU2MmRhMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTExLTEzICBLeXVu Z1RhZSBLaW0gIDxrdGYua2ltQHNhbXN1bmcuY29tPgorCisgICAgICAgIFtFRkxdIEZpeCBidWls ZCB3YXJuaW5nIGluIE5ldHdvcmtTdGF0ZU5vdGlmaWVyRWZsLmNwcAorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyMDYxCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIHNlY29uZCBhcmd1bWVudCBmb3Ig TkxNU0dfT0sgbmVlZCB0byBiZSB1bnNpZ25lZCBmb3IgZml4aW5nIHRoZSAtV3NpZ24tY29tcGFy ZSB3YXJuaW5nLgorCisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9lZmwvTmV0d29ya1N0YXRl Tm90aWZpZXJFZmwuY3BwOgorICAgICAgICAoV2ViQ29yZTo6cmVhZFNvY2tldENhbGxiYWNrKToK KwogMjAxMi0xMS0xMiAgSm9lIE1hc29uICA8am1hc29uQHJpbS5jb20+CiAKICAgICAgICAgW0Js YWNrQmVycnldIE5ldHdvcmtKb2Igc2hvdWxkIG5vdCBjaGVjayBpZiBkYXRhIGlzIHJlY2VpdmVk IHdpdGggSEVBRApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9l ZmwvTmV0d29ya1N0YXRlTm90aWZpZXJFZmwuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v bmV0d29yay9lZmwvTmV0d29ya1N0YXRlTm90aWZpZXJFZmwuY3BwCmluZGV4IGEwODQ4OTAwN2Mz ZmE5MTMyZWQzYTZiYzk2OGNiODRiN2VmZTQ3YWEuLjdkYTFmYWEzY2RkMDMwMGY3NDhlZjJjNGYy ZTY5MDViYjQ0NjRiYjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdv cmsvZWZsL05ldHdvcmtTdGF0ZU5vdGlmaWVyRWZsLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9uZXR3b3JrL2VmbC9OZXR3b3JrU3RhdGVOb3RpZmllckVmbC5jcHAKQEAgLTEwNyw3 ICsxMDcsNyBAQCBzdGF0aWMgRWluYV9Cb29sIHJlYWRTb2NrZXRDYWxsYmFjayh2b2lkKiB1c2Vy RGF0YSwgRWNvcmVfRmRfSGFuZGxlciogaGFuZGxlcikKICAgICBzc2l6ZV90IGxlbjsKICAgICBu bG1zZ2hkciogbmxoID0gcmVpbnRlcnByZXRfY2FzdDxubG1zZ2hkcio+KGJ1ZmZlcik7CiAgICAg d2hpbGUgKChsZW4gPSByZWN2KHNvY2ssIG5saCwgYnVmZmVyU2l6ZSwgTVNHX0RPTlRXQUlUKSkg PiAwKSB7Ci0gICAgICAgIHdoaWxlICgoTkxNU0dfT0sobmxoLCBsZW4pKSAmJiAobmxoLT5ubG1z Z190eXBlICE9IE5MTVNHX0RPTkUpKSB7CisgICAgICAgIHdoaWxlICgoTkxNU0dfT0sobmxoLCAo dW5zaWduZWQpIGxlbikpICYmIChubGgtPm5sbXNnX3R5cGUgIT0gTkxNU0dfRE9ORSkpIHsKICAg ICAgICAgICAgIGlmIChubGgtPm5sbXNnX3R5cGUgPT0gTkxNU0dfRVJST1IpIHsKICAgICAgICAg ICAgICAgICBMT0dfRVJST1IoIkVycm9yIHdoaWxlIHJlYWRpbmcgc29ja2V0LCBzdG9wIG1vbml0 b3Jpbmcgb25MaW5lIHN0YXRlLiIpOwogICAgICAgICAgICAgICAgIHJldHVybiBFQ09SRV9DQUxM QkFDS19DQU5DRUw7Cg== 174048 2012-11-13 18:32:27 -0800 2012-11-13 22:10:25 -0800 Patch bug-102061-20121114113028.patch text/plain 1853 ktf.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0NTE1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYmMzOWY3ZDUyYzgzZGQw MmU0NjExMDcxN2U0YjRmMTI0ZDI5M2U0MC4uMGNmNDg0OTFkZDgzNzU3YmM0NmU2MTFkODhmMzhm MTU5ODRmYTA5NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTExLTEzICBLeXVu Z1RhZSBLaW0gIDxrdGYua2ltQHNhbXN1bmcuY29tPgorCisgICAgICAgIFtFRkxdIEZpeCBidWls ZCB3YXJuaW5nIGluIE5ldHdvcmtTdGF0ZU5vdGlmaWVyRWZsLmNwcAorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyMDYxCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIHNlY29uZCBhcmd1bWVudCBmb3Ig TkxNU0dfT0sgbmVlZHMgdG8gYmUgdW5zaWduZWQgdG8gYXZvaWQgdGhlIC1Xc2lnbi1jb21wYXJl IHdhcm5pbmcuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL2VmbC9OZXR3b3JrU3RhdGVO b3RpZmllckVmbC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpyZWFkU29ja2V0Q2FsbGJhY2spOgor CiAyMDEyLTExLTEzICBLZW5pY2hpIElzaGliYXNoaSAgPGJhc2hpQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBbV2ViU29ja2V0XSBzZW5kKCkgYW5kIGNsb3NlKCkgc2hvdWxkIG5vdCB0aHJvdyBh biBleGNlcHRpb24gZm9yIGFuIHVucGFpcmVkIHN1cnJvZ2F0ZSBidXQgdXNlIHRoZSByZXBsYWNl bWVudCBjaGFyYWN0ZXIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdv cmsvZWZsL05ldHdvcmtTdGF0ZU5vdGlmaWVyRWZsLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL25ldHdvcmsvZWZsL05ldHdvcmtTdGF0ZU5vdGlmaWVyRWZsLmNwcAppbmRleCBhMDg0ODkw MDdjM2ZhOTEzMmVkM2E2YmM5NjhjYjg0YjdlZmU0N2FhLi5kMGQ3YjlmYzkwMjJhM2NlMGQxYjlm NTY3NzMzYmI1NDY2NTYyNTU1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9u ZXR3b3JrL2VmbC9OZXR3b3JrU3RhdGVOb3RpZmllckVmbC5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vbmV0d29yay9lZmwvTmV0d29ya1N0YXRlTm90aWZpZXJFZmwuY3BwCkBAIC0x MDcsNyArMTA3LDcgQEAgc3RhdGljIEVpbmFfQm9vbCByZWFkU29ja2V0Q2FsbGJhY2sodm9pZCog dXNlckRhdGEsIEVjb3JlX0ZkX0hhbmRsZXIqIGhhbmRsZXIpCiAgICAgc3NpemVfdCBsZW47CiAg ICAgbmxtc2doZHIqIG5saCA9IHJlaW50ZXJwcmV0X2Nhc3Q8bmxtc2doZHIqPihidWZmZXIpOwog ICAgIHdoaWxlICgobGVuID0gcmVjdihzb2NrLCBubGgsIGJ1ZmZlclNpemUsIE1TR19ET05UV0FJ VCkpID4gMCkgewotICAgICAgICB3aGlsZSAoKE5MTVNHX09LKG5saCwgbGVuKSkgJiYgKG5saC0+ bmxtc2dfdHlwZSAhPSBOTE1TR19ET05FKSkgeworICAgICAgICB3aGlsZSAoKE5MTVNHX09LKG5s aCwgc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KGxlbikpKSAmJiAobmxoLT5ubG1zZ190eXBlICE9IE5M TVNHX0RPTkUpKSB7CiAgICAgICAgICAgICBpZiAobmxoLT5ubG1zZ190eXBlID09IE5MTVNHX0VS Uk9SKSB7CiAgICAgICAgICAgICAgICAgTE9HX0VSUk9SKCJFcnJvciB3aGlsZSByZWFkaW5nIHNv Y2tldCwgc3RvcCBtb25pdG9yaW5nIG9uTGluZSBzdGF0ZS4iKTsKICAgICAgICAgICAgICAgICBy ZXR1cm4gRUNPUkVfQ0FMTEJBQ0tfQ0FOQ0VMOwo=