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 <la...@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
