Re: [PATCH] Updated fix for Debian Bug#574863: boot failure with lvm2 and snapshot volumes

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4443 bytes --]

On 06/03/2010 08:41 PM, sean finney wrote:
> and here is a slightly updated patch.  there is no functional change in the
> code, i have only reformatted the whitespace etc so that the code matches
> with the style of the surrounding code.
>
> i have also tested this now on my primary testing system and haven't
> noticed any problems.
>
>   
I've cleaned up your patch using my more generic string parsing
functions. Could you test attached patch?
> 	sean
>
> On Thu, Jun 03, 2010 at 12:41:32AM +0200, sean finney wrote:
>   
>> okay, I think the attached patch should fix the problem.
>>
>> I haven't tested it thoroughly, though my system does boot.  It seems
>> there may be a seperate issue with os-prober that results in some junk
>> entries being added to grub.conf if the snapshot volumes happen to be
>> root filesystems, but that probably needs to be taken up seperately and
>> the critical aspect of the bug is fixed anyway.
>>
>>
>> 	sean
>>     
>   
>> Author: Sean Finney <seanius@debian.org>
>> Description: Fix for lvm2 parsing failures with snapshot logical volumes
>>
>> This patch prevents the lvm2 parsing code from prematurely aborting
>> when encountering LV and segment metadata related to snapshot volumes.
>> Instead, the parser will now skip over these as if it never saw them,
>> which is probably the safest thing to do without a major injection of
>> lvm2 support code.
>>
>> Bug-Debian: #574863
>> --- disk/lvm.c	2010-04-27 15:25:12 +0000
>> +++ disk/lvm.c	2010-06-02 22:12:58 +0000
>> @@ -420,9 +420,11 @@
>>  	  /* And add all the lvs to the volume group. */
>>  	  while (1)
>>  	    {
>> -	      int s;
>> +	      int s, skip_lv = 0, status_visible = 0;
>>  	      struct grub_lvm_lv *lv;
>>  	      struct grub_lvm_segment *seg;
>> +	      char *status = NULL, *status_end = NULL;
>> +	      grub_size_t status_len = 0;
>>  
>>  	      while (grub_isspace (*p))
>>  		p++;
>> @@ -431,6 +433,8 @@
>>  		break;
>>  
>>  	      lv = grub_malloc (sizeof (*lv));
>> +		skip_lv = 0; /*Flag to skip snapshots */
>> +		status_visible = 0; /*Flag to skip non-visible LV's */
>>  
>>  	      q = p;
>>  	      while (*q != ' ')
>> @@ -445,6 +449,25 @@
>>  
>>  	      lv->size = 0;
>>  
>> +		/* read LV status and ignore ones not listed as "VISIBLE" */
>> +		p = grub_strstr (p, "status = ");
>> +		if (p == NULL)
>> +			goto lvs_fail;
>> +		status_end = grub_strchr(p, ']');
>> +		if (status_end == NULL)
>> +			goto lvs_fail;
>> +		status_len = (status_end - p) + 1;
>> +		status = grub_malloc(status_len + 1);
>> +		if (status == NULL)
>> +			goto lvs_fail;
>> +		grub_memcpy(status, p, status_len);
>> +		status[status_len] = '\0';
>> +		if (grub_strstr(status, "VISIBLE") != NULL)
>> +			status_visible = 1;
>> +		grub_free(status);
>> +		if (!status_visible)
>> +			goto lv_parsed;  /* don't bother parsing this one */
>> +
>>  	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
>>  	      if (p == NULL)
>>  		goto lvs_fail;
>> @@ -465,6 +488,18 @@
>>  		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
>>  		  if (p == NULL)
>>  		    goto lvs_segment_fail;
>> +
>> +			/* Skip LV's that have snapshot segments */
>> +			p = grub_strstr (p, "type = ");
>> +			if (p == NULL)
>> +				goto lvs_segment_fail;
>> +			p += sizeof("type = ") - 1;
>> +			if (!grub_strncmp(p, "\"snapshot\"", 10)) {
>> +				/* Found a snapshot, give up and move on */
>> +				skip_lv=1;
>> +				break;
>> +			}
>> +
>>  		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
>>  		  if (p == NULL)
>>  		    goto lvs_segment_fail;
>> @@ -531,12 +566,19 @@
>>  		  goto fail4;
>>  		}
>>  
>> +	    lv_parsed: /* all done with parsing this LV, seek to the end */
>>  	      if (p != NULL)
>>  		p = grub_strchr (p, '}');
>>  	      if (p == NULL)
>>  		goto lvs_fail;
>>  	      p += 3;
>>  
>> +		if (skip_lv || ! status_visible) {
>> +			grub_free (lv->name);
>> +			grub_free (lv);
>> +			continue;
>> +		}
>> +
>>  	      lv->number = lv_count++;
>>  	      lv->vg = vg;
>>  	      lv->next = vg->lvs;
>>
>>     
>
>
>
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: lvm.diff --]
[-- Type: text/x-diff; name="lvm.diff", Size: 2721 bytes --]

=== modified file 'disk/lvm.c'
--- disk/lvm.c	2010-04-27 15:25:12 +0000
+++ disk/lvm.c	2010-06-29 05:27:15 +0000
@@ -46,6 +46,52 @@ grub_lvm_getvalue (char **p, char *str)
 }
 
 static int
+grub_lvm_checkvalue (char **p, char *str, char *tmpl)
+{
+  int tmpllen = grub_strlen (tmpl);
+  *p = grub_strstr (*p, str);
+  if (! *p)
+    return 0;
+  *p += grub_strlen (str);
+  if (**p != '"')
+    return 0;
+  return (grub_memcmp (*p + 1, tmpl, tmpllen) == 0 && (*p)[tmpllen + 1] == '"');
+}
+
+static int
+grub_lvm_check_flag (char *p, char *str, char *flag)
+{
+  int len_str = grub_strlen (str), len_flag = grub_strlen (flag);
+  while (1)
+    {
+      char *q;
+      p = grub_strstr (p, str);
+      if (! p)
+	return 0;
+      p += len_str;
+      if (grub_memcmp (p, " = [", sizeof (" = [") - 1) != 0)
+	continue;
+      q = p + sizeof (" = [") - 1;
+      while (1)
+	{
+	  while (grub_isspace (*q))
+	    q++;
+	  if (*q != '"')
+	    return 0;
+	  q++;
+	  if (grub_memcmp (q, flag, len_flag) == 0 && q[len_flag] == '"')
+	    return 1;
+	  while (*q != '"')
+	    q++;
+	  q++;
+	  if (*q == ']')
+	    return 0;
+	  q++;
+	}
+    }
+}
+
+static int
 grub_lvm_iterate (int (*hook) (const char *name))
 {
   struct grub_lvm_vg *vg;
@@ -421,6 +467,7 @@ grub_lvm_scan_device (const char *name)
 	  while (1)
 	    {
 	      int s;
+	      int skip_lv = 0;
 	      struct grub_lvm_lv *lv;
 	      struct grub_lvm_segment *seg;
 
@@ -445,6 +492,12 @@ grub_lvm_scan_device (const char *name)
 
 	      lv->size = 0;
 
+	      if (!grub_lvm_check_flag (p, "status", "VISIBLE"))
+		{
+		  skip_lv = 1;
+		  goto lv_parsed;
+		}
+
 	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
 	      if (p == NULL)
 		goto lvs_fail;
@@ -465,6 +518,14 @@ grub_lvm_scan_device (const char *name)
 		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
+
+		  if (grub_lvm_checkvalue (&p, "type = ", "snapshot"))
+		    {
+		      /* Found a snapshot, give up and move on. */
+		      skip_lv = 1;
+		      break;
+		    }
+
 		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
@@ -531,12 +592,20 @@ grub_lvm_scan_device (const char *name)
 		  goto fail4;
 		}
 
+	    lv_parsed:
 	      if (p != NULL)
 		p = grub_strchr (p, '}');
 	      if (p == NULL)
 		goto lvs_fail;
 	      p += 3;
 
+	      if (skip_lv)
+		{
+		  grub_free (lv->name);
+		  grub_free (lv);
+		  continue;
+		}
+
 	      lv->number = lv_count++;
 	      lv->vg = vg;
 	      lv->next = vg->lvs;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]