Re: New #mtools extfs?

[Pavel Roskin <proski gnu org>]

Hello!

> > I don't understand why you need to disable this check.  I don't think
> > it's right to reuse the archive filename as the disk name.  Actually,
> > what I suggested was "/#dos:a/" - this syntax doesn't need your patch.
> 
> Yes, it does although need quite more extensive patch that would send
> the part after colon to the extfs script. I'm telling you that this was
> the most least affecting patch I can think of...

There is another solution - make drive part of the filename:

/#dos:/a/

> > I don't like this coding style.  mystat is actually used later.  If
> > you ignore the return value of mc_stat, then
> > current_archive->extfsstat will point to invalid data.
> 
> No, it is _not_ a pointer it is only a structure that is copied into
> current_archive->extfsstat.

Then current_archive->extfsstat will still have random data.  Random data
is bad.  Sometimes is cannot be distinguished from valid data.

> Actually if the filesystem is marked so that it doesn't need an archive
> name (the 'a' mtools extfs you have in CVS)  than the
> current_archive->extfsstat would contain the same (possibly invalid)
> data as for the case of invalid (nonexistent) archive filename.

I was talking about style, not about "practical" side of the patch.  Even
if you give me a proof that random data will never affect anything, you
cannot prove that somebody else won't introduce a bug by checking that
data.

> > In fact, mystat is allocated on the stack, so
> > current_archive->extfsstat becomes invalid in any case as soon as
> > open_archive() returns.  current_archive is allocated by g_new() and
> > is returned by open_archive()  using the pparc argument.  I'll have to
> > debug mc to understand why it doesn't crash later.
> 
> Nope. It should not crash ... it is not a pointer ... look into the code
> more carefuly.

Right.  It's an implicit copy of the whole structure.  It's used very
rarely.  I figured it out as soon as I sent my previous e-mail.

> > The code is ugly already, let's not make is even worse.
> 
> I would like to not to make it worse. I just see this as very easy
> implementation where the archive stat() is not needed at all. It just
> prevents the extfs script to be called in case the archive name is not
> valid. But the script (or the apps called from in there) do these checks
> too, don't they?

Let's distinguish two things:

1) if stat() is not needed at all, then don't call it and remove all the
code that deals with that data.  Bigger patches are harder to understand,
but introducing dead variables of fields is not better.

2) if stat() is not needed only for mtools, then let's not call stat() for
mtools (and fill mystat with zeroes), although I would rather avoid this
special case.

> > I'm not going to apply it in this form.  Do it in the right way, i.e. by 
> > using flags after ":", like it's done in fish.
> 
> Hmm, so should I change the extfs.c so that it would send some extra 
> parameters into the script along the list command? Or what way would you 
> like it to be done?

Maybe using the disk as part of the path can make it unnecessary.  If this
approach doesn't work, then indeed we need to pass the flags, probably in
the environment.

> Actually, in the case of this patch (disable the stat() result check)  
> appliance, I can't see any reason to have the ':' flags in the extfs.ini
> configuration. If the archive does exist then this will be ok and the
> stat() information will be valid. Otherwise either the script would fail
> (e.g. on unzip -Z archiveName) because of that the archive doesn't exist
> or the script (like my #mtools) would use the archive name for something
> different or not use this at all. I see this as a matter of
> simplification rather than making code more unreadable. Sure I can
> comment the changes more to be more selfdescriptive.

OK, let's do the simplification first.  It's really worth it to clean up
the code before trying to fix anything non-trivial in that code.

> If you don't like to dig into the code then leave it to others so they 
> can judge the patch effect.

It's more a matter of time than preference.  I wish I could dig into the
recent stderr redirection problems right now, but it already 4am here, and
I do have a job (no, it's not hacking mc).

I'm trying to check the code, but sometimes the choice is whether I can
answer an e-mail at all.  I know that some people don't like to be
ignored.

> It seems to me that it is not me who did not play with the code. Please,
> take this only as a suggestions and thoughts that are emerging in my
> head. I appreciate your effort on mc... I intend _not_ to offense
> anyone, I just read the code and see this as a good, not harming,
> change.

That's fine.  If you care about mtools support, then please continue.  
Hacking mc is not easy because our "founding fathers" were not always 
careful to write clean code.  We can only say that we have learned from 
them if we don't repeat their errors.

-- 
Regards,
Pavel Roskin