Re: Some libl4 patches

[Matthieu Lemerre]

Hi,


*To answer to Neal:

> Next, please supply a change log entry.  For an example of a change
> log entry, look in the ChangeLog files and in the GNU Coding
> Standards.  It is important that this is included.

Should I provide it in the patch or in a separate file (tell me what's
easier for you)

>
> Also, can you please fix your indentation to be consistent with
>ours.  For instance, in the last hunk of your patch, you have:
>
>       +  if(msg_buffer[1])
>
> This should be:
>
>       +  if (msg_buffer[1])
>
> Note the space.  All of this is covered in the GNU coding standards
> (and if you use emacs, tab helps a lot with indentation).
>

I (of course?) use emacs, and try to follow the GNU coding standards,
but it is true that I never took the habit to put a space between a
function name and its parenthesis (I wrote a function to do it for me,
but it was lost in a recent hard drive failure :().

I'll try to be more careful.

>
> A very minor nit: please use -p when generating diffs.  This makes
>them slightly easier to read.
>

OK. I didn't knew this option.

>
> Finally, for something a bit more positive: your explanation of what
>is wrong is excellent and a text like this should accompany all
>patches.
>

Thank you.


*To answer to Marcus:
>  
>> in ia32/vregs.h: There was some problem with Buffer register
>> loading/storing (basically, it didn't worked because the data were
>> processed backward)
>
> Yes.  The same problem exists in the powerpc port.
>  

Do you want me to include it in the patch too?

> So, you would have to check the first word of the string item.  This
> is still flawed, but in a more subtle way, as even a string buffer of
> length 0 is valid!  And thus you could not differentiate between no
> string buffer items (buffer[0] == 0) and a single simple string buffer
> item of length 0.

When I read the reference manual, I thought that:
 -For simple strings: even if the length is 0, you have to provide a pointer
 -For compound strings: as it is j-1 that is stored in the length word, (where 
j is
the number of substring pointers), we are obliged to have j>1

So in both case buffer[1] would be erased and we could for instance
check if string_item->stringptr is nil.

Anyway, your implementation is far better.

> What the official L4 library does is to set buffer[0] to 0 and
> buffer[2] to 0, and to set buffer[2] to ~0 if the first string item
> has been set.  The trick works because either there is no string
> item, then both are 0, or there is a single simple string item, then
> buffer[2] is 0, or there is a single compound string item, then
> buffer[0] is != 0 because of the "nr_substrings" field, or there is
> more than one string item, and then buffer[0] is != 0 because of the
> "cont" field.
>
> I think this trick is basically the only way to allow for all possible
> border cases, so we have to adopt it.  The algorithm is then:
>
> clear: buffer[0] = buffer[2] = 0;
>
> append: if (buffer[0] == 0 && buffer[2] == 0)
>           There is no string item yet.
>           Use first one.
>           if (this is simple string item)
>             buffer[2] = ~0;
>           else
>             Do nothing - buffer[0] is already guaranteed to be != 0.
>         else
>           Walk the string items until we find the buffer word after
>           the last one.
>           Use it, set previous cont field to 1.
>           At least now buffer[0] will be != 0 (if this was the second added).

OK, I'll write this.

>
>> I didn't checked, but I wonder if _L4_add_substring_to hasn't the same
>> problem.
>
> Could very well be.  Could you please check?
>

I'll check.

> It would be very worthwhile to write test cases for all these
> functions, that test for the silly border cases like adding a single
> empty string item etc.
>  

I could, but functions like store_brs don't have sense if not run on a
thread on top of l4...

So should I write test cases that would be "embedded" in a server?
If so, I will maybe provide a separate test file. (I already test my
patch with some printfs in bucket-manage-mt())

BTW, When in physmem, I manage to receive string items from deva, but
propagating the message to the worker thread seems to block when doing
the call. If you have a clue... otherwise I'll try to find out.

>> I also wonder if we shouldn't add a l4_string_item "creator" function
>> in the GNU interface to libl4; this function is useful (at least, it
>> was useful for me).
>
> Is this what you wrote about in your other mail?  I think I saw a mai
> by you that I marked in my inbox and didn't reply to yet.

As I said in the other thread, I will include it in the patch.

>
> I'd like you to make a new patch which we can apply that includes a
>fix for the powerpc port, addresses the minor nitpicks from Neal,
>implements the above solution and also fixes the other problematic
>functions which have the same problems.  Can you please do that?
>Otherwise it is already great that you found the problem, and fixing
>it wouldn't be too hard for me either, so just let us know if you
>want to keep going on it.  We certainly appreciate it.
>

I'll happily do it.

Thanks,
Matthieu