From patchwork Fri Nov 12 15:40:26 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luiz Capitulino X-Patchwork-Id: 70979 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 9A571B7140 for ; Sat, 13 Nov 2010 02:41:56 +1100 (EST) Received: from localhost ([127.0.0.1]:58814 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PGvlB-0001q5-Q0 for incoming@patchwork.ozlabs.org; Fri, 12 Nov 2010 10:41:53 -0500 Received: from [140.186.70.92] (port=42898 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PGvk2-0001nG-2g for qemu-devel@nongnu.org; Fri, 12 Nov 2010 10:40:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PGvjw-0000h9-2o for qemu-devel@nongnu.org; Fri, 12 Nov 2010 10:40:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:24921) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PGvjv-0000h1-Ph for qemu-devel@nongnu.org; Fri, 12 Nov 2010 10:40:36 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oACFeYQw016764 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 12 Nov 2010 10:40:34 -0500 Received: from doriath (ovpn-113-80.phx2.redhat.com [10.3.113.80]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id oACFeRcx029718 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 12 Nov 2010 10:40:32 -0500 Date: Fri, 12 Nov 2010 13:40:26 -0200 From: Luiz Capitulino To: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver Message-ID: <20101112134026.0d825aa4@doriath> In-Reply-To: References: <1289503913-27413-1-git-send-email-lcapitulino@redhat.com> <1289503913-27413-2-git-send-email-lcapitulino@redhat.com> <20101112115740.3f5d33c7@doriath> <20101112124954.5c0b0aa4@doriath> Organization: Red Hat Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, avi@redhat.com X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Fri, 12 Nov 2010 16:04:39 +0100 Markus Armbruster wrote: > Luiz Capitulino writes: > > > On Fri, 12 Nov 2010 15:16:33 +0100 > > Markus Armbruster wrote: > > > >> Luiz Capitulino writes: > >> > >> > On Fri, 12 Nov 2010 11:21:57 +0100 > >> > Markus Armbruster wrote: > >> > > >> >> Luiz Capitulino 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. > > How should I extract the substring of S beginning at index B with length > L? If I cant't do this for any B, L with interval [B,B+L-1] fully > within [0,length(S)], then the API is flawed, and ought to be replaced. Not sure we're talking about the same problem, anymore. When you said: > >> What's wrong with that? What did you mean? Did you mean 'let's not decrement outbuf_size' or did you mean 'let's pass -1 anyway'? Both seem wrong to me: the substring [0,-1] should be invalid and not decrementing outbuf_size is wrong, because it contains the buffer size and qstring_from_substr() will consume an additional char from the buffer (which should be '\0' today, but we shouldn't count on that). > > > 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. > > Err, doesn't qstring_from_substr(s, 0, 0) extract a substring of length > 1? Yeah, it's a bug. But that doesn't change my suggestion, can we do this way? This should fix the bug (not even compiled tested): diff --git a/qstring.c b/qstring.c index 4e2ba08..72a25de 100644 --- a/qstring.c +++ b/qstring.c @@ -42,10 +42,10 @@ QString *qstring_from_substr(const char *str, int start, int end) qstring = qemu_malloc(sizeof(*qstring)); - qstring->length = end - start + 1; - qstring->capacity = qstring->length; + qstring->length = end - start; + qstring->capacity = qstring->length + 1; - qstring->string = qemu_malloc(qstring->capacity + 1); + qstring->string = qemu_malloc(qstring->capacity); memcpy(qstring->string, str + start, qstring->length); qstring->string[qstring->length] = 0;