[mlmmj] [PATCH 00 of 10] mlmmj-maintd.c tweaks

[mlmmj] [PATCH 00 of 10] mlmmj-maintd.c tweaks

[Richard Mortimer]

All,

As mentioned the other day here are a number of patches against 
mlmmj-maintd.c that fix relatively minor issues. I haven't attempted 
to make any changes outside of this one file but intend to look at those 
over the coming month or two as time allows.

I have also spotted a couple of other issues that I haven't addressed yet. 
The first is the fact that unlink calls never have their error status 
checked. I was thinking about introducing a myunlink() method that does 
the unlink and reports any error. That would help catch any permissions
problems. There is a potential that some of the files might not exist
in all situations (the -probe files for example) so it might not be
appropriate to log an error in all circumstances.

Also there are a couple of places in mlmmj-maintd.c where it doesn't find 
the contents of the files that it expects. It just carries onto the next 
file without logging an error.

Review comments most welcome.

Regards

Richard

Re: [mlmmj] [PATCH 00 of 10] mlmmj-maintd.c tweaks

[Ben Schmidt]

On 28/01/11 9:59 AM, Richard Mortimer wrote:
> As mentioned the other day here are a number of patches against
> mlmmj-maintd.c that fix relatively minor issues. I haven't attempted
> to make any changes outside of this one file but intend to look at those
> over the coming month or two as time allows.

Thanks. I'll look at them when my time allows. I have a couple of high
Mlmmj priorities, so it may be a little while, I'm afraid.

> I have also spotted a couple of other issues that I haven't addressed yet.
> The first is the fact that unlink calls never have their error status
> checked.

Yes. This is a problem with unlink and a number of other system calls.
They need to be checked and errors logged appropriately.

Another big issue is that messages related to errno are often logged
when it shouldn't be, so it produces bogus error messages.

In fact, the whole error and logging parts of Mlmmj need a makeover.
This is one of my priorities after getting 1.2.18 out, and I would love
help with it. We need to design a small family of functions and/or
macros that provide a simpler, richer and more consistent interface for
performing logging and handling errors. Then we need to go through the
code and make existing logging use the new interfaces, and add logging
where it's missing for unchecked return codes and so on.

> I was thinking about introducing a myunlink() method that does
> the unlink and reports any error. That would help catch any permissions
> problems. There is a potential that some of the files might not exist
> in all situations (the -probe files for example) so it might not be
> appropriate to log an error in all circumstances.

Yeah, that's right. I think this needs to be done on a case-by-case
basis, not by adding just one myunlink() function.

> Also there are a couple of places in mlmmj-maintd.c where it doesn't find
> the contents of the files that it expects. It just carries onto the next
> file without logging an error.

Yep, of course this should be fixed too.

> Review comments most welcome.

I'll throw them your way when I have time. You may see your changesets
in Hg before or after I review them. We'll see.

Anyone else, of course, is also welcome to make comments. Please ensure
I am CCed if you do, guys.

Cheers,

Ben.

Re: [mlmmj] [PATCH 00 of 10] mlmmj-maintd.c tweaks

[Ben Schmidt]

Thanks again for this, Richard, and sorry for taking so long to get
around to looking at it. The work is great, and you made my life really
easy by writing clear patches and log messages, and ensuring each
changeset dealt with a single bug.

I have applied patches 01 and 04 through 10 as is, though I have removed
your email address from the author field of the changeset, just so that
it doesn't get exposed to the world unobfuscated through our hg web
interface, and in case it changes. One day I might keep a list of
contributors' contact details in a file in the distribution, but that
can wait.

Regarding patches 02 and 03, I love this work, but I will leave applying
it until we can do it throughout the codebase rather than just in
mlmmj-maintd. The 'wait for child' function should go in a separate file
and be linked into and called by all the executables that fork. If you'd
like to make a more comprehensive patch to do this, I would be very
grateful.

Thanks once again.

Smiles,

Ben.



On 28/01/11 9:59 AM, Richard Mortimer wrote:
> All,
>
> As mentioned the other day here are a number of patches against
> mlmmj-maintd.c that fix relatively minor issues. I haven't attempted
> to make any changes outside of this one file but intend to look at those
> over the coming month or two as time allows.
>
> I have also spotted a couple of other issues that I haven't addressed yet.
> The first is the fact that unlink calls never have their error status
> checked. I was thinking about introducing a myunlink() method that does
> the unlink and reports any error. That would help catch any permissions
> problems. There is a potential that some of the files might not exist
> in all situations (the -probe files for example) so it might not be
> appropriate to log an error in all circumstances.
>
> Also there are a couple of places in mlmmj-maintd.c where it doesn't find
> the contents of the files that it expects. It just carries onto the next
> file without logging an error.
>
> Review comments most welcome.
>
> Regards
>
> Richard
>
>
>

Re: [mlmmj] [PATCH 00 of 10] mlmmj-maintd.c tweaks

[Richard Mortimer]

Hi Ben,

On 17/01/2012 14:08, Ben Schmidt wrote:
> Thanks again for this, Richard, and sorry for taking so long to get
> around to looking at it. The work is great, and you made my life really
> easy by writing clear patches and log messages, and ensuring each
> changeset dealt with a single bug.
>
> I have applied patches 01 and 04 through 10 as is, though I have removed
> your email address from the author field of the changeset, just so that
> it doesn't get exposed to the world unobfuscated through our hg web
> interface, and in case it changes. One day I might keep a list of
> contributors' contact details in a file in the distribution, but that
> can wait.
Sure. Sounds good.

>
> Regarding patches 02 and 03, I love this work, but I will leave applying
> it until we can do it throughout the codebase rather than just in
> mlmmj-maintd. The 'wait for child' function should go in a separate file
> and be linked into and called by all the executables that fork.
Sure. That makes sense.

> If you'd
> like to make a more comprehensive patch to do this, I would be very
> grateful.
I'm pretty busy with other things right now but I'll keep it on my TODO 
list unless someone else beats me to it!

Regards

Richard


>
> Thanks once again.
>
> Smiles,
>
> Ben.
>
>
>
> On 28/01/11 9:59 AM, Richard Mortimer wrote:
>> All,
>>
>> As mentioned the other day here are a number of patches against
>> mlmmj-maintd.c that fix relatively minor issues. I haven't attempted
>> to make any changes outside of this one file but intend to look at those
>> over the coming month or two as time allows.
>>
>> I have also spotted a couple of other issues that I haven't addressed
>> yet.
>> The first is the fact that unlink calls never have their error status
>> checked. I was thinking about introducing a myunlink() method that does
>> the unlink and reports any error. That would help catch any permissions
>> problems. There is a potential that some of the files might not exist
>> in all situations (the -probe files for example) so it might not be
>> appropriate to log an error in all circumstances.
>>
>> Also there are a couple of places in mlmmj-maintd.c where it doesn't find
>> the contents of the files that it expects. It just carries onto the next
>> file without logging an error.
>>
>> Review comments most welcome.
>>
>> Regards
>>
>> Richard
>>
>>
>>