From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=35633 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PGux0-0008NO-2q
	for qemu-devel@nongnu.org; Fri, 12 Nov 2010 09:50:03 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1PGuwy-0006vB-Sk
	for qemu-devel@nongnu.org; Fri, 12 Nov 2010 09:50:01 -0500
Received: from mx1.redhat.com ([209.132.183.28]:22889)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1PGuwy-0006uP-Je
	for qemu-devel@nongnu.org; Fri, 12 Nov 2010 09:50:00 -0500
Date: Fri, 12 Nov 2010 12:49:54 -0200
From: Luiz Capitulino <lcapitulino@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver
Message-ID: <20101112124954.5c0b0aa4@doriath>
In-Reply-To: <m3iq02oce6.fsf@blackfin.pond.sub.org>
References: <1289503913-27413-1-git-send-email-lcapitulino@redhat.com>
	<1289503913-27413-2-git-send-email-lcapitulino@redhat.com>
	<m3vd42c056.fsf@blackfin.pond.sub.org>
	<20101112115740.3f5d33c7@doriath>
	<m3iq02oce6.fsf@blackfin.pond.sub.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Markus Armbruster <armbru@redhat.com>
Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, avi@redhat.com

On Fri, 12 Nov 2010 15:16:33 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 12 Nov 2010 11:21:57 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> [...]
> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> >> > +{
> >> > +    MemoryDriver *d = chr->opaque;
> >> > +
> >> > +    if (d->outbuf_size == 0) {
> >> > +        return qstring_new();
> >> > +    }
> >> 
> >> Why is this necessary?  Is qstring_from_substr() broken for empty
> >> substrings?  If it is, it ought to be fixed!
> >
> > qstring_from_substr() takes a character range; outbuf_size stores a size,
> > not a string length. So we do:
> >
> >> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> >
> > If outbuf_size is 0, we'll be passing a negative value down.
> 
> What's wrong with that?

Although it's going to work with the current QString implementation, I don't
think it's it's a good idea to rely on a negative index.

Maybe, we could have:

return qstring_from_substr((char *) d->outbuf, 0,
                            d->outbuf_size > 0 ? d->outbuf_size - 1 : 0);

A bit harder to read, but makes the function smaller.