Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
On Sat, Dec 1, 2012 at 11:48 PM, Junio C Hamano wrote: >> I suggest a separate patch series dedicated to deleting *all* the extra >> imap infrastructure at once. That being said, I'm not committing to do >> so. (We could add it to an "straightforward projects for aspiring git >> developers" list, if we had such a thing.) > > A "low-hanging fruit and/or janitorial work" stack may be worth > having. That would be good for not so versed developers, I think. Do we have a place for listing janitor projects? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
On 12/02/2012 02:48 AM, Junio C Hamano wrote: > Michael Haggerty writes: > >> On 11/29/2012 10:30 PM, Junio C Hamano wrote: >> A side effect of this change is that the memory for each message is freed after it is used rather than leaked, though that detail is unimportant given that imap-send is a top-level command. -- >>> >>> ? >> >> If by "?" you are wondering where the memory leak was, it was: > > No, I was wondering if you meant to say "---" to mark te remainder > of what you wrote does not exactly belong to the log message. Oh. Yes, that was my intention. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
Michael Haggerty writes: > On 11/29/2012 10:30 PM, Junio C Hamano wrote: > >>> A side effect of this change is that the memory for each message is >>> freed after it is used rather than leaked, though that detail is >>> unimportant given that imap-send is a top-level command. >>> >>> -- >> >> ? > > If by "?" you are wondering where the memory leak was, it was: No, I was wondering if you meant to say "---" to mark te remainder of what you wrote does not exactly belong to the log message. >>> For some reason, there is a bunch of infrastructure in this file for >>> dealing with IMAP flags, although there is nothing in the code that >>> actually allows any flags to be set. If there is no plan to add >>> support for flags in the future, a bunch of code could be ripped out >>> and "struct msg_data" could be completely replaced with strbuf. >> >> Yeah, after all these years we have kept the unused flags field >> there and nobody needed anything out of it. I am OK with a removal >> if it is done at the very end of the series. > > I don't think the removal of flags needs to be part of the same series. Oh, I did not think so, either. > I suggest a separate patch series dedicated to deleting *all* the extra > imap infrastructure at once. That being said, I'm not committing to do > so. (We could add it to an "straightforward projects for aspiring git > developers" list, if we had such a thing.) A "low-hanging fruit and/or janitorial work" stack may be worth having. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
On 11/29/2012 10:30 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> struct msg_data stored (char *, len) of the data to be included in a > > That (, ) is a bit funny notation, even though it is > understandable. I understand that it is funny, but it seems like the clearest way to express what is meant in a way that fits in the summary line. Feel free to change it if you like. >> message, kept the character data NUL-terminated, etc., much like a >> strbuf would do. So change it to use a struct strbuf. This makes the >> code clearer and reduces copying a little bit. >> >> A side effect of this change is that the memory for each message is >> freed after it is used rather than leaked, though that detail is >> unimportant given that imap-send is a top-level command. >> >> -- > > ? If by "?" you are wondering where the memory leak was, it was: * The while loop in main() called split_msg() * split_msg() cleared the msg_data structure using memset(msg, 0, sizeof *msg) * split_msg() copied the first message out of all_msgs using xmemdupz() and stored the result to msg->data * The msg_data was passed to imap_store_msg(). Its contents were copied to cb.data (which will be freed in the imap functions) but the original was left unfreed. * The next time through the loop, split_msg() zeroed the msg_data structure again, thus discarding the pointer to the xmemdupz()ed memory. The leak caused more memory than necessary to be allocated (worst case: nearly the total size of all_msgs). But (a) all_msgs is already stored in memory, so the wastage is at most a factor of 2; and (b) this all happens in main() shortly before program exit erases all sins. I didn't bother documenting this in the commit message because the patch changes the code anyway, but feel free to add the above explanation to the commit message if you think it is useful. >> For some reason, there is a bunch of infrastructure in this file for >> dealing with IMAP flags, although there is nothing in the code that >> actually allows any flags to be set. If there is no plan to add >> support for flags in the future, a bunch of code could be ripped out >> and "struct msg_data" could be completely replaced with strbuf. > > Yeah, after all these years we have kept the unused flags field > there and nobody needed anything out of it. I am OK with a removal > if it is done at the very end of the series. I don't think the removal of flags needs to be part of the same series. I suggest a separate patch series dedicated to deleting *all* the extra imap infrastructure at once. That being said, I'm not committing to do so. (We could add it to an "straightforward projects for aspiring git developers" list, if we had such a thing.) Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
On Thu, Nov 29, 2012 at 01:30:54PM -0800, Junio C Hamano wrote: > > For some reason, there is a bunch of infrastructure in this file for > > dealing with IMAP flags, although there is nothing in the code that > > actually allows any flags to be set. If there is no plan to add > > support for flags in the future, a bunch of code could be ripped out > > and "struct msg_data" could be completely replaced with strbuf. > > Yeah, after all these years we have kept the unused flags field > there and nobody needed anything out of it. I am OK with a removal > if it is done at the very end of the series. There's a bunch of unused junk in imap-send. The original implementation copied a bunch of code from isync, a much more full-featured imap client, and the result ended up way more complex than it needed to be. I have ripped a few things out over the years when they cause a problem (e.g., portability of /dev/urandom, conflict over the name "struct string_list"), but have mostly let it be out of a vague sense that we might one day want to pull bugfixes from isync upstream. That has not happened once in the last six years, though, and I would doubt that a straightforward merge would work after so many years. So ripping out and refactoring the code in the name of maintainability is probably a good thing at this point. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
Michael Haggerty writes: > struct msg_data stored (char *, len) of the data to be included in a That (, ) is a bit funny notation, even though it is understandable. > message, kept the character data NUL-terminated, etc., much like a > strbuf would do. So change it to use a struct strbuf. This makes the > code clearer and reduces copying a little bit. > > A side effect of this change is that the memory for each message is > freed after it is used rather than leaked, though that detail is > unimportant given that imap-send is a top-level command. > > -- ? > For some reason, there is a bunch of infrastructure in this file for > dealing with IMAP flags, although there is nothing in the code that > actually allows any flags to be set. If there is no plan to add > support for flags in the future, a bunch of code could be ripped out > and "struct msg_data" could be completely replaced with strbuf. Yeah, after all these years we have kept the unused flags field there and nobody needed anything out of it. I am OK with a removal if it is done at the very end of the series. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
struct msg_data stored (char *, len) of the data to be included in a
message, kept the character data NUL-terminated, etc., much like a
strbuf would do. So change it to use a struct strbuf. This makes the
code clearer and reduces copying a little bit.
A side effect of this change is that the memory for each message is
freed after it is used rather than leaked, though that detail is
unimportant given that imap-send is a top-level command.
--
For some reason, there is a bunch of infrastructure in this file for
dealing with IMAP flags, although there is nothing in the code that
actually allows any flags to be set. If there is no plan to add
support for flags in the future, a bunch of code could be ripped out
and "struct msg_data" could be completely replaced with strbuf.
Signed-off-by: Michael Haggerty
---
imap-send.c | 92 +++--
1 file changed, 47 insertions(+), 45 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 86cf603..a5e0e33 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -69,12 +69,7 @@ struct store {
};
struct msg_data {
- /* NUL-terminated data: */
- char *data;
-
- /* length of data (not including NUL): */
- int len;
-
+ struct strbuf data;
unsigned char flags;
};
@@ -1268,46 +1263,49 @@ static int imap_make_flags(int flags, char *buf)
return d;
}
-static void lf_to_crlf(struct msg_data *msg)
+static void lf_to_crlf(struct strbuf *msg)
{
+ size_t new_len;
char *new;
int i, j, lfnum = 0;
- if (msg->data[0] == '\n')
+ if (msg->buf[0] == '\n')
lfnum++;
for (i = 1; i < msg->len; i++) {
- if (msg->data[i - 1] != '\r' && msg->data[i] == '\n')
+ if (msg->buf[i - 1] != '\r' && msg->buf[i] == '\n')
lfnum++;
}
- new = xmalloc(msg->len + lfnum + 1);
- if (msg->data[0] == '\n') {
+ new_len = msg->len + lfnum;
+ new = xmalloc(new_len + 1);
+ if (msg->buf[0] == '\n') {
new[0] = '\r';
new[1] = '\n';
i = 1;
j = 2;
} else {
- new[0] = msg->data[0];
+ new[0] = msg->buf[0];
i = 1;
j = 1;
}
for ( ; i < msg->len; i++) {
- if (msg->data[i] != '\n') {
- new[j++] = msg->data[i];
+ if (msg->buf[i] != '\n') {
+ new[j++] = msg->buf[i];
continue;
}
- if (msg->data[i - 1] != '\r')
+ if (msg->buf[i - 1] != '\r')
new[j++] = '\r';
/* otherwise it already had CR before */
new[j++] = '\n';
}
- new[j] = '\0';
- msg->len += lfnum;
- free(msg->data);
- msg->data = new;
+ strbuf_attach(msg, new, new_len, new_len + 1);
}
-static int imap_store_msg(struct store *gctx, struct msg_data *data)
+/*
+ * Store msg to IMAP. Also detach and free the data from msg->data,
+ * leaving msg->data empty.
+ */
+static int imap_store_msg(struct store *gctx, struct msg_data *msg)
{
struct imap_store *ctx = (struct imap_store *)gctx;
struct imap *imap = ctx->imap;
@@ -1316,16 +1314,15 @@ static int imap_store_msg(struct store *gctx, struct
msg_data *data)
int ret, d;
char flagstr[128];
- lf_to_crlf(data);
+ lf_to_crlf(&msg->data);
memset(&cb, 0, sizeof(cb));
- cb.dlen = data->len;
- cb.data = xmalloc(cb.dlen);
- memcpy(cb.data, data->data, data->len);
+ cb.dlen = msg->data.len;
+ cb.data = strbuf_detach(&msg->data, NULL);
d = 0;
- if (data->flags) {
- d = imap_make_flags(data->flags, flagstr);
+ if (msg->flags) {
+ d = imap_make_flags(msg->flags, flagstr);
flagstr[d++] = ' ';
}
flagstr[d] = 0;
@@ -1356,7 +1353,8 @@ static void encode_html_chars(struct strbuf *p)
strbuf_splice(p, i, 1, """, 6);
}
}
-static void wrap_in_html(struct msg_data *msg)
+
+static void wrap_in_html(struct strbuf *msg)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf **lines;
@@ -1366,9 +1364,7 @@ static void wrap_in_html(struct msg_data *msg)
static char *pre_close = "\n";
int added_header = 0;
- strbuf_attach(&buf, msg->data, msg->len, msg->len);
- lines = strbuf_split(&buf, '\n');
- strbuf_release(&buf);
+ lines = strbuf_split(msg, '\n');
for (p = lines; *p; p++) {
if (! added_header) {
if ((*p)->len == 1 && *((*p)->buf) == '\n') {
@@ -1385,8 +1381,8 @@ static void wrap_in_html(struct msg_data *msg)
}
strbuf_addstr(&buf, pre_close);
strbuf_list_free(lines);
- msg->len = buf.len;
- msg-
