Re: New #mtools extfs?

[Standa Opichal <opichals seznam cz>]

Hi!

On Wed, 4 Dec 2002, Pavel Roskin wrote:

> 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...

> 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. 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.

> 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.

> 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?

> 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?

> > > What if we implement something like this:
> > > 
> > >   VFS name                mtools/dos name
> > > 
> > > /#dos:a/                  a:\
> > > /#dos:c/dos/sys.com       c:\dos\sys.com
> 
> Please quote only relevant parts of the messages you are replying to.  
> You didn't consider my suggestion and didn't even explain the reason.

I did, but as I've written for the second time (just look through my 
mails) it is not that extfs.c sends any attributes to the script along the 
'list' command...

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.

If you don't like to dig into the code then leave it to others so they 
can judge the patch effect. 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.

_best_ regards

STan