[Pkg-xfce-devel] Bug#480209: Bug#480209: Bug#480209: Use HAL preferred mount point

[Yves-Alexis Perez]

On jeu, 2008-05-08 at 22:05 +0100, Simon Huggins wrote:
> On Thu, May 08, 2008 at 03:54:04PM -0300, Otavio Salvador wrote:
> > While working at a customization here at O.S. Systems, we came up with
> > the attached patch that adds support to exo to consult HAL about the
> > preferred mount point that could be defined using a policy.
> 
> > I think this is a nice improvement and would be nice to get it up to
> > upstream and add it there too if possible.
> 
> I agree that the thought is nice but I don't like some of the details of
> the implementation that change existing behaviour.
> 
> > Index: exo-0.3.4/exo-mount/exo-mount-hal.c
> > ===================================================================
> > --- exo-0.3.4.orig/exo-mount/exo-mount-hal.c	2008-05-08 15:34:02.000000000 -0300
> > +++ exo-0.3.4/exo-mount/exo-mount-hal.c	2008-05-08 15:34:26.000000000 -0300
> > @@ -708,13 +708,32 @@
> >          }
> >      }
> 
> > +  char *hal_mount_point;
> > +
> >    /* try to determine a usable mount point */
> >    if (G_LIKELY (device->volume != NULL))
> >      {
> > -      /* maybe we can use the volume's label... */
> > -      mount_point = (gchar *) libhal_volume_get_label (device->volume);
> 
> You remove this and change the logic never to try it.
> 
> > +      const char *drive_udi = libhal_volume_get_storage_device_udi (device->volume);
> 
> You create this string but never use this variable and then...
> 
> > +
> > +      if (device->drive != NULL)
> > +        {
> > +          hal_mount_point = libhal_device_get_property_string (hal_context, device->drive, "storage.policy.desired_mount_point", NULL);
> > +          if (hal_mount_point != NULL)
> > +            {
> > +              mount_point = g_strdup (hal_mount_point);
> > +              libhal_free_string(hal_mount_point);
> > +            }
> > +        }
> >      }
> > -  else
> > +
> > +    hal_mount_point = libhal_device_get_property_string (hal_context, libhal_volume_get_udi (device->volume), "volume.policy.desired_mount_point", NULL);
> 
> ... call libhal_volume_get_udi again here.
> 
> > +    if (hal_mount_point != NULL)
> > +      {
> > +        mount_point = g_strdup (hal_mount_point);
> > +        libhal_free_string(hal_mount_point);
> > +      }
> 
> You want to try libhal_volume_get_label stuff here I think.
> 
> > +
> > +  if (mount_point == NULL)
> >      {
> >        /* maybe we can use the the textual type... */
> >        mount_point = (gchar *) libhal_drive_get_type_textual (device->drive);
> 
> I think this patch needs to be improved before we consider sending it
> upstream.
> 
> At the moment it changes behaviour because it no longer looks at the
> volume label.  It's also getting close to lenny release for a patch that
> changes behaviour.
> 
> Can I suggest you clean it up and submit it upstream?

Did you made the cleaning and upstream submitting? I guess it could be
nice to have it included.


-- 
Yves-Alexis
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.alioth.debian.org/pipermail/pkg-xfce-devel/attachments/20091009/29a6d497/attachment.pgp>