From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeremy White <jwhite@codeweavers.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
Date: Fri, 30 Nov 2012 14:36:47 +0100 [thread overview]
Message-ID: <50B8B66F.3090300@alum.mit.edu> (raw)
In-Reply-To: <7vboegp04x.fsf@alter.siamese.dyndns.org>
On 11/29/2012 10:30 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> struct msg_data stored (char *, len) of the data to be included in a
>
> That (<type>, <varname>) 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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
next prev parent reply other threads:[~2012-11-30 13:37 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-25 11:08 [PATCH 0/8] Add function strbuf_addstr_xml_quoted() and more Michael Haggerty
2012-11-25 11:08 ` [PATCH 1/8] Add new function strbuf_add_xml_quoted() Michael Haggerty
2012-11-25 11:08 ` [PATCH 2/8] xml_entities(): use function strbuf_addstr_xml_quoted() Michael Haggerty
2012-11-25 11:08 ` [PATCH 3/8] lf_to_crlf(): NUL-terminate msg_data::data Michael Haggerty
2012-11-25 11:08 ` [PATCH 4/8] imap-send: store all_msgs as a strbuf Michael Haggerty
2012-11-25 11:08 ` [PATCH 5/8] imap-send: correctly report errors reading from stdin Michael Haggerty
2012-11-25 11:08 ` [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf Michael Haggerty
2012-11-29 21:30 ` Junio C Hamano
2012-11-29 23:43 ` Jeff King
2012-11-30 13:36 ` Michael Haggerty [this message]
2012-12-02 1:48 ` Junio C Hamano
2012-12-02 6:03 ` Michael Haggerty
2012-12-03 15:06 ` Thiago Farina
2012-11-25 11:08 ` [PATCH 7/8] wrap_in_html(): use strbuf_addstr_xml_quoted() Michael Haggerty
2012-11-29 21:41 ` [PATCH 0/8] Add function strbuf_addstr_xml_quoted() and more Junio C Hamano
[not found] ` <1353841721-16269-9-git-send-email-mhagger@alum.mit.edu>
[not found] ` <7v7gp4p00u.fsf@alter.siamese.dyndns.org>
2012-11-30 13:40 ` [PATCH 8/8] wrap_in_html(): process message in bulk rather than line-by-line Michael Haggerty
2012-12-02 9:25 ` Junio C Hamano
2012-12-02 10:35 ` Michael Haggerty
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50B8B66F.3090300@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jwhite@codeweavers.com \
--cc=peff@peff.net \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).