[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Some libl4 patches
Re: Some libl4 patches
Thu, 06 Jan 2005 22:24:09 +0100
Gnus/5.1007 (Gnus v5.10.7) Emacs/21.3 (gnu/linux)
*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)
> This should be:
> + if (msg_buffer)
> 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
*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) 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
the number of substring pointers), we are obliged to have j>1
So in both case buffer 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 to 0 and
> buffer to 0, and to set buffer 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 is 0, or there is a single compound string item, then
> buffer is != 0 because of the "nr_substrings" field, or there is
> more than one string item, and then buffer 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 = buffer = 0;
> append: if (buffer == 0 && buffer == 0)
> There is no string item yet.
> Use first one.
> if (this is simple string item)
> buffer = ~0;
> Do nothing - buffer is already guaranteed to be != 0.
> 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 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
> Could very well be. Could you please 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.