| Message ID | 20210928025730.9706-1-Lulu_Su@wistron.com |
|---|---|
| State | New |
| Headers | show |
| Series | discover/udev.c: Added warning in system status log | expand |
Hi Lulu, > When the same iso files are installed at the same time, > the petitboot menu will only display the installed device first, > and will not notify the user that the same iso file has been > installed, > so an alert is added to the system status log to remind the user that > they have mounted the same iso file. > > Signed-off-by: LuluTHSu <Lulu_Su@wistron.com> If you're sending a second version of the patch, please include a short description of the change. It's OK for now, but can be handy for future revisions. It looks like this version just has the discover_device_create() removed, which is better. However, I still think it needs simplification: > @@ -184,6 +185,13 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev, > if (ddev) { > pb_log("SKIP: %s UUID [%s] already present (as %s)\n", > name, uuid, ddev->device->id); > + if (strncmp(name, ddev->device->id, strlen(ddev->device->id)) && (strlen(name) > strlen(ddev->device->id))) { > + char temp[] = "sdx"; > + strncpy(temp, name, strlen(ddev->device->id)); This introduces a potential stack overflow - we may attempt to write more data into 'temp' than it can store. But still, I don't know why you're making the log message conditional on not being a prefix of the other device. From your log in the previous discussion, you end up with four devices with the same UUID; surely you want to inform the user about all of them, rather than a subset? This check makes a lot of assumptions about when a duplicate device is worth logging or not. In that case, this patch would be *really* simple: just unconditionally call device_handler_status_info() with the log message. You probably want to make it shorter so it can fit in the status line of a default 80-column petitboot UI. Regards, Jeremy
Hi Lulu, > But still, I don't know why you're making the log message conditional > on not being a prefix of the other device. From your log in the > previous > discussion, you end up with four devices with the same UUID; surely > you > want to inform the user about all of them, rather than a subset? Thinking about this a little more, let me know if I have your requirements correct: - you have a system with two storage devices connected, both containing the same installer image - petitboot is suppressing the duplicates - 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? If that's the case, then the conditional that you've proposed makes sense, but using the device names (to establish whether the devices are on the same underlying media) doesn't seem like a reliable method of doing this - the names are fairly arbitrary. Instead, if you do want to do that, I'd suggest looking at the udev properties of the device. Two options here: - the ID_PATH properties of the two partitions will be the same on sda/sda1 in your examples - the DEVPATH property of the sda device will be a substring of the DEVPATH for the sda1 device. This way, you're not relying on the "sda" -> "sda1" matching, which is specific to the drivers' naming scheme. [that's mainly why I'd suggested making the log message unconditional - the duplicate matching isn't as straightforward as you're proposing here. If it's not a problem that you generate 3 log messages instead of 1, then that may be a simpler approach] Cheers, Jeremy
Hi Jeremy, > Thinking about this a little more, let me know if I have your requirements correct: > > - you have a system with two storage devices connected, both containing > the same installer image > > - petitboot is suppressing the duplicates > > - 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. > If that's the case, then the conditional that you've proposed makes sense, but using the device names (to establish whether the devices are on the same underlying media) doesn't seem like a reliable method of doing this - the names are fairly arbitrary. > > Instead, if you do want to do that, I'd suggest looking at the udev properties of the device. Two options here: > > - the ID_PATH properties of the two partitions will be the same on > sda/sda1 in your examples > > - the DEVPATH property of the sda device will be a substring of the > DEVPATH for the sda1 device. > > This way, you're not relying on the "sda" -> "sda1" matching, which is specific to the drivers' naming scheme. > > [that's mainly why I'd suggested making the log message unconditional - the duplicate matching isn't as straightforward as you're proposing here. If it's not a problem that you generate 3 log messages instead of 1, then that may be a simpler approach] 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. Like you said, I didn't think thoroughly enough, so no longer set the display conditions, all the log messages will be displayed. 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 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) The reason for retaining the first part of the condition is that if the file is successfully mounted to the petitboot list (sda), then its partition (sda1) does not need to display a warning message. This warning message is only displayed when trying to mount a file but a file with the same UUID is already mounted on petitboot. Is the content of the warning message clearly stated? After discussing and reaching a consensus, I will upload the third edition patch, thank you. Lulu Su --------------------------------------------------------------------------------------------------------------------------------------------------------------- This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately. ---------------------------------------------------------------------------------------------------------------------------------------------------------------
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
Hi Jeremy, > 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 Yes this is exactly what I want to achieve. > - 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?) I want to use the sd*1 device to generate messages, but I found that some iso files do not have this device; and on my machine, the device sd* will be generated repeatedly 3 times, which is not suitable as a condition... Since no correlation was found, it was decided not to set conditions for devices that cannot be mounted. > 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? Yes this is exactly what i expected. > 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. I thought about it before, but I didn't find a condition to reset this boolean. I will try again, thank you for your suggestion. > 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. Thank you so much for your suggestion. > 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. When the petitboot list has mounted the USB device (the USB has been plugged into the machine), this will happen when using virtual media to mount the same iso file; but on the contrary, only sdc and sdc1 will be displayed. I have seen this phenomenon on two platforms: Mihawk and Mowgli > Maybe try debug mode again. Ok, if there are other discoveries, i will share, thank you~ Cheers, Lulu Su --------------------------------------------------------------------------------------------------------------------------------------------------------------- This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately. ---------------------------------------------------------------------------------------------------------------------------------------------------------------
Hi Lulu, > > 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 > > Yes this is exactly what I want to achieve. Ok, but you say below: > > 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? > > Yes this is exactly what i expected. - this is different from the case above, so they can't be both what you want? The first option is that you generate a message for each *device* that holds a duplicate option, but do not generate a message where the duplicate is on a partition of a device already discovered. The second option is that you generate one message if a duplicate is found, and no further messages. So, to match the list above, the second option would give you: 1) sda: shows a boot option 2) sda1: is a duplicate of sda, skipped, message is displayed 3) sdc: is a duplicate of sda, skipped, no message displayed 4) sdc1: is a duplicate of sda, skipped, no message displayed The difficulty is that the first option requires us to track the devices that we have seen with a duplicate UUID (sdc in your example), so that we can then suppress further messages for any partitions that may be on that device. At the moment, we just skip all duplicates, so do not track that information. So, if you want the first option, we will need to not skip those duplicates, but instead implement some kind of duplicate tracking. Keep in mind that we absolutely *cannot* allow normal discovery to proceed for a device with a duplicate UUID - there are a lot of assumptions that require UUIDs to represent a unique device, both in petitboot and the bootloader configurations that petitboot reads. So, my question was whether the second option is acceptable, as it'll be considerably less code to implement. > > 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. > > I thought about it before, but I didn't find a condition to reset > this boolean. I will try again, thank you for your suggestion. You would never clear the boolean. Once we have generated a warning about seeing a duplicate of a device, we do not generate any further warnings about that same UUID. > > 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. > > When the petitboot list has mounted the USB device (the USB has been > plugged into the machine), this will happen when using virtual media > to mount the same iso file; but on the contrary, only sdc and sdc1 > will be displayed. > I have seen this phenomenon on two platforms: Mihawk and Mowgli Yes, I understand there will be duplicates if you connect the same filesytstem both locally and via virtual media. However, seeing the same device (sdc in your logs) appear in three separate udev events is unusual - we might want to investigate that. Regards, Jeremy
Hi Jeremy, > - this is different from the case above, so they can't be both what you want? > > The first option is that you generate a message for each *device* that holds a duplicate option, but do not generate a message where the duplicate is on a partition of a device already discovered. > > The second option is that you generate one message if a duplicate is found, and no further messages. Sorry for the confusion, thanks for the detailed explanation, the first option is what I expected. > The difficulty is that the first option requires us to track the devices that we have seen with a duplicate UUID (sdc in your example), so that we can then suppress further messages for any partitions that may be on that device. At the moment, we just skip all duplicates, so do not track that information. > > So, if you want the first option, we will need to not skip those duplicates, but instead implement some kind of duplicate tracking. > > Keep in mind that we absolutely *cannot* allow normal discovery to proceed for a device with a duplicate UUID - there are a lot of assumptions that require UUIDs to represent a unique device, both in petitboot and the bootloader configurations that petitboot reads. Thanks for your reminder. Knowing that duplicate device information will not be recorded, so I only used the device name of the mounted device and the duplicate device as a condition before, but this condition, as you said, was not fully considered. > So, my question was whether the second option is acceptable, as it'll be considerably less code to implement. For the user, when the device is mounted and displayed on the petitboot list (sda), they will not care whether the device has a partition (sda1); but when they tries to mount a new device (sdc), Due to duplicate UUID, it will not be mounted and will not be displayed any warning (although pb-discover.log has a record, but the user will not take the initiative to confirm this log), they will be confused whether the device or the system is malfunctioning. 1) sda: shows a boot option < DEVTYPE=disk 2) sda1: is a duplicate of sda, skipped, no message displayed < DEVTYPE=partition 3) sdc: duplicate of sda, skipped, message is displayed < DEVTYPE=disk 4) sdc1: duplicate of sda, skipped, no message displayed < DEVTYPE=partition udev_handle_block_add #111 typestr = udev_device_get_devtype(dev); What do you think about using DEVTYPE to distinguish sdc and sdc1? Only warnings are displayed on the disk because not all devices have partitions. Although it will be displayed 3 times on my machine, it can distinguish sdc and sdc1 under normal circumstances, right? I want to use the boolean you mentioned below, but I haven't fully understood it yet, sorry... > You would never clear the boolean. Once we have generated a warning about seeing a duplicate of a device, we do not generate any further warnings about that same UUID. > Yes, I understand there will be duplicates if you connect the same filesytstem both locally and via virtual media. However, seeing the same device (sdc in your logs) appear in three separate udev events is unusual - we might want to investigate that. Tracking the code, it is found that the device action of the first two sdc and sdc1 is "add", and the device action of the last two sdc and sdc is "change". The debug log is still under study. Regards, Lulu Su --------------------------------------------------------------------------------------------------------------------------------------------------------------- This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately. ---------------------------------------------------------------------------------------------------------------------------------------------------------------
Hi Lulu, > For the user, when the device is mounted and displayed on the > petitboot list (sda), they will not care whether the device has a > partition (sda1); but when they tries to mount a new device (sdc), > Due to duplicate UUID, it will not be mounted and will not be > displayed any warning (although pb-discover.log has a record, but the > user will not take the initiative to confirm this log), they will be > confused whether the device or the system is malfunctioning. > > 1) sda: shows a boot option < DEVTYPE=disk > 2) sda1: is a duplicate of sda, skipped, no message displayed < > DEVTYPE=partition > 3) sdc: duplicate of sda, skipped, message is displayed < > DEVTYPE=disk > 4) sdc1: duplicate of sda, skipped, no message displayed < > DEVTYPE=partition > > udev_handle_block_add > #111 typestr = udev_device_get_devtype(dev); > > What do you think about using DEVTYPE to distinguish sdc and sdc1? No, that's not suitable. It's pretty unusual for there to be a filesystem on the base device and a partition. The reason this is occurring is that the install image is reporting itself as both an iso9660 device, *and* a MBR partition table. That's why we're seeing the duplicates on the same device. The regular scenario for discovered filesystem is just the partitions carrying filesystems, and none on the top-level device. I assume you still need warnings for that case too. If you only generate the message when DEVTYPE == disk, then you'll miss those, and this code will only work for these specific installer-type images. > Only warnings are displayed on the disk because not all devices have > partitions. > Although it will be displayed 3 times on my machine, it can > distinguish sdc and sdc1 under normal circumstances, right? > > I want to use the boolean you mentioned below, but I haven't fully > understood it yet, sorry... My suggestion is to sort-of reverse the duplicate detection - and just record in the first device (that provided a specific UUID) whether we've warned about that UUID. So, when we find the first duplicate (sda1, sdc, or sdc1 in your examples), you generate the log message, and set ->dup_warn on the discover_device for sda. Then, subsequent duplicates do not generate a warning, because ->dup_warn is already set on the original duplicate (ie, sda). However, this may not be sufficient for your example - I assume you require the warning to be generated when the new device, sdc, is attached. With this flow, it would be generated when we detect sda1, so may not be obvious to indicate to the user what is happening when the new virtual media device appears. So, to implement it the way you're wanting here, it sounds like you're going to have to implement a system for duplicate tracking - eg, by keeping a enough info about the devices & UUIDs that you've seen, separate from the main device set, so you can generate the warnings in the way that you require (ie, warn once for a complete new device). > > Yes, I understand there will be duplicates if you connect the same > > filesytstem both locally and via virtual media. However, seeing the > > same device (sdc in your logs) appear in three separate udev events > > is unusual - we might want to investigate that. > > Tracking the code, it is found that the device action of the first > two sdc and sdc1 is "add", and the device action of the last two sdc > and sdc is "change". OK, that makes sense then, all sounds fine there. Regards, Jeremy
Hi Jeremy, > No, that's not suitable. It's pretty unusual for there to be a filesystem on the base device and a partition. The reason this is occurring is that the install image is reporting itself as both an > iso9660 device, *and* a MBR partition table. That's why we're seeing the duplicates on the same device. > > The regular scenario for discovered filesystem is just the partitions carrying filesystems, and none on the top-level device. I assume you still need warnings for that case too. If you > only generate the message when DEVTYPE == disk, then you'll miss those, and this code will only work for these specific installer-type images. > > My suggestion is to sort-of reverse the duplicate detection - and just record in the first device (that provided a specific UUID) whether we've warned about that UUID. > > So, when we find the first duplicate (sda1, sdc, or sdc1 in your examples), you generate the log message, and set ->dup_warn on the discover_device for sda. > > Then, subsequent duplicates do not generate a warning, because->dup_warn is already set on the original duplicate (ie, sda). > > However, this may not be sufficient for your example - I assume you require the warning to be generated when the new device, sdc, is attached. With this flow, it would be generated when we detect sda1, so may not be obvious to indicate to the user what is happening when the new virtual media device appears. > > So, to implement it the way you're wanting here, it sounds like you're going to have to implement a system for duplicate tracking - eg, by keeping a enough info about the devices & UUIDs that you've seen, separate from the main device set, so you can generate the warnings in the way that you require (ie, warn once for a complete new device). Thank you for your information, it has benefited me a lot. Based on the previous discussion, I amended the conditions as follows: 1. When the device sda is mounted, use ID_PATH to determine that sad1 and sda are the same device. 2. When a new device (sdc) is detected, the warning will be displayed only once; the Boolean value will not be cleared until the device is removed. Therefore, the warning will only be displayed once. When a new device (sdc) with the same UUID is detected, and when the user repeatedly mounts the same device (sdc), it will still be displayed again. Hope this modification meets your suggestion, thank you very much~ Here is the patch: discover/device-handler.h | 2 ++ discover/udev.c | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/discover/device-handler.h b/discover/device-handler.h index 6591120..216d17b 100644 --- a/discover/device-handler.h +++ b/discover/device-handler.h @@ -25,6 +25,7 @@ struct discover_device { const char *uuid; const char *label; + const char *id_path; char *mount_path; char *root_path; @@ -36,6 +37,7 @@ struct discover_device { bool crypt_device; bool notified; + bool dup_warn; struct list boot_options; struct list params; diff --git a/discover/udev.c b/discover/udev.c index 0c3da66..a140008 100644 --- a/discover/udev.c +++ b/discover/udev.c @@ -20,6 +20,7 @@ #include <waiter/waiter.h> #include <system/system.h> #include <process/process.h> +#include <i18n/i18n.h> #include "event.h" #include "udev.h" @@ -101,6 +102,7 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev, const char *prop; const char *type; const char *devname; + const char *idpath; const char *ignored_types[] = { "linux_raid_member", "swap", @@ -179,11 +181,19 @@ 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); + /* Only warn once in petitboot status log to remind users */ + if (strcmp(idpath, ddev->id_path) && !ddev->dup_warn) { + device_handler_status_info(udev->handler, + _("Duplicate filesystem as %s detected; skipping duplicates"), + ddev->device->id); + ddev->dup_warn = true; + } return 0; } } @@ -211,6 +221,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); @@ -355,6 +366,7 @@ static int udev_handle_dev_remove(struct pb_udev *udev, struct udev_device *dev) { struct discover_device *ddev; const char *name; + const char *uuid; name = udev_device_get_sysname(dev); if (!name) { @@ -363,9 +375,15 @@ static int udev_handle_dev_remove(struct pb_udev *udev, struct udev_device *dev) } ddev = device_lookup_by_id(udev->handler, name); - if (!ddev) + if (!ddev) { + uuid = udev_device_get_property_value(dev, "ID_FS_UUID"); + if (uuid) { + ddev = device_lookup_by_uuid(udev->handler, uuid); + if (ddev) + ddev->dup_warn = false; + } return 0; - + } device_handler_remove(udev->handler, ddev); return 0; Regards, Lulu Su --------------------------------------------------------------------------------------------------------------------------------------------------------------- This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately. ---------------------------------------------------------------------------------------------------------------------------------------------------------------
Hi Lulu, > Based on the previous discussion, I amended the conditions as > follows: > 1. When the device sda is mounted, use ID_PATH to determine > that sad1 and sda are the same device. > 2. When a new device (sdc) is detected, the warning will be > displayed only once; the Boolean value will not be cleared until the > device is removed. > Therefore, the warning will only be displayed once. When a new device > (sdc) with the same UUID is detected, and when the user repeatedly > mounts the same device (sdc), it will still be displayed again. > Hope this modification meets your suggestion, thank you very much~ Yes, that all sounds fine to me. You could even do this without the udev_handle_dev_remove changes (which clear the dev_warn flag); we've warned about sda already, so I don't see much need to re-generate the warning later. But either way works. If you could send this as a proper commit, I can apply to the tree. Cheers, Jeremy
Hi Jeremy, >> Based on the previous discussion, I amended the conditions as >> follows: >> 1. When the device sda is mounted, use ID_PATH to determine >> that sad1 and sda are the same device. >> 2. When a new device (sdc) is detected, the warning will be >> displayed only once; the Boolean value will not be cleared until the >> device is removed. >> Therefore, the warning will only be displayed once. When a new device >> (sdc) with the same UUID is detected, and when the user repeatedly >> mounts the same device (sdc), it will still be displayed again. >> Hope this modification meets your suggestion, thank you very much~ > You could even do this without the udev_handle_dev_remove changes (which clear the dev_warn flag); we've warned about sda already, so I don't see much need to re-generate the warning later. Allow me to clarify that this warning is not displayed when device sda is mounted, nor when its partition sda1 is detected. If another device (sdc) with the same UUID is detected, this warning will be displayed only once. That is, the warning is only issued when a device (sdc) is detected that cannot be successfully mounted to the petitboot list; the regenerated warning is necessary when the user removes the device (sdc) and tries to mount it again, or mounts another device with the same UUID. > If you could send this as a proper commit, I can apply to the tree. OK, I will send commit later. Thank you for all your assistance, I really appreciate your help in solving the problem. Regards, Lulu Su --------------------------------------------------------------------------------------------------------------------------------------------------------------- This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately. ---------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/discover/udev.c b/discover/udev.c index 0c3da66..4d7dcc2 100644 --- a/discover/udev.c +++ b/discover/udev.c @@ -20,6 +20,7 @@ #include <waiter/waiter.h> #include <system/system.h> #include <process/process.h> +#include <i18n/i18n.h> #include "event.h" #include "udev.h" @@ -184,6 +185,13 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev, if (ddev) { pb_log("SKIP: %s UUID [%s] already present (as %s)\n", name, uuid, ddev->device->id); + if (strncmp(name, ddev->device->id, strlen(ddev->device->id)) && (strlen(name) > strlen(ddev->device->id))) { + char temp[] = "sdx"; + strncpy(temp, name, strlen(ddev->device->id)); + device_handler_status_info(udev->handler, + _("[%s] Duplicate filesystem has been detected (as %s); only listing the first"), + temp, ddev->device->id); + } return 0; } }