On Mon, Aug 09, 2021 at 05:34:41PM +0200, Daniel Kiper wrote: > On Mon, Aug 02, 2021 at 05:40:20PM +0800, Michael Chang via Grub-devel wrote: > > Currently the grub_diskfilter_memberlist function returns all physical > > volumes added to a volume group to which a logical volume (LV) belongs. > > However this is suboptimal as it doesn't fit the intended behavior of > > returning underlying devices that make up the LV. To give a clear > > picture, the result should be identical to running commands below to > > display the logical volumes with underlying physical volumes in use. > > > > lvs -o +devices /dev/system/root > > lvdisplay --maps /dev/system/root > > May I ask you to provide output from these two commands too? Additionally, > I think it would be useful to have output from "pvs" and "lvs" commands > in the commit message. I know "lsblk" shows you most information but > I think both "pvs" and "lvs" are giving more readable output.
It sounds good as that helps better understanding. I'll add these information to next version. > > > This change is required if any part of the PV not used by root LV is > > encrypted. Using this lvm setup as an example: > > > > localhost:~ # lsblk > > NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT > > vda 253:0 0 20G 0 disk > > ├─vda1 253:1 0 8M 0 part > > └─vda2 253:2 0 20G 0 part > > ├─system-swap 254:0 0 1.4G 0 lvm [SWAP] > > └─system-root 254:1 0 18.6G 0 lvm / > > vdb 253:16 0 10M 0 disk > > └─data 254:2 0 8M 0 crypt > > └─system-data 254:3 0 4M 0 lvm /data > > > > Running grub-install would end up with error because "system" VG > > contains /dev/vdb which is encrypted. > > > > error: attempt to install to encrypted disk without cryptodisk > > enabled. Set `GRUB_ENABLE_CRYPTODISK=y' in file `/etc/default/grub'. > > > > Certainly we can enable GRUB_ENABLE_CRYPTODISK=y and move on, but that > > is not always acceptable since the server may need to boot unattended, > > s/,/./ OK. > > > in addition typing passphase for every system startup can be a big > > s/in addition/Additionally,/ OK. > > > hassle of which most users would like to avoid. > > > > This patch solves the problem by returning physical volumes, /dev/vda2, > > rightly used by system-root in the example above, thus changing how > > grub-install perceives the underlying block device to boot and avoids > > the error from happening. > > > > Signed-off-by: Michael Chang <mch...@suse.de> > > Tested-by: Olav Reinert <seroto...@gmail.com> > > --- > > grub-core/disk/diskfilter.c | 44 +++++++++++++++++++++++-------------- > > 1 file changed, 27 insertions(+), 17 deletions(-) > > > > diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c > > index 6eb2349a6..595f5f70f 100644 > > --- a/grub-core/disk/diskfilter.c > > +++ b/grub-core/disk/diskfilter.c > > @@ -300,6 +300,8 @@ grub_diskfilter_memberlist (grub_disk_t disk) > > grub_disk_dev_t p; > > struct grub_diskfilter_vg *vg; > > struct grub_diskfilter_lv *lv2 = NULL; > > + struct grub_diskfilter_segment *seg; > > + unsigned int i, j; > > > > if (!lv->vg->pvs) > > return NULL; > > @@ -331,25 +333,33 @@ grub_diskfilter_memberlist (grub_disk_t disk) > > } > > } > > > > - for (pv = lv->vg->pvs; pv; pv = pv->next) > > - { > > - if (!pv->disk) > > + for (i = 0, seg = lv->segments; i < lv->segment_count; i++, seg++) > > + for (j =0; j < seg->node_count; ++j) > > s/=0/= 0/ OK. > > > + if ((pv = seg->nodes[j].pv)) > > if (seg->nodes[j].pv != NULL) > > ...and then later... > > pv = seg->nodes[j].pv OK. > > > { > > - /* TRANSLATORS: This message kicks in during the detection of > > - which modules needs to be included in core image. This happens > > - in the case of degraded RAID and means that autodetection may > > - fail to include some of modules. It's an installation time > > - message, not runtime message. */ > > - grub_util_warn (_("Couldn't find physical volume `%s'." > > - " Some modules may be missing from core image."), > > - pv->name); > > - continue; > > + > > + if (!pv->disk) > > + { > > + /* TRANSLATORS: This message kicks in during the detection of > > + which modules needs to be included in core image. This happens > > + in the case of degraded RAID and means that autodetection may > > + fail to include some of modules. It's an installation time > > + message, not runtime message. */ > > Please fix this comment formatting if you move it. May I ask what is wrong as it looks good to the indention that has been adjusted to the new place ? > > > + grub_util_warn (_("Couldn't find physical volume `%s'." > > + " Some modules may be missing from core > > image."), > > + pv->name); > > + continue; > > + } > > + > > + for (tmp = list; tmp; tmp = tmp->next) > > tmp != NULL please... OK. > > > + if (grub_strcmp (tmp->disk->name, pv->disk->name) == 0) > > !grub_strcmp() instead of grub_strcmp() == 0 OK. Well. It appears "grub_strcmp() == 0" more popular as it has more grep hits . But I am fine in doing either way. :) grep -R '!grub_strcmp' | wc -l 26 grep -R 'grub_strcmp.*== 0' | wc -l 385 > > > + continue; > > I know what you mean but this "continue" does not make much sense here... Indeed this looks bogus. I will fix this in next version. > > > + > > + tmp = grub_malloc (sizeof (*tmp)); > > Please do not ignore grub_malloc() failures. OK. Thanks a lot for review. Michael > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel