Re: [Nautilus-list] Need to add Removabel Media Support for Solaris on gnome CVS

[Alex Larsson <alexl redhat com>]

On Mon, 8 Apr 2002, Rajeev  Karale wrote:

> > +        if (path != NULL) {
> > +                mount_uri = nautilus_link_local_get_link_uri (path);
> > +                mount_path = gnome_vfs_get_local_path_from_uri
> (mount_uri);
> >
> > You need to check for mount_path != NULL before you use it, since the path
> > may not be local.
> 
> we will be doing all these activity only when volume is mounted.So I feel
> that mount_path check is not needed here.

How do you know it's always gonna be mounted? Do you trust the data in 
some random .desktop file to be what you want it to be? It could be put 
there by a bug in nautilus, some other app or a malicious person.
Always program defensively, always check for errors, and never crash due 
to unexpected values.

If mount_uri is non-local, for whatever reason, you will pass NULL to 
nautilus_volume_monitor_get_volume_for_path(), which will pass NULL to 
stat() and crash.

> > This part should be written more like:
> >
> > command = NULL;
> >
> > #ifdef HAVE_MEDIA_FORMAT_COMMAND
> > if (strcmp(verb, "Format Conditional)  == 0) {
> > command = g_strdup_printf (MEDIA_FORMAT_COMMAND, device_path);
> > }
> > #endif
> > #ifdef HAVE_MEDIA_PROPERTIES_COMMAND
> > if (strcmp(verb, "Media Properties Conditional)  == 0) {
> > command = g_strdup_printf (MEDIA_PROPERTIES_COMMAND, device_path);
> > }
> > #endif
> > #ifdef HAVE_MEDIA_PROTECT_COMMAND
> > if (strcmp(verb, "Protect Conditional)  == 0) {
> > command = g_strdup_printf (MEDIA_PROTECT_COMMAND, device_path);
> > }
> > #endif
> >
> > Then, if command != NULL it must fork off a process that runs command
> > instead of hanging nautilus waiting for it to finish. And you much check
> > the return value for popen().
> 
> Since media_callback itself is called within #ifdefs I don't think we need a
> #ifdefs here.
> I will do fork and do check popen return value.

You will need an ifdef because MEDIA_PROTECT_COMMAND will be undefined. Or 
you could handle that in the configure script. But is seems nicer (more 
explicit) to do in the code.

BTW. You should probably use g_spawn() to make the fork() + exec() code 
simpler. And it does the double fork thing so you won't have to reap the 
child.

Something like: 
char *argv[3]={"command", "-s", argument, NULL};
res = g_spawn_async (NULL, argv, NULL,
                     G_SPAWN_SEARCH_PATH,
                     NULL, NULL,
	             NULL, NULL);

For bonus points you would want to pass a GError and display a nice dialog 
on failure.

> > Why do you always hide unmount on solaris?
> 
> In solaris unmount volume is unmounting volume but not ejecting.

And why do you hide it? Since you never want to unmount a volume but not 
eject it? Whatever we decide is the best behaviour here, it certainly 
should not differ between architectures. That makes no sense.

> > Here you have both Eject and Unmount Volume again.
> >
> > +                        <menuitem name="Protect Media" _label="Protect"
> verb="Protect Conditional"/>
> > +                        <menuitem name="Format Media" _label="Format"
> verb="Format Conditional"/>
> > +                        <menuitem name="Media Properties Media"
> _label="Media Properties" verb="Media Properties Conditional"/>
> >
> > The naming of these, both name, label and verb, seems inconsistant. Why do
> > you have Media twice in properties, and why does only properties have
> > a "Media" in the label name?
> 
> I have removed extra "Media" in menuitem name.
> There is already  a menu item "show properties" added default with icon. So
> to avoid confusion, Properties was prefixed with Media to convey that,
> properties item is media specific.

Btw. Just so I know. Exactly what do these helper programs let you do? 
(Maybe we want Gnome versions of them too.)

Anyway, I'm awaiting the next version of the patch.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's an old-fashioned guitar-strumming paranormal investigator living 
undercover at Ringling Bros. Circus. She's a foxy hip-hop schoolgirl with a 
birthmark shaped like Liberty's torch. They fight crime!