Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 12 Nov 2010 15:16:33 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Fri, 12 Nov 2010 11:21:57 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Luiz Capitulino <address@hidden> 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.

> 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?