BTW, we usually use unsigned long for bit flags. So, DeviceFound in many places should be also changed to unsigned long?

@keszybz Thank you for the comments. Updated. Now 'not-found' is dropped. Please take another look. Thank you.

Actually I'm wondering if we really need DeviceFound to be a bit flag...

PS: it would have been nice if you had put me on Cc BTW

@fbuihuu So, you mean that Device.found is always set at most one bit?

I think so.

"found" is basically a state: either the device has been introduced by a mount unit, a swap unit or by a udev event but can't be all of them at the same time.

It can be first seen by a mount unit then afterward a udev event confirms the existence of the device but I don't think there's a point in keeping the info that the device was first seen by a mount unit.

Just out of curiosity, did you encounter a bug ?

Just out of curiosity, did you encounter a bug ?

@fbuihuu No. I thought bit fields should be converted to multiple words theoretically.

It can be first seen by a mount unit then afterward a udev event confirms the existence of the device but I don't think there's a point in keeping the info that the device was first seen by a mount unit.

Hmm, then in such a case, which bit should be serialized? Or the DEVICE_FOUND_MOUNT flag is already removed in such a case?

No. I thought bit fields should be converted to multiple words theoretically.

You're correct.

Hmm, then in such a case, which bit should be serialized? Or the DEVICE_FOUND_MOUNT flag is already removed in such a case?

It's not removed but I think there's no point in keeping this bit once DEVICE_FOUND_UDEV is raised.

It's not removed but I think there's no point in keeping this bit once DEVICE_FOUND_UDEV is raised.

OK. So, this PR makes something redundant, but can treat such a case, which is not handled previously.

Indeed but my point is rather than fixing the serialization of the bit flags, do the right thing (if I'm right) and drop the use of the bit flag completely.

If you prefer, I can prep a patch.

Indeed but my point is rather than fixing the serialization of the bit flags, do the right thing (if I'm right) and drop the use of the bit flag completely.

@fbuihuu Yeah, it may be another approach to fix such a theoretical issue. But I cannot judge which is a better solution...

If the bit flag is really unnecessary, dropping its usage seems the best way IMHO.

@poettering could you confirm/invalidate that the use of a bit flag for "d->found" is unneeded ?

device_update_found_by_sysfs and device_update_found_by_name set different bits. See my follow-up commit I'll push here.

@keszybz Thank you for the follow-up commit. But @fbuihuu's point is that if DEVICE_FOUND_UDEV is set, then it is not necessary to serialize DEVICE_FOUND_MOUNT or DEVICE_FOUND_SWAP. Though I am not sure it is true or not...

I think it's better to serialize it, so that the state after daemon-reload or daemon-reexec is exactly the same as before.

@keszybz I think there's a misunderstanding: the question is not about serializing or not found. The question is "does found need to be a bit flag`.

I dont think so.

device_update_found_one makes use of the bits.

Again, I think there's no point in doing so.

OK, so can you make a patch that drops the bitness, and turns it into just a single value field without losing functionality? I don't see how to do this, and without that our discussion is going around in circles.

BTW, we usually use unsigned long for bit flags. So, DeviceFound in many places should be also changed to unsigned long?

Hmm, there's not clear theme what we use actually, afaics... When the kernel defines somethings we use what the kernel defines (for example the clone flags), but other than that I am not sure what we should use... I figure either uint64_t (so that we know how how many bits we have) or the enum type the bit flags are defined as

If the bit flag is really unnecessary, dropping its usage seems the best way IMHO.

@poettering could you confirm/invalidate that the use of a bit flag for "d->found" is unneeded ?

They are necessary. The bit flag encodes where we have seen the device as up so far. This is necessary to deal with races between /proc/swaps or /proc/self/mountinfo on one hand and udev on the other. Note that udev rules can take any time they want to finish, hence it might very well happen that a file system is already mounted before the udev rules finished, as well as the opposite. Hence this is a bit field, so that we always know whether udev has announced it, or whether it is already announced as used in those /proc files...

So I merged this one now, since it at fixes the bitmap confusion, but this needs more love still. The "DEVICE_FOUND_UDEV_DB" flag shouldn't be there.

I am not sure I grok everything fully, but I think we might not need to serialize the flag set at all, and always use what the kernel reports, but then when we notice that the new flag set is not in sync with the unit state enum we enqueue another state change, but do that in a defer event. That means that after deserialization we'll initially be in the old state, but pretty soon on react to the new state, and thus trigger the usual deps...

I filed #8832 to remind us this needs more love before the release.

@poettering sorry but I still don't see why the flag needs to be a bit flag. Just to make sure there's no misunderstanding: I'm not saying that the flag is not needed, I'm disputing the fact that the flag uses a bit flag.

I see the need to encode where the device has been seen up for the first time. But in my understanding, once udev has announced a device, there's no need to keep the information that the device has been first seen in /proc/swaps or /proc/self/mountinfo anymore.

So IOW when d->found = DEVICE_FOUND_SWAP | DEVICE_FOUND_UDEV, I don't see the need to keep DEVICE_FOUND_SWAP

@fbuihuu /proc/swaps + /proc/self/mountinfo on one hand and udev on the other may be out of sync in both "directions", one can list a device the other doesn't know and vice versa. Examples:

1. /proc/self/mountinfo can show stuff udev hasn't announced yet. This happens for example when some script sets up /dev/loop0, then mounts it right away, without waiting for udev. In that case mountinfo will show the device available before udev does. This is the "plug" case, but there's also a case here on "unplug": you pull a mounted usb device from the machine. In this case the mount is not removed, will continue to show the device even though udevd will show it as disappeared.

2. udev can show stuff /proc/self/mountinfo hasn't shown yet. This one I figure is obvious...

So yeah, we'll typically see 'mount|udev', we'll also see the 'udev' flag alone, and the 'mount' flag alone, as well as neither set. Since all four combinations are seen regularly its a bitfield.

And something similar applies to swaps too... A script could set up block device, format it and swapon it, all before udev announced it.

Hopefully we'll never see both the 'swap' and 'mount' flags on for the same devices, because that'd be weird, but there's no reason to rule that out, who knows, maybe the kernel invents a file system that can also act as swap one day...