From: Rasmus Villemoes <linux@rasmusvillemoes.dk> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Trond Myklebust <trond.myklebust@primarydata.com>, "J. Bruce Fields" <bfields@fieldses.org>, "David S. Miller" <davem@davemloft.net>, linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Date: Sat, 31 Jan 2015 00:39:22 +0100 [thread overview] Message-ID: <87pp9vhm8l.fsf@rasmusvillemoes.dk> (raw) In-Reply-To: <1422613655.31903.351.camel@linux.intel.com> (Andy Shevchenko's message of "Fri, 30 Jan 2015 12:27:35 +0200") On Fri, Jan 30 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2015-01-29 at 15:29 +0100, Rasmus Villemoes wrote: >> On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> >> >> * >> >> * Return: >> >> - * The amount of the characters processed to the destination buffer, or >> >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is >> >> - * returned. >> >> - * >> >> - * Even in the case of error @dst pointer will be updated to point to the byte >> >> - * after the last processed character. >> >> + * The total size of the escaped output that would be generated for >> >> + * the given input and flags. To check whether the output was >> >> + * truncated, compare the return value to osz. There is room left in >> >> + * dst for a '\0' terminator if and only if ret < osz. >> >> */ >> >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> >> - unsigned int flags, const char *esc) >> >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, >> >> + unsigned int flags, const char *esc) >> > >> > I prefer to leave the prototype the same. int for return is okay. dst >> > should be updated accordingly. >> >> Please explain exactly what you think the return value should be, and >> what *dst should be set to. > > Return value like you proposed, *dst is incremented by it. The more I think about it, the less I like having dst being char**. (1) It is unlike most other functions taking buffer+size arguments. (2) It is inconvenient. Callers doing char escaped[SOME_SIZE]; or char *escaped = kmalloc(likely_big_enough); can't just pass &escaped; they would need to use an extra dummy variable. (3) With the return value telling the size of the would-be generated output, it is redundant. In fact, I dislike it so much that I'm not going to sign off on a patch where dst is still char**. >> > Could it be a part of nomem test still? >> >> What nomem test? string_escape_mem with snprintf-like semantics cannot >> return an error; that has to be checked by the caller. > > Make this code a separate test, which actually still nomem, since you > have not enough memory in the destination buffer. What the problem to > check for proper return value and the last couple of characters written > to the destination buffer? Sure, it could go into a separate helper. Anyway, the semantics of string_escape_mem needs to be decided before it makes sense to update the tests. > When your series will be ready (and actually I recommend to push first > patch apart from the rest since it's not related) I agree on that part. Andrew, could you take 1/3 (http://thread.gmane.org/gmane.linux.kernel/1876841/focus=348404), and then we'll see when the %pE case gets sorted out. Rasmus
WARNING: multiple messages have this Message-ID (diff)
From: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> To: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Trond Myklebust <trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>, "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>, "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Date: Sat, 31 Jan 2015 00:39:22 +0100 [thread overview] Message-ID: <87pp9vhm8l.fsf@rasmusvillemoes.dk> (raw) In-Reply-To: <1422613655.31903.351.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> (Andy Shevchenko's message of "Fri, 30 Jan 2015 12:27:35 +0200") On Fri, Jan 30 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: > On Thu, 2015-01-29 at 15:29 +0100, Rasmus Villemoes wrote: >> On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: >> >> >> * >> >> * Return: >> >> - * The amount of the characters processed to the destination buffer, or >> >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is >> >> - * returned. >> >> - * >> >> - * Even in the case of error @dst pointer will be updated to point to the byte >> >> - * after the last processed character. >> >> + * The total size of the escaped output that would be generated for >> >> + * the given input and flags. To check whether the output was >> >> + * truncated, compare the return value to osz. There is room left in >> >> + * dst for a '\0' terminator if and only if ret < osz. >> >> */ >> >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> >> - unsigned int flags, const char *esc) >> >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, >> >> + unsigned int flags, const char *esc) >> > >> > I prefer to leave the prototype the same. int for return is okay. dst >> > should be updated accordingly. >> >> Please explain exactly what you think the return value should be, and >> what *dst should be set to. > > Return value like you proposed, *dst is incremented by it. The more I think about it, the less I like having dst being char**. (1) It is unlike most other functions taking buffer+size arguments. (2) It is inconvenient. Callers doing char escaped[SOME_SIZE]; or char *escaped = kmalloc(likely_big_enough); can't just pass &escaped; they would need to use an extra dummy variable. (3) With the return value telling the size of the would-be generated output, it is redundant. In fact, I dislike it so much that I'm not going to sign off on a patch where dst is still char**. >> > Could it be a part of nomem test still? >> >> What nomem test? string_escape_mem with snprintf-like semantics cannot >> return an error; that has to be checked by the caller. > > Make this code a separate test, which actually still nomem, since you > have not enough memory in the destination buffer. What the problem to > check for proper return value and the last couple of characters written > to the destination buffer? Sure, it could go into a separate helper. Anyway, the semantics of string_escape_mem needs to be decided before it makes sense to update the tests. > When your series will be ready (and actually I recommend to push first > patch apart from the rest since it's not related) I agree on that part. Andrew, could you take 1/3 (http://thread.gmane.org/gmane.linux.kernel/1876841/focus=348404), and then we'll see when the %pE case gets sorted out. Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-01-30 23:39 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-01-28 13:25 [PATCH 0/2] Two printf fixes Rasmus Villemoes 2015-01-28 13:25 ` [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-01-28 14:53 ` Andy Shevchenko 2015-01-28 15:49 ` Rasmus Villemoes 2015-01-28 16:43 ` Andy Shevchenko 2015-01-28 13:25 ` [PATCH 2/2] string_helpers: Change semantics of string_escape_mem Rasmus Villemoes 2015-01-28 15:05 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 0/3] Two printf fixes Rasmus Villemoes 2015-01-29 10:03 ` [PATCH v2 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-01-29 10:43 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-01-29 12:12 ` Andy Shevchenko 2015-01-29 13:10 ` Rasmus Villemoes 2015-01-29 13:37 ` Andy Shevchenko 2015-01-29 19:33 ` Jeff Epler 2015-01-30 10:14 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-01-29 13:29 ` Andy Shevchenko 2015-01-29 14:29 ` Rasmus Villemoes 2015-01-30 10:27 ` Andy Shevchenko 2015-01-30 23:39 ` Rasmus Villemoes [this message] 2015-01-30 23:39 ` Rasmus Villemoes 2015-02-02 10:56 ` Andy Shevchenko 2015-02-09 23:44 ` [PATCH v3 0/3] Two printf fixes Rasmus Villemoes 2015-02-09 23:44 ` Rasmus Villemoes 2015-02-09 23:44 ` [PATCH v3 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-02-09 23:44 ` [PATCH v3 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-02-10 12:16 ` Andy Shevchenko 2015-02-09 23:44 ` [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-02-09 23:44 ` Rasmus Villemoes 2015-02-10 12:32 ` Andy Shevchenko 2015-02-10 12:32 ` Andy Shevchenko 2015-02-10 13:02 ` Rasmus Villemoes 2015-02-10 14:22 ` Andy Shevchenko 2015-02-10 14:22 ` Andy Shevchenko 2015-02-21 1:35 ` Rasmus Villemoes 2015-02-23 12:50 ` Andy Shevchenko 2015-02-23 12:50 ` Andy Shevchenko 2015-02-23 22:55 ` Rasmus Villemoes 2015-02-23 22:55 ` Rasmus Villemoes 2015-03-02 12:37 ` Andy Shevchenko 2015-03-02 12:37 ` Andy Shevchenko 2015-03-02 23:03 ` Rasmus Villemoes 2015-03-03 10:26 ` Andy Shevchenko 2015-03-03 10:26 ` Andy Shevchenko 2015-03-03 23:20 ` [PATCH v4 0/3] Two printf fixes Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-03-04 10:51 ` Andy Shevchenko 2015-03-03 23:20 ` [PATCH v4 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-03-04 11:49 ` Andy Shevchenko 2015-03-04 11:49 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87pp9vhm8l.fsf@rasmusvillemoes.dk \ --to=linux@rasmusvillemoes.dk \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=bfields@fieldses.org \ --cc=davem@davemloft.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=trond.myklebust@primarydata.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.