Message ID | 201212040346.qB43kVSQ018028@thirdoffive.cmf.nrl.navy.mil |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
firstly, sorry for reply late (yesterday I have an annual leave for some personal things). and thank you for your reply, too. 于 2012年12月04日 11:46, Chas Williams (CONTRACTOR) 写道: > static ssize_t show_address(struct device *cdev, > struct device_attribute *attr, char *buf) > { > - char *pos = buf; > struct atm_dev *adev = to_atm_dev(cdev); > - int i; > - > - for (i = 0; i < (ESI_LEN - 1); i++) > - pos += sprintf(pos, "%02x:", adev->esi[i]); > - pos += sprintf(pos, "%02x\n", adev->esi[i]); > > - return pos - buf; > + return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi); > } > "%p" seems print a pointer, not contents of pointer (is it correct ?) will it change the original display format to outside ? > static ssize_t show_atmaddress(struct device *cdev, > struct device_attribute *attr, char *buf) > { > unsigned long flags; > - char *pos = buf; > struct atm_dev *adev = to_atm_dev(cdev); > struct atm_dev_addr *aaddr; > int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin; > - int i, j; > + int i, j, count = 0; > > spin_lock_irqsave(&adev->lock, flags); > list_for_each_entry(aaddr, &adev->local, entry) { > for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) { > if (j == *fmt) { > - pos += sprintf(pos, "."); > + count += scnprintf(buf + count, > + PAGE_SIZE - count, "."); > ++fmt; > j = 0; > } > - pos += sprintf(pos, "%02x", > - aaddr->addr.sas_addr.prv[i]); > + count += scnprintf(buf + count, > + PAGE_SIZE - count, "%02x", > + aaddr->addr.sas_addr.prv[i]); > } > - pos += sprintf(pos, "\n"); > + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > } > spin_unlock_irqrestore(&adev->lock, flags); > > - return pos - buf; > + return count; > } > need we judge whether count >= PAGE_SIZE ? these are my suggestions, maybe not correct. :-) thanks.
In message <50BEA2CB.9000800@asianux.com>,Chen Gang writes: >> - for (i = 0; i < (ESI_LEN - 1); i++) >> - pos += sprintf(pos, "%02x:", adev->esi[i]); >> - pos += sprintf(pos, "%02x\n", adev->esi[i]); >> >> - return pos - buf; >> + return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi); >> } >> > > "%p" seems print a pointer, not contents of pointer (is it correct ?) > will it change the original display format to outside ? %pM means format this pointer as a mac address. it didnt exist when the atm stack was originally written but can be used now to save a bit of messy code. >> - pos += sprintf(pos, "\n"); >> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); ... > need we judge whether count >= PAGE_SIZE ? count will eventually make PAGE_SIZE - count reach 0 at which point, scnprintf() won't be able to write into the buffer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年12月05日 11:57, Chas Williams (CONTRACTOR) 写道: > In message <50BEA2CB.9000800@asianux.com>,Chen Gang writes: >>> - for (i = 0; i < (ESI_LEN - 1); i++) >>> - pos += sprintf(pos, "%02x:", adev->esi[i]); >>> - pos += sprintf(pos, "%02x\n", adev->esi[i]); >>> >>> - return pos - buf; >>> + return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi); >>> } >>> >> >> "%p" seems print a pointer, not contents of pointer (is it correct ?) >> will it change the original display format to outside ? > > %pM means format this pointer as a mac address. it didnt exist when the > atm stack was originally written but can be used now to save a bit of > messy code. > it is my fault. thank you :-) >>> - pos += sprintf(pos, "\n"); >>> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > .. >> need we judge whether count >= PAGE_SIZE ? > > count will eventually make PAGE_SIZE - count reach 0 at which point, > scnprintf() won't be able to write into the buffer. I also think so. I think, maybe it will be better to break the loop when we already know that "count >= PAGE_SIZE" (it can save waste looping, although it seems unlikly happen, for example, using unlikly(...) ). By the way: will it be better that always let "\n" at the end ? (if count == PAGE_SIZE in a loop, we can not let "\n" at the end). I think what I said above are minor, if you think, for this patch, do not need consider them, it is ok (at least for me, it is true). :-) > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
于 2012年12月05日 12:56, Chen Gang 写道: >>>> >>> - pos += sprintf(pos, "\n"); >>>> >>> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); >> > .. >>> >> need we judge whether count >= PAGE_SIZE ? >> > >> > count will eventually make PAGE_SIZE - count reach 0 at which point, >> > scnprintf() won't be able to write into the buffer. > I also think so. > > I think, maybe it will be better to break the loop when we already > know that "count >= PAGE_SIZE" (it can save waste looping, although it > seems unlikly happen, for example, using unlikly(...) ). > > By the way: > will it be better that always let "\n" at the end ? > (if count == PAGE_SIZE in a loop, we can not let "\n" at the end). oh, sorry ! count will never >= PAGE_SIZE. I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we can make the room for the end of "\n".
于 2012年12月05日 13:40, Chen Gang 写道: > 于 2012年12月05日 12:56, Chen Gang 写道: >>>>>>>> - pos += sprintf(pos, "\n"); >>>>>>>> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); >>>> .. >>>>>> need we judge whether count >= PAGE_SIZE ? >>>> >>>> count will eventually make PAGE_SIZE - count reach 0 at which point, >>>> scnprintf() won't be able to write into the buffer. >> I also think so. >> >> I think, maybe it will be better to break the loop when we already >> know that "count >= PAGE_SIZE" (it can save waste looping, although it >> seems unlikly happen, for example, using unlikly(...) ). >> >> By the way: >> will it be better that always let "\n" at the end ? >> (if count == PAGE_SIZE in a loop, we can not let "\n" at the end). > > oh, sorry ! count will never >= PAGE_SIZE. > > I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we > can make the room for the end of "\n". > > > sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2".
On Wed, 05 Dec 2012 13:59:26 +0800 Chen Gang <gang.chen@asianux.com> wrote: > 于 2012年12月05日 13:40, Chen Gang 写道: > > 于 2012年12月05日 12:56, Chen Gang 写道: > >>>>>>>> - pos += sprintf(pos, "\n"); > >>>>>>>> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > >>>> .. > >>>>>> need we judge whether count >= PAGE_SIZE ? > >>>> > >>>> count will eventually make PAGE_SIZE - count reach 0 at which point, > >>>> scnprintf() won't be able to write into the buffer. > >> I also think so. > >> > >> I think, maybe it will be better to break the loop when we already > >> know that "count >= PAGE_SIZE" (it can save waste looping, although it > >> seems unlikly happen, for example, using unlikly(...) ). it doesn't seem like optimizing for this corner case is a huge concern. the list cannot be infinitely long. > >> > >> By the way: > >> will it be better that always let "\n" at the end ? > >> (if count == PAGE_SIZE in a loop, we can not let "\n" at the end). > > > > oh, sorry ! count will never >= PAGE_SIZE. > > > > I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we > > can make the room for the end of "\n". > > > > > > > sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2". did you mean '\0' instead of '\n'? scnprintf() considers the trailing '\0' when formatting. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道: > On Wed, 05 Dec 2012 13:59:26 +0800 > > it doesn't seem like optimizing for this corner case is a huge > concern. the list cannot be infinitely long. > ok. >>>> >>>> By the way: >>>> will it be better that always let "\n" at the end ? >>>> (if count == PAGE_SIZE in a loop, we can not let "\n" at the end). >>> >>> oh, sorry ! count will never >= PAGE_SIZE. >>> >>> I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we >>> can make the room for the end of "\n". >>> >>> >>> >> sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2". > > did you mean '\0' instead of '\n'? scnprintf() considers the trailing > '\0' when formatting. no, originally, the end is "\n\0". I prefer we still compatible "\n" when the contents are very large. if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end. - pos += sprintf(pos, "\n"); + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Hi Chas Williams: all of my original reply are my idea (or suggestions), not for issues. if you do not need them, please help to send patch. I have tried to check it. at least, I did not find issues (and I also also learned from it) hope the patch can pass reviewers checking ! Good Luck ! :-) gchen. 于 2012年12月06日 09:15, Chen Gang 写道: > 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道: >> On Wed, 05 Dec 2012 13:59:26 +0800 >> >> it doesn't seem like optimizing for this corner case is a huge >> concern. the list cannot be infinitely long. >> > > ok. > > >>>>> >>>>> By the way: >>>>> will it be better that always let "\n" at the end ? >>>>> (if count == PAGE_SIZE in a loop, we can not let "\n" at the end). >>>> >>>> oh, sorry ! count will never >= PAGE_SIZE. >>>> >>>> I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we >>>> can make the room for the end of "\n". >>>> >>>> >>>> >>> sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2". >> >> did you mean '\0' instead of '\n'? scnprintf() considers the trailing >> '\0' when formatting. > > no, originally, the end is "\n\0". > > I prefer we still compatible "\n" when the contents are very large. > if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end. > > - pos += sprintf(pos, "\n"); > + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > > >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > >
On Thu, 06 Dec 2012 09:15:10 +0800 Chen Gang <gang.chen@asianux.com> wrote: > 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道: > > did you mean '\0' instead of '\n'? scnprintf() considers the trailing > > '\0' when formatting. > > no, originally, the end is "\n\0". > > I prefer we still compatible "\n" when the contents are very large. > if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end. > > - pos += sprintf(pos, "\n"); > + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); i would make the code a bit messy to do this for not much gain. again, it isnt likely you would run into this in a normal situation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年12月06日 22:08, chas williams - CONTRACTOR 写道: > On Thu, 06 Dec 2012 09:15:10 +0800 > Chen Gang <gang.chen@asianux.com> wrote: > >> 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道: > >>> did you mean '\0' instead of '\n'? scnprintf() considers the trailing >>> '\0' when formatting. >> >> no, originally, the end is "\n\0". >> >> I prefer we still compatible "\n" when the contents are very large. >> if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end. >> >> - pos += sprintf(pos, "\n"); >> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > > i would make the code a bit messy to do this for not much gain. again, > it isnt likely you would run into this in a normal situation. surely. thanks. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Hello Chas Williams: Excuse me: I am a reporter (not a reviewer), and not suitable to review another's patch. so: I suggest you to send your patch (as your willing) to the reviewers, directly. and need not cc to me (I am not a reviewer). thanks. gchen. 于 2012年12月07日 09:07, Chen Gang 写道: > 于 2012年12月06日 22:08, chas williams - CONTRACTOR 写道: >> On Thu, 06 Dec 2012 09:15:10 +0800 >> Chen Gang <gang.chen@asianux.com> wrote: >> >>> 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道: >> >>>> did you mean '\0' instead of '\n'? scnprintf() considers the trailing >>>> '\0' when formatting. >>> >>> no, originally, the end is "\n\0". >>> >>> I prefer we still compatible "\n" when the contents are very large. >>> if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end. >>> >>> - pos += sprintf(pos, "\n"); >>> + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); >> >> i would make the code a bit messy to do this for not much gain. again, >> it isnt likely you would run into this in a normal situation. > > surely. > > thanks. > >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > >
diff --git a/net/atm/atm_sysfs.c b/net/atm/atm_sysfs.c index f49da58..350bf62 100644 --- a/net/atm/atm_sysfs.c +++ b/net/atm/atm_sysfs.c @@ -14,49 +14,45 @@ static ssize_t show_type(struct device *cdev, struct device_attribute *attr, char *buf) { struct atm_dev *adev = to_atm_dev(cdev); - return sprintf(buf, "%s\n", adev->type); + + return scnprintf(buf, PAGE_SIZE, "%s\n", adev->type); } static ssize_t show_address(struct device *cdev, struct device_attribute *attr, char *buf) { - char *pos = buf; struct atm_dev *adev = to_atm_dev(cdev); - int i; - - for (i = 0; i < (ESI_LEN - 1); i++) - pos += sprintf(pos, "%02x:", adev->esi[i]); - pos += sprintf(pos, "%02x\n", adev->esi[i]); - return pos - buf; + return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi); } static ssize_t show_atmaddress(struct device *cdev, struct device_attribute *attr, char *buf) { unsigned long flags; - char *pos = buf; struct atm_dev *adev = to_atm_dev(cdev); struct atm_dev_addr *aaddr; int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin; - int i, j; + int i, j, count = 0; spin_lock_irqsave(&adev->lock, flags); list_for_each_entry(aaddr, &adev->local, entry) { for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) { if (j == *fmt) { - pos += sprintf(pos, "."); + count += scnprintf(buf + count, + PAGE_SIZE - count, "."); ++fmt; j = 0; } - pos += sprintf(pos, "%02x", - aaddr->addr.sas_addr.prv[i]); + count += scnprintf(buf + count, + PAGE_SIZE - count, "%02x", + aaddr->addr.sas_addr.prv[i]); } - pos += sprintf(pos, "\n"); + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); } spin_unlock_irqrestore(&adev->lock, flags); - return pos - buf; + return count; } static ssize_t show_atmindex(struct device *cdev, @@ -64,25 +60,21 @@ static ssize_t show_atmindex(struct device *cdev, { struct atm_dev *adev = to_atm_dev(cdev); - return sprintf(buf, "%d\n", adev->number); + return scnprintf(buf, PAGE_SIZE, "%d\n", adev->number); } static ssize_t show_carrier(struct device *cdev, struct device_attribute *attr, char *buf) { - char *pos = buf; struct atm_dev *adev = to_atm_dev(cdev); - pos += sprintf(pos, "%d\n", - adev->signal == ATM_PHY_SIG_LOST ? 0 : 1); - - return pos - buf; + return scnprintf(buf, PAGE_SIZE, "%d\n", + adev->signal == ATM_PHY_SIG_LOST ? 0 : 1); } static ssize_t show_link_rate(struct device *cdev, struct device_attribute *attr, char *buf) { - char *pos = buf; struct atm_dev *adev = to_atm_dev(cdev); int link_rate; @@ -100,9 +92,7 @@ static ssize_t show_link_rate(struct device *cdev, default: link_rate = adev->link_rate * 8 * 53; } - pos += sprintf(pos, "%d\n", link_rate); - - return pos - buf; + return scnprintf(buf, PAGE_SIZE, "%d\n", link_rate); } static DEVICE_ATTR(address, S_IRUGO, show_address, NULL);