On Mon, 27.09.10 18:34, Fabiano Fidencio (fiden...@profusion.mobi) wrote: > + > +static void mount_points_list_free(MountPoint **mp_list_head) { > + MountPoint *mp, *mp_next; > + LIST_FOREACH_SAFE(mount_point, mp, mp_next, *mp_list_head) > + mount_point_remove_and_free(mp, mp_list_head); > +}
I'd just do while (*list) mount_point_remove_and_free(*list, *list) Looks more readable to me. > + if ((k = fscanf(proc_self_mountinfo, > + "%*s " /* (1) mount id */ > + "%*s " /* (2) parent id */ > + "%*s " /* (3) major:minor */ > + "%*s " /* (4) root */ > + "%ms " /* (5) mount point */ > + "%*s" /* (6) mount options */ > + "%*[^-]" /* (7) optional fields */ > + "- " /* (8) seperator */ > + "%*s " /* (9) file system type */ > + "%*s" /* (10) mount source */ > + "%*s" /* (11) mount options 2 */ > + "%*[^\n]", /* some rubbish at the end */ > + &path)) != 1) { > + if (k == EOF) > + break; > + > + r = -EBADMSG; > + goto finish; > + } Thinking about this we probably should just go on with the next line when we fail to parse something, maybe with a warning. I figure mount.c exits here too, but I guess we should fix that. > + > + if (mount_point_is_api(path)) > + goto free; > + > + if (!(p = cunescape(path))) { > + r = -ENOMEM; > + free(p); > + goto finish; > + } > + > + mp = mount_point_alloc(p); You need to check for OOM here! > + > + LIST_FOREACH_SAFE(mount_point, mp, mp_next, *mp_list_head) { > + if (!(strcmp(mp->path, "/"))) > + continue; We have this little macro streq() which is a bit nicer to read. > + > + /* Trying to umount. Forcing to umount if busy (only for NFS > mounts) */ > + if (umount2(mp->path, MNT_FORCE) == 0) { > + if (!(*changed)) > + *changed = true; Hmm, why do you test *changed first? > + mount_point_remove_and_free(mp, mp_list_head); > + log_debug("umount: \"%s\" was umounted > succesfully\n", mp->path); > + > + } else > + log_debug("umount: forced umount of \"%s\" > failed!\n", mp->path); Please output error message here, and do so as log_warn(). Use %m. > + if (mount(NULL, mp->path, NULL, > MS_MGC_VAL|MS_REMOUNT|MS_RDONLY, NULL) == 0) { > + mp->read_only = true; > + if (!(*changed)) > + *changed = true; > + mount_point_remove_and_free(mp, mp_list_head); > + log_debug("umount: \"%s\" busy - remounted > read-only\n", mp->path); > + } else { > + log_error("Cannot remount \"%s\" read-only\n", > mp->path); > + sleep(1); Similar here. Otherwise looks fine! Lennart -- Lennart Poettering - Red Hat, Inc. _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel