[PATCH] discover/udev.c: Added warning in system status log

[Jeremy Kerr]

Hi Lulu,

> >  - you want to display a message indicating that petitboot has
> >    skipped *some* of the duplicates. Specifically, those duplicates that
> >    exist on the separate devices, but *not* the duplicates that exist as
> >    part of the partition layout that a single device is presenting.
> > 
> > Is that what you're trying to do here?
> 
> Yes, what you describe is exactly what I want to achieve.

OK, great!

> Thank you for your suggestion. According to your suggestion, I
> corrected the patch as follows: (Only list conditions)
> 
> @@ -179,11 +181,17 @@ static int udev_handle_block_add(struct pb_udev
> *udev, struct udev_device *dev,
>         /* We may see multipath devices; they'll have the same uuid as an
>          * existing device, so only parse the first. */
>         uuid = udev_device_get_property_value(dev, "ID_FS_UUID");
> +       idpath = udev_device_get_property_value(dev, "ID_PATH");
>         if (uuid) {
>                 ddev = device_lookup_by_uuid(udev->handler, uuid);
>                 if (ddev) {
>                         pb_log("SKIP: %s UUID [%s] already present (as %s)\n",
>                                         name, uuid, ddev->device->id);
> +                       if (strcmp(idpath, ddev->id_path)) {
> +                               device_handler_status_info(udev->handler, 
> +                               _("Duplicate filesystem (%s) has been detected; only listing the %s"), 
> +                               name, ddev->device->id);
> +                       } 
>                         return 0;
>                 }
>         }
> @@ -211,6 +219,7 @@ static int udev_handle_block_add(struct pb_udev
> *udev, struct udev_device *dev,
>                 }
>         }
>  
> +       ddev->id_path = talloc_strdup(ddev, idpath);
>         ddev->device_path = talloc_strdup(ddev, node);
>         talloc_free(devlinks);
> 
> As you suggested in the first part, I used ID_PATH instead of name as
> the condition.
> But in the second part, after verifying several different .iso files,
> I found that not all files have a partition (sdc1), so it is not
> suitable as a condition.

That's correct, not all devices will have a partition. But I don't think
that's a problem here; more on that below:

> Like you said, I didn't think thoroughly enough, so no longer set the
> display conditions, all the log messages will be displayed.

No, not all of them - it has suppressed the message for sda1, but not
sdc or sdc1.

According to your requirements above, in this case it sounds like you
want to the following to happen:

 1) sda: shows a boot option
 2) sda1: is a duplicate of sda, skipped, no message displayed
 3) sdc: duplicate of sda, skipped, message is displayed
 4) sdc1: duplicate of sda, skipped, no message displayed

- but we have no way of determining any difference between sdc and sdc1
  here; since sdc was skipped, we are not comparing ID_PATH against sdc.
  Can you come up with a policy for this case? Which of the sdc/sdc1
  devices should generate the message? (and how would you apply that
  logic?)

If no policy is obvious, then you may even want to do something like
warning *once* when we see a duplicate UUID as the first-discovered
device, no matter how many duplicates you see. Would that suit your
intended UX?

If so, you could just add a 'dup_warn' boolean on struct
discover_device, and only generate the warning if you haven't done so
already.

> Using the modified patch, the messages displayed is as follows:
> In Petitboot status log:
>  Duplicate filesystem (sdc) has been detected; only listing the sda
>  Duplicate filesystem (sdc1) has been detected; only listing the sda
>  Duplicate filesystem (sdc) has been detected; only listing the sda
>  Duplicate filesystem (sdc) has been detected; only listing the sda

I'd suggest a minor change to that wording - "the sda" sounds odd. Maybe
something like:

  Duplicate filesystem as sda detected on sdc.

Or if we're just warning once:

  Duplicate filesystem as sda detected; skipping duplicates.

> In pb-discover.log:
> [02:24:02] SKIP: sda1 UUID [2020-04-04-04-38-27-00] already present (as sda)
> [02:28:45] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda)
> [02:28:45] SKIP: sdc1 UUID [2020-04-04-04-38-27-00] already present (as sda)
> [02:28:45] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda)
> [02:28:45] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda)

Somewhat unlreated, but can you see why you're getting three events for
sdc? That's probably not helping with multiple log outputs in the UI.
Maybe try debug mode again.

Cheers,


Jeremy