[RAUC] Failed to mount seed slot

[RAUC] Failed to mount seed slot

[Ladislav Michl]

Hi there,

I'm using rauc with casync to update target system - it is symmetric rootfsA,
rootfsB, scenario, so slots are defined as:
[slot.rootfs.0]
device=/dev/ubi0_0
type=ubifs
bootname=system0

[slot.rootfs.1]
device=/dev/ubi0_1
type=ubifs
bootname=system1

Now system is booted with rootfs mounted read only which makes mounting seed
slot fail:
rauc[871]: Mounting /dev/ubi0_0 to use as seed
rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.

Here adding '-o ro' would quick fix it up (but break rw case, as options have to
match), so it seems better to use bind mount as comment casync_extract_image
function suggests, but do not see it code.

Am I missing anything?

Thank you,
	ladis

_______________________________________________
RAUC mailing list

Re: [RAUC] Failed to mount seed slot

[Ladislav Michl]

On Mon, May 06, 2019 at 07:44:09PM +0200, Ladislav Michl wrote:
> Hi there,
> 
> I'm using rauc with casync to update target system - it is symmetric rootfsA,
> rootfsB, scenario, so slots are defined as:
> [slot.rootfs.0]
> device=/dev/ubi0_0
> type=ubifs
> bootname=system0
> 
> [slot.rootfs.1]
> device=/dev/ubi0_1
> type=ubifs
> bootname=system1
> 
> Now system is booted with rootfs mounted read only which makes mounting seed
> slot fail:
> rauc[871]: Mounting /dev/ubi0_0 to use as seed
> rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
> rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
> rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.
> 
> Here adding '-o ro' would quick fix it up (but break rw case, as options have to
> match), so it seems better to use bind mount as comment casync_extract_image
> function suggests, but do not see it code.
> 
> Am I missing anything?

So, whats still missing is a review of pull request #406 (Normalize device names
to find mounted slots) :)

Then determine_slot_states should properly fill slot's ext_mount_point and
casync_extract_image would then use it to bind mount seed slot.

And here's the tricky part. For device-less mount, volume to mount is specified
for example using ubiX_Y or ubiX:NAME syntax, so fix in #406 will fail in this
case.

A workaround is to add support to bootloader which will try to guess UBI
volume character device as seen by kernel.

Now it is time to make design decision: if rauc is going to suppport device-less
mounts this should be fixed (read: I'll provide a patch for review). Otherwise
I'll change a way bootloader passes root= option.

Thank you,
	ladis

_______________________________________________
RAUC mailing list

Re: [RAUC] Failed to mount seed slot

[Ladislav Michl]

On Tue, May 07, 2019 at 09:31:28AM +0200, Ladislav Michl wrote:
> On Mon, May 06, 2019 at 07:44:09PM +0200, Ladislav Michl wrote:
> > Hi there,
> > 
> > I'm using rauc with casync to update target system - it is symmetric rootfsA,
> > rootfsB, scenario, so slots are defined as:
> > [slot.rootfs.0]
> > device=/dev/ubi0_0
> > type=ubifs
> > bootname=system0
> > 
> > [slot.rootfs.1]
> > device=/dev/ubi0_1
> > type=ubifs
> > bootname=system1
> > 
> > Now system is booted with rootfs mounted read only which makes mounting seed
> > slot fail:
> > rauc[871]: Mounting /dev/ubi0_0 to use as seed
> > rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
> > rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
> > rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.
> > 
> > Here adding '-o ro' would quick fix it up (but break rw case, as options have to
> > match), so it seems better to use bind mount as comment casync_extract_image
> > function suggests, but do not see it code.
> > 
> > Am I missing anything?
> 
> So, whats still missing is a review of pull request #406 (Normalize device names
> to find mounted slots) :)

It feels really strange talking to myself, so let's hope someone else joins as
well, hence Cc list.

Problem is similar to that one solved by #406 and #388, however it needs to be
extended to ubi<X> and possibly mtd<X> case. For now let's start with fixing
pull request #406 to:
- use C comments
- avoid unneded allocations
- also allow character devices (for example /dev/ubi0_0)

> Then determine_slot_states should properly fill slot's ext_mount_point and
> casync_extract_image would then use it to bind mount seed slot.
> 
> And here's the tricky part. For device-less mount, volume to mount is specified
> for example using ubiX_Y or ubiX:NAME syntax, so fix in #406 will fail in this
> case.

This one was fixed by extending normalize_mountable_object from #406 and
adjusting casync_extract_image, but lets first get #406 revieved and merged :)
Trent, in case you want to merge following additional patch to your PR here's my
Signed-off-by: Ladislav Michl <ladis@linux-mips.org>

diff --git a/src/config_file.c b/src/config_file.c
index 875e0ee..e592d15 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -494,27 +494,25 @@ free:
 	return res;
 }
 
-// Something that is, or can be, mounted onto a mount point
+/* Something that is, or can be, mounted onto a mount point */
 typedef struct {
-	gboolean is_device;  // vs being a loop mounted file
-	dev_t dev;  // the device, or for a file, the device the file is on
-	ino_t inode;  // inode of file for a non-device
+	gboolean is_device;	/* vs being a loop mounted file */
+	dev_t dev;		/* the device, or for a file, the device the file is on */
+	ino_t inode;		/* inode of file for a non-device */
 } MountableObj;
 
-// Take a device (or file) path and normalize it
-static MountableObj *normalize_mountable_object(const gchar *devicepath)
+/* Take a device (or file) path or name and normalize it */
+static gboolean normalize_mountable_object(const gchar *devicename, MountableObj *obj)
 {
-	MountableObj *obj;
 	GStatBuf st;
 
-	if (g_stat(devicepath, &st) == -1) {
+	if (g_stat(devicename, &st) == -1) {
 		/* Virtual filesystems like devpts trigger case */
-		g_debug("Can't stat '%s', assuming unmountable: %s", devicepath, g_strerror(errno));
-		return NULL;
+		g_debug("Can't stat '%s', assuming unmountable: %s", devicename, g_strerror(errno));
+		return FALSE;
 	}
 
-	obj = g_new0(MountableObj, 1);
-	if (S_ISBLK(st.st_mode)) {
+	if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
 		obj->is_device = TRUE;
 		obj->dev = st.st_rdev;
 	} else if (S_ISREG(st.st_mode)) {
@@ -522,10 +520,10 @@ static MountableObj *normalize_mountable_object(const gchar *devicepath)
 		obj->dev = st.st_dev;
 		obj->inode = st.st_ino;
 	} else {
-		g_debug("Device '%s' is not something which is mountable", devicepath);
-		g_clear_pointer(&obj, g_free);
+		g_debug("Device '%s' is not something which is mountable", devicename);
+		return FALSE;
 	}
-	return obj;
+	return TRUE;
 }
 
 /* Compare two MountableObj for equality */
@@ -546,24 +544,24 @@ RaucSlot *find_slot_by_device(GHashTable *slots, const gchar *device)
 {
 	GHashTableIter iter;
 	RaucSlot *slot;
-	g_autofree MountableObj *obj = NULL;
+	MountableObj obj;
+	gboolean normalized;
 
 	g_return_val_if_fail(slots, NULL);
 	g_return_val_if_fail(device, NULL);
 
-	obj = normalize_mountable_object(device);
+	normalized = normalize_mountable_object(device, &obj);
 
 	g_hash_table_iter_init(&iter, slots);
 	while (g_hash_table_iter_next(&iter, NULL, (gpointer*) &slot)) {
 		if (g_strcmp0(slot->device, device) == 0)
 			goto out;
 
-		// Path doesn't match, but maybe device is same?
-		if (obj) {
-			g_autofree MountableObj *slot_obj =
-				normalize_mountable_object(slot->device);
-
-			if (slot_obj && is_same_mountable_object(obj, slot_obj))
+		/* Path doesn't match, but maybe device is the same? */
+		if (normalized) {
+			MountableObj slot_obj;
+			if (normalize_mountable_object(slot->device, &slot_obj) &&
+			    is_same_mountable_object(&obj, &slot_obj))
 				goto out;
 		}
 	}

Re: [RAUC] Failed to mount seed slot

[Trent Piepho]

On Tue, 2019-05-07 at 17:49 +0200, Ladislav Michl wrote:
> On Tue, May 07, 2019 at 09:31:28AM +0200, Ladislav Michl wrote:
> > On Mon, May 06, 2019 at 07:44:09PM +0200, Ladislav Michl wrote:
> > > [slot.rootfs.0]
> > > device=/dev/ubi0_0
> > > type=ubifs
> > > bootname=system0
> > > 
> > > Now system is booted with rootfs mounted read only which makes mounting seed
> > > slot fail:
> > > rauc[871]: Mounting /dev/ubi0_0 to use as seed
> > > rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
> > > rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
> > > rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.

What does the kernel show in /proc/mounts in this case?  If it's
"/dev/ubi0_0", then it should match based on the simple path compare.

> Problem is similar to that one solved by #406 and #388, however it needs to be
> extended to ubi<X> and possibly mtd<X> case. For now let's start with fixing
> pull request #406 to:

Is there a way to mount a /dev/mtd<X> device directly, and not through
/dev/mtdblock<X>?  I was under the impression that yaffs1/2 and jffs2
used mtdblock, but I haven't used raw nand (what a PITA it is) in
while.

> - use C comments

Is this a requirement for rauc?  I see other // comments in rauc, they
are part of the C standard since C99, and rauc's extensive github
commit checking doesn't flag it.

> - avoid unneded allocations

Perhaps this is better, since none of the allocations need to live.

> - also allow character devices (for example /dev/ubi0_0)

Seems reasonable to fix that case.  This would work for /dev/mtd too,
if there is a way to mount those.

> > Then determine_slot_states should properly fill slot's ext_mount_point and
> > casync_extract_image would then use it to bind mount seed slot.
> > 
> > And here's the tricky part. For device-less mount, volume to mount is specified
> > for example using ubiX_Y or ubiX:NAME syntax, so fix in #406 will fail in this
> > case.

If the mount call uses ubiX:NAME, what does /proc/mounts report?  That
really the issue: matching the config file device string to the string
reported by the kernel when the device is mounted.

> This one was fixed by extending normalize_mountable_object from #406 and
> adjusting casync_extract_image, but lets first get #406 revieved and merged :)
> Trent, in case you want to merge following additional patch to your PR here's my
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> diff --git a/src/config_file.c b/src/config_file.c
> index 875e0ee..e592d15 100644
> --- a/src/config_file.c
> +++ b/src/config_file.c
> @@ -494,27 +494,25 @@ free:
>  	return res;
>  }
>  
> -// Something that is, or can be, mounted onto a mount point
> +/* Something that is, or can be, mounted onto a mount point */
>  typedef struct {
> -	gboolean is_device;  // vs being a loop mounted file
> -	dev_t dev;  // the device, or for a file, the device the file is on
> -	ino_t inode;  // inode of file for a non-device
> +	gboolean is_device;	/* vs being a loop mounted file */
> +	dev_t dev;		/* the device, or for a file, the device the file is on */
> +	ino_t inode;		/* inode of file for a non-device */
>  } MountableObj;

There would also need to be another bool to indicate if the device is a
 block dev or char dev, since the major:minor could match between
different device types.  That could be part of a union with inode since
the two are mutually exclusive.

>  
> -// Take a device (or file) path and normalize it
> -static MountableObj *normalize_mountable_object(const gchar *devicepath)
> +/* Take a device (or file) path or name and normalize it */
> +static gboolean normalize_mountable_object(const gchar *devicename, MountableObj *obj)
>  {
> -	MountableObj *obj;
>  	GStatBuf st;
>  
> -	if (g_stat(devicepath, &st) == -1) {
> +	if (g_stat(devicename, &st) == -1) {
>  		/* Virtual filesystems like devpts trigger case */
> -		g_debug("Can't stat '%s', assuming unmountable: %s", devicepath, g_strerror(errno));
> -		return NULL;
> +		g_debug("Can't stat '%s', assuming unmountable: %s", devicename, g_strerror(errno));
> +		return FALSE;
>  	}
>  
> -	obj = g_new0(MountableObj, 1);
> -	if (S_ISBLK(st.st_mode)) {
> +	if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
>  		obj->is_device = TRUE;
>  		obj->dev = st.st_rdev;
>  	} else if (S_ISREG(st.st_mode)) {
> @@ -522,10 +520,10 @@ static MountableObj *normalize_mountable_object(const gchar *devicepath)
>  		obj->dev = st.st_dev;
>  		obj->inode = st.st_ino;
>  	} else {
> -		g_debug("Device '%s' is not something which is mountable", devicepath);
> -		g_clear_pointer(&obj, g_free);
> +		g_debug("Device '%s' is not something which is mountable", devicename);
> +		return FALSE;
>  	}
> -	return obj;
> +	return TRUE;
>  }
>  
>  /* Compare two MountableObj for equality */
> @@ -546,24 +544,24 @@ RaucSlot *find_slot_by_device(GHashTable *slots, const gchar *device)
>  {
>  	GHashTableIter iter;
>  	RaucSlot *slot;
> -	g_autofree MountableObj *obj = NULL;
> +	MountableObj obj;
> +	gboolean normalized;
>  
>  	g_return_val_if_fail(slots, NULL);
>  	g_return_val_if_fail(device, NULL);
>  
> -	obj = normalize_mountable_object(device);
> +	normalized = normalize_mountable_object(device, &obj);
>  
>  	g_hash_table_iter_init(&iter, slots);
>  	while (g_hash_table_iter_next(&iter, NULL, (gpointer*) &slot)) {
>  		if (g_strcmp0(slot->device, device) == 0)
>  			goto out;
>  
> -		// Path doesn't match, but maybe device is same?
> -		if (obj) {
> -			g_autofree MountableObj *slot_obj =
> -				normalize_mountable_object(slot->device);
> -
> -			if (slot_obj && is_same_mountable_object(obj, slot_obj))
> +		/* Path doesn't match, but maybe device is the same? */
> +		if (normalized) {
> +			MountableObj slot_obj;
> +			if (normalize_mountable_object(slot->device, &slot_obj) &&
> +			    is_same_mountable_object(&obj, &slot_obj))
>  				goto out;
>  		}
>  	}

Re: [RAUC] Failed to mount seed slot

[Ladislav Michl]

On Tue, May 07, 2019 at 05:59:36PM +0000, Trent Piepho wrote:
> On Tue, 2019-05-07 at 17:49 +0200, Ladislav Michl wrote:
> > On Tue, May 07, 2019 at 09:31:28AM +0200, Ladislav Michl wrote:
> > > On Mon, May 06, 2019 at 07:44:09PM +0200, Ladislav Michl wrote:
> > > > [slot.rootfs.0]
> > > > device=/dev/ubi0_0
> > > > type=ubifs
> > > > bootname=system0
> > > > 
> > > > Now system is booted with rootfs mounted read only which makes mounting seed
> > > > slot fail:
> > > > rauc[871]: Mounting /dev/ubi0_0 to use as seed
> > > > rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
> > > > rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
> > > > rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.
> 
> What does the kernel show in /proc/mounts in this case?  If it's
> "/dev/ubi0_0", then it should match based on the simple path compare.

System is using barebox' bootchooser with boot loader specification. Here
device is ubi0:rootfs0. And yes, device name could be changed in rauc's
slot configuration to be ubi0:rootfs0 allowing path compare succeed.
But see bellow why it is worth changing code a bit.

> > Problem is similar to that one solved by #406 and #388, however it needs to be
> > extended to ubi<X> and possibly mtd<X> case. For now let's start with fixing
> > pull request #406 to:
> 
> Is there a way to mount a /dev/mtd<X> device directly, and not through
> /dev/mtdblock<X>?  I was under the impression that yaffs1/2 and jffs2
> used mtdblock, but I haven't used raw nand (what a PITA it is) in
> while.

I guess mtd layer does not need block device to mount for over a decade,
but haven't bothered to found that commit :)

> > - use C comments
> 
> Is this a requirement for rauc?  I see other // comments in rauc, they
> are part of the C standard since C99, and rauc's extensive github
> commit checking doesn't flag it.

I have no strong opinion here, just made it consistent with the rest
of file.

> > - avoid unneded allocations
> 
> Perhaps this is better, since none of the allocations need to live.
> 
> > - also allow character devices (for example /dev/ubi0_0)
> 
> Seems reasonable to fix that case.  This would work for /dev/mtd too,
> if there is a way to mount those.

Please see http://www.linux-mtd.infradead.org/faq/jffs2.html#L_mtdblock

> > > Then determine_slot_states should properly fill slot's ext_mount_point and
> > > casync_extract_image would then use it to bind mount seed slot.
> > > 
> > > And here's the tricky part. For device-less mount, volume to mount is specified
> > > for example using ubiX_Y or ubiX:NAME syntax, so fix in #406 will fail in this
> > > case.
> 
> If the mount call uses ubiX:NAME, what does /proc/mounts report?  That
> really the issue: matching the config file device string to the string
> reported by the kernel when the device is mounted.

/proc/mounts reports whatever was used as mount argument.

> > This one was fixed by extending normalize_mountable_object from #406 and
> > adjusting casync_extract_image, but lets first get #406 revieved and merged :)
> > Trent, in case you want to merge following additional patch to your PR here's my
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > 
> > diff --git a/src/config_file.c b/src/config_file.c
> > index 875e0ee..e592d15 100644
> > --- a/src/config_file.c
> > +++ b/src/config_file.c
> > @@ -494,27 +494,25 @@ free:
> >  	return res;
> >  }
> >  
> > -// Something that is, or can be, mounted onto a mount point
> > +/* Something that is, or can be, mounted onto a mount point */
> >  typedef struct {
> > -	gboolean is_device;  // vs being a loop mounted file
> > -	dev_t dev;  // the device, or for a file, the device the file is on
> > -	ino_t inode;  // inode of file for a non-device
> > +	gboolean is_device;	/* vs being a loop mounted file */
> > +	dev_t dev;		/* the device, or for a file, the device the file is on */
> > +	ino_t inode;		/* inode of file for a non-device */
> >  } MountableObj;
> 
> There would also need to be another bool to indicate if the device is a
>  block dev or char dev, since the major:minor could match between
> different device types.  That could be part of a union with inode since
> the two are mutually exclusive.

Ah, will fix that. Eventually it is possible to walk /sys/dev using major:minor
and find corresponding uevent file which is holding DEVNAME key, but I like
your approach a lot more as it will also allow us to move all logic which is
now scattered over different places to find_slot_by_device. The loop in
determine_slot_states then boils down to:

	/* Determine active slot mount points */
	mountlist = g_unix_mounts_get(NULL);
	for (GList *l = mountlist; l != NULL; l = l->next) {
		GUnixMountEntry *m = (GUnixMountEntry*)l->data;
		RaucSlot *s = find_config_slot_by_device(r_context()->config, g_unix_mount_get_device_path(m));
		if (!s || s->ext_mount_point)
			continue;

		s->ext_mount_point = g_strdup(g_unix_mount_get_mount_path(m));
		g_debug("Found external mountpoint for slot %s at %s",
				s->name, s->ext_mount_point);
	}
	g_list_free_full(mountlist, (GDestroyNotify)g_unix_mount_free);

Then we can remove resolve_loop_device as your match code should cover
that special case too and with normalize_mountable_object first checking
for "special" device names and then stating path if it is starting with
'/' all quirks elsewhere are gone.

So here's the last remaining piece to puzzle:
--- a/src/update_handler.c
+++ b/src/update_handler.c
@@ -253,11 +253,29 @@ static gboolean casync_extract_image(RaucImage *image, gchar *dest, GError **err
 		 * file systems, additional mounts, etc. */
 		if (!seedslot->mount_point) {
 			g_debug("Mounting %s to use as seed", seedslot->device);
-			res = r_mount_slot(seedslot, &ierror);
-			if (!res) {
-				g_warning("Failed mounting for seeding: %s", ierror->message);
-				g_clear_error(&ierror);
-				goto extract;
+			if (seedslot->ext_mount_point) {
+				gchar *mount_point = r_create_mount_point(seedslot->name, &ierror);
+				if (!mount_point) {
+					g_warning("Failed creating bind mount point for seeding: %s", ierror->message);
+					g_clear_error(&ierror);
+					goto extract;
+				}
+				res = r_mount_full(seedslot->ext_mount_point, mount_point, NULL, 0, "bind", &ierror);
+				if (!res) {
+					g_warning("Failed bind mounting for seeding: %s", ierror->message);
+					g_clear_error(&ierror);
+					g_rmdir(mount_point);
+					g_free(mount_point);
+					goto extract;
+				}
+				seedslot->mount_point = mount_point;
+			} else {
+				res = r_mount_slot(seedslot, &ierror);
+				if (!res) {
+					g_warning("Failed mounting for seeding: %s", ierror->message);
+					g_clear_error(&ierror);
+					goto extract;
+				}
 			}
 			seed_mounted = TRUE;
 		}

Above change would actually bind mount slot for seeding...

> > -// Take a device (or file) path and normalize it
> > -static MountableObj *normalize_mountable_object(const gchar *devicepath)
> > +/* Take a device (or file) path or name and normalize it */
> > +static gboolean normalize_mountable_object(const gchar *devicename, MountableObj *obj)
> >  {
> > -	MountableObj *obj;
> >  	GStatBuf st;
> >  
> > -	if (g_stat(devicepath, &st) == -1) {
> > +	if (g_stat(devicename, &st) == -1) {
> >  		/* Virtual filesystems like devpts trigger case */
> > -		g_debug("Can't stat '%s', assuming unmountable: %s", devicepath, g_strerror(errno));
> > -		return NULL;
> > +		g_debug("Can't stat '%s', assuming unmountable: %s", devicename, g_strerror(errno));
> > +		return FALSE;
> >  	}
> >  
> > -	obj = g_new0(MountableObj, 1);
> > -	if (S_ISBLK(st.st_mode)) {
> > +	if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
> >  		obj->is_device = TRUE;
> >  		obj->dev = st.st_rdev;
> >  	} else if (S_ISREG(st.st_mode)) {
> > @@ -522,10 +520,10 @@ static MountableObj *normalize_mountable_object(const gchar *devicepath)
> >  		obj->dev = st.st_dev;
> >  		obj->inode = st.st_ino;
> >  	} else {
> > -		g_debug("Device '%s' is not something which is mountable", devicepath);
> > -		g_clear_pointer(&obj, g_free);
> > +		g_debug("Device '%s' is not something which is mountable", devicename);
> > +		return FALSE;
> >  	}
> > -	return obj;
> > +	return TRUE;
> >  }
> >  
> >  /* Compare two MountableObj for equality */
> > @@ -546,24 +544,24 @@ RaucSlot *find_slot_by_device(GHashTable *slots, const gchar *device)
> >  {
> >  	GHashTableIter iter;
> >  	RaucSlot *slot;
> > -	g_autofree MountableObj *obj = NULL;
> > +	MountableObj obj;
> > +	gboolean normalized;
> >  
> >  	g_return_val_if_fail(slots, NULL);
> >  	g_return_val_if_fail(device, NULL);
> >  
> > -	obj = normalize_mountable_object(device);
> > +	normalized = normalize_mountable_object(device, &obj);
> >  
> >  	g_hash_table_iter_init(&iter, slots);
> >  	while (g_hash_table_iter_next(&iter, NULL, (gpointer*) &slot)) {
> >  		if (g_strcmp0(slot->device, device) == 0)
> >  			goto out;
> >  
> > -		// Path doesn't match, but maybe device is same?
> > -		if (obj) {
> > -			g_autofree MountableObj *slot_obj =
> > -				normalize_mountable_object(slot->device);
> > -
> > -			if (slot_obj && is_same_mountable_object(obj, slot_obj))
> > +		/* Path doesn't match, but maybe device is the same? */
> > +		if (normalized) {
> > +			MountableObj slot_obj;
> > +			if (normalize_mountable_object(slot->device, &slot_obj) &&
> > +			    is_same_mountable_object(&obj, &slot_obj))
> >  				goto out;
> >  		}
> >  	}

_______________________________________________
RAUC mailing list

Re: [RAUC] Failed to mount seed slot

[Ladislav Michl]

On Tue, May 07, 2019 at 05:59:36PM +0000, Trent Piepho wrote:
> On Tue, 2019-05-07 at 17:49 +0200, Ladislav Michl wrote:
> > On Tue, May 07, 2019 at 09:31:28AM +0200, Ladislav Michl wrote:
> >  typedef struct {
> > -	gboolean is_device;  // vs being a loop mounted file
> > -	dev_t dev;  // the device, or for a file, the device the file is on
> > -	ino_t inode;  // inode of file for a non-device
> > +	gboolean is_device;	/* vs being a loop mounted file */
> > +	dev_t dev;		/* the device, or for a file, the device the file is on */
> > +	ino_t inode;		/* inode of file for a non-device */
> >  } MountableObj;
> 
> There would also need to be another bool to indicate if the device is a
>  block dev or char dev, since the major:minor could match between
> different device types.  That could be part of a union with inode since
> the two are mutually exclusive.

As those functions are close to each other I didn't bothered with creating
an union and simply used 1 and 2 for is_device :)
Also directory can be a bind mount, so added that as well, however it is not
yet tested.

---
 src/config_file.c | 60 ++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/src/config_file.c b/src/config_file.c
index 875e0ee..d966533 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -494,38 +494,44 @@ free:
 	return res;
 }
 
-// Something that is, or can be, mounted onto a mount point
+/* Something that is, or can be, mounted onto a mount point */
 typedef struct {
-	gboolean is_device;  // vs being a loop mounted file
-	dev_t dev;  // the device, or for a file, the device the file is on
-	ino_t inode;  // inode of file for a non-device
+	dev_t dev;		/* the device, or for a file, the device the file is on */
+	ino_t inode;		/* inode of file for a non-device */
+	gchar is_device;	/* vs being a loop mounted file */
 } MountableObj;
 
-// Take a device (or file) path and normalize it
-static MountableObj *normalize_mountable_object(const gchar *devicepath)
+/* Take a device (or file) path or name and normalize it */
+static gboolean normalize_mountable_object(const gchar *name, MountableObj *obj)
 {
-	MountableObj *obj;
 	GStatBuf st;
 
-	if (g_stat(devicepath, &st) == -1) {
+	if (g_stat(name, &st) == -1) {
 		/* Virtual filesystems like devpts trigger case */
-		g_debug("Can't stat '%s', assuming unmountable: %s", devicepath, g_strerror(errno));
-		return NULL;
+		g_debug("Can't stat '%s', assuming unmountable: %s", name, g_strerror(errno));
+		return FALSE;
 	}
 
-	obj = g_new0(MountableObj, 1);
-	if (S_ISBLK(st.st_mode)) {
-		obj->is_device = TRUE;
+	switch (st.st_mode & S_IFMT) {
+	case S_IFCHR:
+		obj->is_device = 1;
 		obj->dev = st.st_rdev;
-	} else if (S_ISREG(st.st_mode)) {
-		obj->is_device = FALSE;
+		break;
+	case S_IFBLK:
+		obj->is_device = 2;
+		obj->dev = st.st_rdev;
+		break;
+	case S_IFDIR:
+	case S_IFREG:
+		obj->is_device = 0;
 		obj->dev = st.st_dev;
 		obj->inode = st.st_ino;
-	} else {
-		g_debug("Device '%s' is not something which is mountable", devicepath);
-		g_clear_pointer(&obj, g_free);
+		break;
+	default:
+		g_debug("Device '%s' is unmountable", name);
+		return FALSE;
 	}
-	return obj;
+	return TRUE;
 }
 
 /* Compare two MountableObj for equality */
@@ -546,24 +552,24 @@ RaucSlot *find_slot_by_device(GHashTable *slots, const gchar *device)
 {
 	GHashTableIter iter;
 	RaucSlot *slot;
-	g_autofree MountableObj *obj = NULL;
+	MountableObj obj;
+	gboolean normalized;
 
 	g_return_val_if_fail(slots, NULL);
 	g_return_val_if_fail(device, NULL);
 
-	obj = normalize_mountable_object(device);
+	normalized = normalize_mountable_object(device, &obj);
 
 	g_hash_table_iter_init(&iter, slots);
 	while (g_hash_table_iter_next(&iter, NULL, (gpointer*) &slot)) {
 		if (g_strcmp0(slot->device, device) == 0)
 			goto out;
 
-		// Path doesn't match, but maybe device is same?
-		if (obj) {
-			g_autofree MountableObj *slot_obj =
-				normalize_mountable_object(slot->device);
-
-			if (slot_obj && is_same_mountable_object(obj, slot_obj))
+		/* Path doesn't match, but maybe device is the same? */
+		if (normalized) {
+			MountableObj slot_obj;
+			if (normalize_mountable_object(slot->device, &slot_obj) &&
+			    is_same_mountable_object(&obj, &slot_obj))
 				goto out;
 		}
 	}
-- 
2.20.1

Re: [RAUC] Failed to mount seed slot

[Ladislav Michl]

On Tue, May 07, 2019 at 10:33:48PM +0200, Ladislav Michl wrote:
> On Tue, May 07, 2019 at 05:59:36PM +0000, Trent Piepho wrote:
> > On Tue, 2019-05-07 at 17:49 +0200, Ladislav Michl wrote:
> > > On Tue, May 07, 2019 at 09:31:28AM +0200, Ladislav Michl wrote:
> > > > On Mon, May 06, 2019 at 07:44:09PM +0200, Ladislav Michl wrote:
> > > > > [slot.rootfs.0]
> > > > > device=/dev/ubi0_0
> > > > > type=ubifs
> > > > > bootname=system0
> > > > > 
> > > > > Now system is booted with rootfs mounted read only which makes mounting seed
> > > > > slot fail:
> > > > > rauc[871]: Mounting /dev/ubi0_0 to use as seed
> > > > > rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
> > > > > rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
> > > > > rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.
> > 
> > What does the kernel show in /proc/mounts in this case?  If it's
> > "/dev/ubi0_0", then it should match based on the simple path compare.
> 
> System is using barebox' bootchooser with boot loader specification. Here
> device is ubi0:rootfs0. And yes, device name could be changed in rauc's
> slot configuration to be ubi0:rootfs0 allowing path compare succeed.

Well, that is actually not true, with
[slot.rootfs.0]
device=ubi0:rootfs0
type=ubifs
bootname=system0

rauc will try to be smart here:
$ rauc status
Compatible:  Foomatic System
Variant:     
Booted from: system0
Activated:   rootfs.0 (system0)
slot states:
  rootfs.1: class=rootfs, device=/etc/rauc/ubi0:rootfs1, type=ubifs, bootname=system1
      state=inactive, description=, parent=(none), mountpoint=(none)
      boot status=good
  rootfs.0: class=rootfs, device=/etc/rauc/ubi0:rootfs0, type=ubifs, bootname=system0
      state=booted, description=, parent=(none), mountpoint=(none)
      boot status=good

> But see bellow why it is worth changing code a bit.

The only way to make it work now is to use /dev/ubi0_0 and fix barebox to use
the same naming scheme.

Also, there are more problems with mtd:<name> (those are passed to flash_erase) or
ubi:<volume> (passed to mkfs.ubifs) schemes, so some kind of normalizing device
names is required anyway. Either that or making each kind (block devices (ext4,
vfat), mtd, ubi..) of slot type to be descendants of common slot ancestor and
let them handle all those quirks on their own [*].

[patch stripped]

...and instead of my previous patch to bind mount seed slot, r_mount_slot
should be modified to do just that if ext_mount_point is assigned as
all remaining code logic suggests it is right thing to do.

Btw, it looks like development nad review is done on github using web
interface. Would that explain low volume of this very mailing list?

I would really appreciate some brief review of suggested solutions to
problems with using rauc with barebox bootchooser and ubi volumes.
They do work for me (quick hacked), but I'm new to rauc codebase
and it is easy to break somene else usecase.

Thank you,
	ladis

As a side note, why does rauc depend on glib? Does not seem to be intended
to be portable and if it just for all those hash or list containers, isn't
that exactly what STL is for? Also imagine it to be OOP ;-) Common slot
object with all those specialities encapsulated on its descendants. Not
that it cannot be done in C even with glib, but...