Re: [PATCH 2/2] libblkid: Expose more ISO9660 headers

[Karel Zak <kzak@redhat.com>]

On Mon, Feb 11, 2013 at 11:22:12PM +0200, Zeeshan Ali (Khattak) wrote:
>  libblkid/src/superblocks/iso9660.c     | 112 +++++++++++++++++++++++++++++++--
>  libblkid/src/superblocks/superblocks.c |  40 ++++++++++++
>  libblkid/src/superblocks/superblocks.h |   5 +-
>  3 files changed, 151 insertions(+), 6 deletions(-)

 Applied.

> +#define strstrip(string, len) strchomp(strchug(string, len), len)

 I have removed this code, we already have blkid_rtrim_whitespace() so
 it seems better to add blkid_ltrim_whitespace() and use it together.

> +static char *
> +strchug (char *string, size_t len)
> +{
> +	size_t i;
> +
> +	for (i = 0;  i < len && isspace(string[i]); i++);
> +
> +	memmove (string, string + i, len - i);
> +
> +	return string;
> +}
> +
> +static char *
> +strchomp (char *string, size_t len)
> +{
> +	size_t i = len;
> +	while (i--) {
> +		if (isspace((unsigned char) string[i]))
> +			string[i] = '\0';
> +		else
> +			break;
> +}

 BTW, I didn't test it, but it seems that strstrip code will not work as expected
 for input like

    "    ABC "
 
 the result will be very probably

    "ABC ABC\0"

> +   if (! is_str_empty(iso->system_id, sizeof(iso->system_id))) {
> +		unsigned char system_id[32];
> +
> +		memcpy(system_id, iso->system_id, sizeof(system_id));
> +		strstrip(system_id, sizeof(system_id));
> +		blkid_probe_set_system_id(pr, system_id, sizeof(system_id));
> +	}

 the another change to the patch -- it seems better to add
 blkid_probe_set_id_label() and use for all the _IDs. Note that we
 have to terminate the labels by \0.

> +	/* Joliet Extension and Boot Record */
>  	off = ISO_VD_OFFSET;
>  	for (i = 0; i < ISO_VD_MAX; i++) {
> -		iso = (struct iso_volume_descriptor *)
> +		struct boot_record *boot= (struct boot_record *)
>  			blkid_probe_get_buffer(pr,
>  					off,
> -					sizeof(struct iso_volume_descriptor));
> +					sizeof(struct boot_record));

 This is bug, size of struct boot_record is smaller than the size of
 iso_volume_descriptor, so then you can not cast:

> +		/* Not a Boot record, lets see if its supplemntary volume descriptor */
> +		iso = (struct iso_volume_descriptor *) boot;

 ;-)

 Note that "blkid <image>" without -p uses high-level libblkid cache.
 For udev we always bypass the cache and the _ID stuff should not be
 stored in the cache. I have fixed this issue, you have to use -p to
 see your new IDs.

 Thanks, it's definitely good step to use libblkid and udev for the
 ISO stuff.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com