| Message ID | 20210709021956.21244-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. What do you mean by 'iso file has been installed'? Keep in mind that the system status log is not very obvious, so I'm not sure if this will help the user much. This will also create a status message on the main menu screen, but that may be replaced quickly if another event occurs in quick sucession. Could you add a little detail on what issue the user is seeing here? > @@ -184,6 +185,11 @@ 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))) { When would this check ever fail? We'd have multiple block devices with the same kernel name. + ddev = discover_device_create(udev- >handler, NULL, name); Why create the device here? We're about to skip discovery. There are status logging functions that do not require a dev pointer. + device_handler_status_dev_info(udev- >handler, ddev, + _("The list doesn't support displaying the same mount file")); This log message seems overly specific, and doesn't really describe the situation (we're seeing a duplicate filesystem appear); we'd hit this case on a few different situations, and the term 'mount file' doesn't apply to all of those. How about: "%s: A duplicate filesystem has been detected (same as %s); only listing the first" name, ddev->device->id - would that work for your case? Regards, Jeremy
Hi Lulu, [Looks like I made a mess of the previous reply; sending again with the same content, this time with proper formatting] > 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. What do you mean by 'iso file has been installed'? Keep in mind that the system status log is not very obvious, so I'm not sure if this will help the user much. This will also create a status message on the main menu screen, but that may be replaced quickly if another event occurs in quick sucession. Could you add a little detail on what issue the user is seeing here? > @@ -184,6 +185,11 @@ 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))) { When would this check ever fail? We'd have multiple block devices with the same kernel name. > + ddev = discover_device_create(udev->handler, NULL, name); Why create the device here? We're about to skip discovery. There are status logging functions that do not require a dev pointer. > + device_handler_status_dev_info(udev-> handler, ddev, > + _("The list doesn't support displaying the same mount file")); This log message seems overly specific, and doesn't really describe the situation (we're seeing a duplicate filesystem appear); we'd hit this case on a few different situations, and the term 'mount file' doesn't apply to all of those. How about: "%s: A duplicate filesystem has been detected (same as %s); only listing the first" name, ddev->device->id - would that work for your case? Regards, Jeremy
Hi Jeremy, > [Looks like I made a mess of the previous reply; sending again with > the same content, this time with proper formatting] >> 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. > What do you mean by 'iso file has been installed'? > Keep in mind that the system status log is not very obvious, so I'm not sure if this will help the user much. This will also create a status message on the main menu screen, but that may be replaced quickly if another event occurs in quick sucession. Could you add a little detail on what issue the user is seeing here? Forward user description: " Tester has found during the Virtual media Install when he mounted USB and VIrtual Media only 1 USB option is visible in the petitboot for install. It should have another label which user can select for install. Same was showing earlier properly looks like there was some change which has caused this. " But in my understanding, if two devices have the same UUID, the list will only show the first one, right? If I make changes based on user requirements, this change will affect all platforms. I don't confirm the necessity of this feature, so I want to add a warning, at least to remind users when they mount the same file, instead of letting them misunderstand that the device is broken. Could you give me some suggestions? >> @@ -184,6 +185,11 @@ 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))) { > When would this check ever fail? We'd have multiple block devices with the same kernel name. When a device using the UUID already exists, but the device name is different (e.g. Already exists device1: sda, New device2:sdc ) >> + ddev = discover_device_create(udev->handler, NULL, name); > Why create the device here? We're about to skip discovery. > There are status logging functions that do not require a dev pointer. This is my old warning: "[sdc] The list doesn’t support displaying the same mount file" I use this function to display warnings ( https://github.com/open-power/petitboot/blob/f7f98227ae991497ffe361eec3b7a9d1b2d94db4/discover/device-handler.h#L124 ) Besides creating sdc, could you suggest me a way to make this function display sdc? >> + device_handler_status_dev_info(udev-> handler, ddev, >> + _("The list doesn't support displaying the same mount file")); > This log message seems overly specific, and doesn't really describe the situation (we're seeing a duplicate filesystem appear); we'd hit this case on a few different situations, and the term 'mount file' doesn't apply to all of those. > How about: > "%s: A duplicate filesystem has been detected (same as %s); only listing the first" name, ddev->device->id > - would that work for your case? Thank you for your suggestion, I will use this suggestion to modify the log message. >Regards, >Jeremy 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, > Forward user description: > " Tester has found during the Virtual media Install when he mounted > USB and VIrtual Media only 1 USB option is visible in the petitboot > for install. OK, so the test involved adding a USB device with some filesystem image, plus a virtual media device providing the exact same filesystem image then, is that correct? Is there an expectation that both devices appear? [For a bit of background: the UUID check prevents multiple devices appearing in multipath setups; where it's actually the same physical filesystem being added under separate Linux block devices. It's also fairly important that the UUIDs are unique, as they're commonly referenced in bootloader configurations, so we really should ignore the second instance of a duplicated FS UUID] > Could you give me some suggestions? I'm OK with the message, but to look at the bigger picture here: 1) is the presence of only one device in the list actually a problem? Is it stopping the user from achieving a particular goal? and: 2) does the message help with that? > If I make changes based on user requirements, this change will affect > all platforms. I don't confirm the necessity of this feature, so I > want to add a warning, at least to remind users when they mount the > same file, instead of letting them misunderstand that the device is > broken. OK, makes sense. > > When would this check ever fail? We'd have multiple block devices > > with the same kernel name. > > When a device using the UUID already exists, but the device name is > different (e.g. Already exists device1: sda, New device2:sdc ) Yep, but my point is that the device name will always be different - otherwise the kernel has created two block devices with identical names. So, the conditional in the if statement will always evaluate to true. Unless we have a bug elsewhere? > This is my old warning: "[sdc] The list doesn’t support displaying > the same mount file" > I use this function to display warnings ( > https://github.com/open-power/petitboot/blob/f7f98227ae991497ffe361eec3b7a9d1b2d94db4/discover/device-handler.h#L124 > ) > Besides creating sdc, could you suggest me a way to make this > function display sdc? Just include the device name in the string that you're generating. There's nothing special about the _dev_info function over the plain _info version, just that the former adds the device name prefix. > > "%s: A duplicate filesystem has been detected (same as %s); only > > listing the first" > name, ddev->device->id > > > - would that work for your case? > > Thank you for your suggestion, I will use this suggestion to modify > the log message. OK, great! If you wanted to keep it really simple, you could just change the pb_log() call there to a device_handler_status_info(); it's already giving an indication of the duplicate UUID. Regards, Jeremy
Hi Jeremy, > > Forward user description: > > " Tester has found during the Virtual media Install when he mounted > > USB and VIrtual Media only 1 USB option is visible in the petitboot > > for install. > OK, so the test involved adding a USB device with some filesystem image, plus a virtual media device providing the exact same filesystem image then, is that correct? Yes! > Is there an expectation that both devices appear? Under normal circumstances, yes. But I think this time they forgot to confirm if they installed the file with the same UUID, so they said "Same was showing earlier properly looks like there was some change which has caused this." > [For a bit of background: the UUID check prevents multiple devices appearing in multipath setups; where it's actually the same physical filesystem being added under separate Linux block devices. It's also fairly important that the UUIDs are unique, as they're commonly referenced in bootloader configurations, so we really should ignore the second instance of a duplicated FS UUID] Thank you very much for sharing knowledge. > I'm OK with the message, but to look at the bigger picture here: > > 1) is the presence of only one device in the list actually a problem? Is it stopping the user from achieving a particular goal? and: They didn’t mention this part, but I think they just wondered why the list didn't respond after mounting the file. > 2) does the message help with that? They agreed to add a warning message to let users know that a file with the same UUID is installed to make it clearer. > > When a device using the UUID already exists, but the device name is > > different (e.g. Already exists device1: sda, New device2:sdc ) > Yep, but my point is that the device name will always be different > - otherwise the kernel has created two block devices with identical names. So, the conditional in the if statement will always evaluate to true. Unless we have a bug elsewhere? Sorry, correct the previous description, this condition is to prevent sda1 from displaying a warning. In pb-discover.log: [09:12:05] SKIP: sda1 UUID [2020-04-04-04-38-27-00] already present (as sda) [09:15:50] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda) > Just include the device name in the string that you're generating. > There's nothing special about the _dev_info function over the plain _info version, just that the former adds the device name prefix. > > OK, great! > > If you wanted to keep it really simple, you could just change the > pb_log() call there to a device_handler_status_info(); it's already giving an indication of the duplicate UUID. Thanks for your suggestions, I will try these and upload the patch again. 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, > Sorry, correct the previous description, this condition is to prevent > sda1 from displaying a warning. > > In pb-discover.log: > [09:12:05] SKIP: sda1 UUID [2020-04-04-04-38-27-00] already present (as sda) > [09:15:50] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda) OK, something fishy is happening here. You shouldn't see the filesystem UUID appear on both sda and sda1. Is this an installer filesystem image that I can take a look at? And can you send the pb-discover.log when petitboot is run in debug mode? Thanks, Jeremy
Hi Jeremy, > > Sorry, correct the previous description, this condition is to prevent > > sda1 from displaying a warning. > > > In pb-discover.log: > > [09:12:05] SKIP: sda1 UUID [2020-04-04-04-38-27-00] already present > > (as sda) [09:15:50] SKIP: sdc UUID [2020-04-04-04-38-27-00] already > > present (as sda) > OK, something fishy is happening here. You shouldn't see the filesystem UUID appear on both sda and sda1. Is this an installer filesystem image that I can take a look at? The filesystem image of the installer used: rhel-8.2. > And can you send the pb-discover.log when petitboot is run in debug mode? Okay, I will send pb-discover.log to you. In addition, how does petitboot select the debug mode at runtime? Or where can I refer to it? Thanks, -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, > > OK, something fishy is happening here. You shouldn't see the > > filesystem UUID appear on both sda and sda1. Is this an installer > > filesystem image that I can take a look at? > > The filesystem image of the installer used: rhel-8.2. > > > And can you send the pb-discover.log when petitboot is run in debug > > mode? > > Okay, I will send pb-discover.log to you. Got it, thanks. > In addition, how does petitboot select the debug mode at runtime? Or > where can I refer to it? I see you've sorted this, but for anyone else following along: this is set in the platform configuration. On OpenPOWER machines, this is the 'petitboot,debug?' var, which you can set with: nvram --update-config petitboot,debug?=true Back to the log: It looks like you actually have *four* devices with a duplicate filesystem UUID: sda, sda1, sdc, and sdc1. The duplication within the same devices appears to be due to the way the installer image is constructed - there's an is09660 filesystem on the sda adevice, and also a DOS partition table. That partition table contains a partition which then references the same iso9660 data as the underlying (non-partition) device. So, the outcome of this is that we definitely don't want to remove the duplicate supression; it will make the standard case (where only one installer image is present) appear as though you have two installers plugged-in! Let's proceed with the message approach then. Cheers, Jeremy
Hi Jeremy, > I see you've sorted this, but for anyone else following along: this is set in the platform configuration. On OpenPOWER machines, this is the 'petitboot,debug?' var, which you can set with: > > nvram --update-config petitboot,debug?=true Thank you for your sharing. > Back to the log: > > It looks like you actually have *four* devices with a duplicate filesystem UUID: sda, sda1, sdc, and sdc1. > > The duplication within the same devices appears to be due to the way the installer image is constructed - there's an is09660 filesystem on the sda adevice, and also a DOS partition table. That partition table contains a partition which then references the same iso9660 data as the underlying (non-partition) device. > > So, the outcome of this is that we definitely don't want to remove the duplicate supression; it will make the standard case (where only one installer image is present) appear as though you have two installers plugged-in! No wonder! Thanks for the explanation. > Let's proceed with the message approach then. There is also an unusual thing, it will repeatedly detect sdc (device is virtual media). This doesn't affect the increase of warning messages, but is this a common phenomenon? In pb-discover.log: [12:21:14] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda) [12:21:15] SKIP: sdc1 UUID [2020-04-04-04-38-27-00] already present (as sda) [12:21:15] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda) [12:21:15] SKIP: sdc UUID [2020-04-04-04-38-27-00] already present (as sda) Through your knowledge sharing and guidance, I re-modified the code: + 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); + } Due to the repeated detection, the warning message will only be displayed when sd** is detected. The judgment condition is: in the case of the same UUID, the device name is different from the existing device name, and the length of the device name is greater than the existing device name. This is the message displayed in the system status log: " [sdc] Duplicate filesystem has been detected (as sda); only listing the first " Need to modify the description? The number of strings is the limit displayed on one line. Other logs are described by one line, tried to wrap, but it will cause the format to run away... Thanks, -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..e6f67e6 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,11 @@ 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))) { + ddev = discover_device_create(udev->handler, NULL, name); + device_handler_status_dev_info(udev->handler, ddev, + _("The list doesn't support displaying the same mount file")); + } return 0; } }