udev_queue_get_seqnum_is_finished

udev_queue_get_seqnum_is_finished

[Alan Jenkins]

I'm puzzled by this function:


int udev_queue_get_seqnum_is_finished(struct udev_queue *udev_queue, unsigned long long int seqnum)
{
	char filename[UTIL_PATH_SIZE];
	struct stat statbuf;

	if (udev_queue = NULL)
		return -EINVAL;
	/* if we have not seen this seqnum, check if it is/was already queued */
	if (seqnum < udev_queue->last_seen_udev_seqnum) {
		udev_queue_get_udev_seqnum(udev_queue);
		if (seqnum < udev_queue->last_seen_udev_seqnum)
			return 0;
	}



Shouldn't the test be (seqnum > udev_queue->last_seen_udev_seqnum) ?
I.e. "greater than" instead of "less than".

If the code is right, then why does it need to re-check after calling
udev_queue_get_udev_seqnum() ?  The udev seqnum can only increase, so if
the first test is true then the second cannot fail either.

Thanks
Alan

Re: udev_queue_get_seqnum_is_finished

[Kay Sievers]

On Thu, Apr 23, 2009 at 11:39, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> I'm puzzled by this function:
>
>
> int udev_queue_get_seqnum_is_finished(struct udev_queue *udev_queue, unsigned long long int seqnum)
> {
>        char filename[UTIL_PATH_SIZE];
>        struct stat statbuf;
>
>        if (udev_queue = NULL)
>                return -EINVAL;
>        /* if we have not seen this seqnum, check if it is/was already queued */
>        if (seqnum < udev_queue->last_seen_udev_seqnum) {
>                udev_queue_get_udev_seqnum(udev_queue);
>                if (seqnum < udev_queue->last_seen_udev_seqnum)
>                        return 0;
>        }
>
>
> Shouldn't the test be (seqnum > udev_queue->last_seen_udev_seqnum) ?
> I.e. "greater than" instead of "less than".

Sounds like.

> If the code is right, then why does it need to re-check after calling
> udev_queue_get_udev_seqnum() ?  The udev seqnum can only increase, so if
> the first test is true then the second cannot fail either.

The udev_queue->last_seen_udev_seqnum can change with the
udev_queue_get_udev_seqnum() call, hence the second check. It will
work when the <  turns into a >, I guess. :)

Kay

Re: udev_queue_get_seqnum_is_finished

[Kay Sievers]

On Thu, Apr 23, 2009 at 13:20, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Thu, Apr 23, 2009 at 11:39, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:

>> Shouldn't the test be (seqnum > udev_queue->last_seen_udev_seqnum) ?
>> I.e. "greater than" instead of "less than".
>
> Sounds like.

Care to check, if that makes sense now:
  http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;hå3ae78546a535cc9288a190680fb1eb88c6ea7e

Thanks,
Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: udev_queue_get_seqnum_is_finished

[Alan Jenkins]

Kay Sievers wrote:
> On Thu, Apr 23, 2009 at 13:20, Kay Sievers <kay.sievers@vrfy.org> wrote:
>   
>> On Thu, Apr 23, 2009 at 11:39, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>     
>
>   
>>> Shouldn't the test be (seqnum > udev_queue->last_seen_udev_seqnum) ?
>>> I.e. "greater than" instead of "less than".
>>>       
>> Sounds like.
>>     
>
> Care to check, if that makes sense now:
>   http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;hŒ3ae78546a535cc9288a190680fb1eb88c6ea7e
>   

Yes, that makes sense now.  Except for the comment, which seems to
accurately describes the old, confused code :-)

/* if we have not seen this seqnum, check if it is/was already queued */

to match the new code, I think it would be

/* if we have seen this seqnum, check if it is still queued */


Ta
Alan

Re: udev_queue_get_seqnum_is_finished

[Kay Sievers]

On Thu, Apr 23, 2009 at 14:43, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:

> Yes, that makes sense now.

Cool.

> Except for the comment, which seems to
> accurately describes the old, confused code :-)
>
> /* if we have not seen this seqnum, check if it is/was already queued */
>
> to match the new code, I think it would be
>
> /* if we have seen this seqnum, check if it is still queued */

It means, if we have not seen it (seq > last_seen), update (last_seen)
to the current number, and check again. The return 0 means, don't
bother the look at the queue dir, if we find out we don't even queued
this event. Does that sound wrong? To me it sounds ok. :)

Thanks,
Kay