Karel Zak <kzak@xxxxxxxxxx> writes:
> Hi Matthias,
>
> On Wed, May 02, 2007 at 12:33:29PM +0200, Matthias Koenig wrote:
>>
>> this patch adds libvolume_id support and removes the old fs detection code.
>
> Thanks a lot! This is really important patch for the next release.
>
>> Since I had to clean up the original patch I decided to remove the old fs
>> detection code also in this patch.
>> I hope this does not bloat the patch too much.
>
> Well, I'm always more happy with bunch of small patches rather than with
> one huge patch :-)
Yes, I know. But in this case it seems to be more comfortable to do this in
one step. I promise to keep future patches small :-)
>
>> const char *
>> -mount_get_devname_by_uuid(const char *uuid) {
>> +mount_get_devname_by_uuid(const char *uuid)
>> +{
>
> hmm... very useful change :-)
When looking at different parts of mount, there seem to be different
coding styles applied. When touching this file I tried to adapt the style.
Though, we never discussed on the list about coding style, and maybe this
should be done in separate patches, but since I am changing the function
names anyway, I will keep this change in.
>
>> --- /dev/null
>> +++ util-linux-devel/mount/mount_udev.c
>
> why not mount_volumeid.c ? (especially if there is
> --with-libvolume_id)
Ok, yes this seems to be more appropriate.
>
>> + value = calloc(1, VOLUME_ID_LABEL_SIZE);
>> + if (!value)
>> + return NULL;
>
> Please, use routines from xmalloc.h (you can append the xcalloc).
Ok
>
>> +
>> + if (!spec)
>> + return NULL;
>> +
>> + while (volume_id_ptr->token && strcmp(volume_id_ptr->token,token))
>> + volume_id_ptr++;
>> +
>> + if (!volume_id_ptr->token) {
>> + free(value);
>> + value = NULL;
>> + goto out;
>> + }
>> +
>> + /* Quick exit if ID_FS_* variables are set */
>> + if ((var = getenv(volume_id_ptr->env))) {
>> + strncpy(value,var,VOLUME_ID_LABEL_SIZE - 1);
>> + goto out;
>> + }
>
> Who need this ID_FS_* feature? That's strange from my point of view
> and I'll feel better when this feature will be disabled for normal
> (UID!=0) users (just in case we really need this feature).
This is when mount is called via udev. udev already has detected the
filesystem type and propagates this information via the environment.
This is to prevent running the same fs detection code twice.
But yes, it should be disabled for normal users.
>
>> +void
>> +mount_blkid_get_cache(void) {}
>> +
>> +void
>> +mount_blkid_put_cache(void) {}
>
> The API should be generic and independent on blkid or volumenid.
Yes, as you mention it, I totally agree with this.
blkid and volume_id provide two implementations of the same interface.
So the interface definition should be more generic. I will clean this up.
>
> mount_volume_cache_init()
> mount_volume_cache_deinit()
>
> and also
>
> git mv mount_blkid.h mount_volume.h.
>
> (or something better -- I have a very limited imagination when I
> think about new names :-)
Well, I think volume is fine :-)
>
>> +const char *
>> +mount_get_devname_by_uuid(const char *uuid) {
>> + char *dev = NULL;
>> +
>> + if (!uuid)
>> + return NULL;
>> +
>> + dev = malloc(19 + strlen(uuid));
>> + strcpy(dev,"/dev/disk/by-uuid/");
>> + strcat(dev,uuid);
>> + }
>> +
>> + return dev;
>> +}
>> +
>> +const char *
>> +mount_get_devname_by_label(const char *label) {
>> + char *dev = NULL;
>> +
>> + if (!label)
>> + return NULL;
>> +
>> + dev = malloc(20 + strlen(label));
>> + if (dev) {
>> + strcpy(dev,"/dev/disk/by-label/");
>> + strcat(dev,label);
>> + }
>> +
>> + return dev;
>> +}
>
> 19, 20 .. magic numbers? ;-)
>
> #define DEV_BYLABEL_DIR "/dev/disk/by-label"
>
> and sizeof() seems like a better solution.
>
> Can't we canonicalize() the path to avoid /dev/disk/by-*
> paths in /etc/mtab?
>
> const char *
> mount_get_devname_by_label(const char *label) {
> char path[PATH_MAX];
>
> if (!label)
> return NULL;
>
> snprintf(path, sizeof(path), DEV_BYLABEL_DIR "/%s", label);
> return canonicalize(path);
> }
Yes you are right, we should canonicalize the name (or have this done
by the caller, but I think to return a canonicalized name is smarter).
>
>> -#include "mount_by_label.h"
>> +//#include "mount_by_label.h"
> ^^^
> Please, always /* */ ...
Oops, the // comments were not intended to stay. They were just for
testing and should have been deleted anyway.
Reworked patch will follow.
Matthias
-
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
