mlmmj-maintd.c tweaks
PATCH 1/1 mlmmj-maintd.c tweaks
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
Quoted 4 lines · Richard Mortimer
As mentioned the other day here are a number of patches against
mlmmj-maintd.cthat 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.
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 3 lines
Also there are a couple of places in
mlmmj-maintd.cwhere 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.
Quoted 1 line
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.
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.
Quoted 27 lines · Richard Mortimer
All,
As mentioned the other day here are a number of patches against
mlmmj-maintd.cthat 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.cwhere 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
Hi Ben,
Quoted 11 lines · 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.
Sure. Sounds good.
Quoted 5 lines
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.
Quoted 3 lines
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
Quoted 38 lines
Thanks once again.
Smiles,
Ben.
Quoted 28 lines · Richard Mortimer
All,
As mentioned the other day here are a number of patches against
mlmmj-maintd.cthat 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 amyunlink()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.cwhere 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