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

[huggie at earth.li (Simon Huggins)]

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?

If you want us to submit a better version then we can do so but are
more likely to after lenny.


Simon

-- 
... <benj[w0rK]> naoko: ca marche parfaitement ... quand on a une carte QUI
    FONCTIONNE !
    <benj[w0rK]> alors camembert :)