| Message ID | 1477308437-32057-1-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 24, 2016 at 01:27:17PM +0200, Juergen Gross wrote: > docs/misc/xenstore.txt states that xenstored will use "0" as a valid > transaction id after 2^32 transactions. This is not true. Remove that > statement. > > Signed-off-by: Juergen Gross <jgross@suse.com> Can you point me to the relevant code snippet? Better still I would like to see why it is the case in commit message. > --- > docs/misc/xenstore.txt | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt > index c9f4a05..ae1b6a8 100644 > --- a/docs/misc/xenstore.txt > +++ b/docs/misc/xenstore.txt > @@ -229,8 +229,6 @@ TRANSACTION_START | <transid>| > tx_id request header field. When transaction is started whole > db is copied; reads and writes happen on the copy. > It is not legal to send non-0 tx_id in TRANSACTION_START. > - Currently xenstored has the bug that after 2^32 transactions > - it will allocate the transid 0 for an actual transaction. > > TRANSACTION_END T| > TRANSACTION_END F| > -- > 2.6.6 >
On 24/10/16 13:41, Wei Liu wrote: > On Mon, Oct 24, 2016 at 01:27:17PM +0200, Juergen Gross wrote: >> docs/misc/xenstore.txt states that xenstored will use "0" as a valid >> transaction id after 2^32 transactions. This is not true. Remove that >> statement. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Can you point me to the relevant code snippet? Better still I would like > to see why it is the case in commit message. Sure: tools/xenstore/xenstored_transaction.c do_transaction_start(): ... /* Pick an unused transaction identifier. */ do { trans->id = conn->next_transaction_id; exists = transaction_lookup(conn, conn->next_transaction_id++); } while (!IS_ERR(exists)); It should be noted here that conn->next_transaction_id is initialized to be 0. So the error would occur for the first transaction, too. Juergen > >> --- >> docs/misc/xenstore.txt | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt >> index c9f4a05..ae1b6a8 100644 >> --- a/docs/misc/xenstore.txt >> +++ b/docs/misc/xenstore.txt >> @@ -229,8 +229,6 @@ TRANSACTION_START | <transid>| >> tx_id request header field. When transaction is started whole >> db is copied; reads and writes happen on the copy. >> It is not legal to send non-0 tx_id in TRANSACTION_START. >> - Currently xenstored has the bug that after 2^32 transactions >> - it will allocate the transid 0 for an actual transaction. >> >> TRANSACTION_END T| >> TRANSACTION_END F| >> -- >> 2.6.6 >> >
On 24/10/16 12:49, Juergen Gross wrote: > On 24/10/16 13:41, Wei Liu wrote: >> On Mon, Oct 24, 2016 at 01:27:17PM +0200, Juergen Gross wrote: >>> docs/misc/xenstore.txt states that xenstored will use "0" as a valid >>> transaction id after 2^32 transactions. This is not true. Remove that >>> statement. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> Can you point me to the relevant code snippet? Better still I would like >> to see why it is the case in commit message. > Sure: tools/xenstore/xenstored_transaction.c > > do_transaction_start(): > ... > /* Pick an unused transaction identifier. */ > do { > trans->id = conn->next_transaction_id; > exists = transaction_lookup(conn, > conn->next_transaction_id++); > } while (!IS_ERR(exists)); > > It should be noted here that conn->next_transaction_id is initialized > to be 0. So the error would occur for the first transaction, too. Cxenstored isn't the only xenstored implementation, and I can't see anything in the Ocaml version which mitigates this issue. Furthermore, because Ocaml's int is 31 bits or 63 bits, I suspect a 64bit oxenstored will become unusable when the transaction id hits 4 billion and an a truncation occurs when writing the id into the ring. A 32bit oxenstored only uses half the available transaction id space, and does wrap around to 0. ~Andrew
On 24/10/16 14:06, Andrew Cooper wrote: > On 24/10/16 12:49, Juergen Gross wrote: >> On 24/10/16 13:41, Wei Liu wrote: >>> On Mon, Oct 24, 2016 at 01:27:17PM +0200, Juergen Gross wrote: >>>> docs/misc/xenstore.txt states that xenstored will use "0" as a valid >>>> transaction id after 2^32 transactions. This is not true. Remove that >>>> statement. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> Can you point me to the relevant code snippet? Better still I would like >>> to see why it is the case in commit message. >> Sure: tools/xenstore/xenstored_transaction.c >> >> do_transaction_start(): >> ... >> /* Pick an unused transaction identifier. */ >> do { >> trans->id = conn->next_transaction_id; >> exists = transaction_lookup(conn, >> conn->next_transaction_id++); >> } while (!IS_ERR(exists)); >> >> It should be noted here that conn->next_transaction_id is initialized >> to be 0. So the error would occur for the first transaction, too. > > Cxenstored isn't the only xenstored implementation, and I can't see > anything in the Ocaml version which mitigates this issue. Furthermore, > because Ocaml's int is 31 bits or 63 bits, I suspect a 64bit oxenstored > will become unusable when the transaction id hits 4 billion and an a > truncation occurs when writing the id into the ring. A 32bit oxenstored > only uses half the available transaction id space, and does wrap around > to 0. Okay, so either oxenstored should be corrected by some ocaml capable developer, or I can send a patch which will limit the bug statement to oxenstored. Such a simple to fix problem should not be just mentioned in some text file, but it should be fixed! Leaving the text unmodified is no option IMHO. Juergen
On Mon, Oct 24, 2016 at 02:18:17PM +0200, Juergen Gross wrote: > On 24/10/16 14:06, Andrew Cooper wrote: > > On 24/10/16 12:49, Juergen Gross wrote: > >> On 24/10/16 13:41, Wei Liu wrote: > >>> On Mon, Oct 24, 2016 at 01:27:17PM +0200, Juergen Gross wrote: > >>>> docs/misc/xenstore.txt states that xenstored will use "0" as a valid > >>>> transaction id after 2^32 transactions. This is not true. Remove that > >>>> statement. > >>>> > >>>> Signed-off-by: Juergen Gross <jgross@suse.com> > >>> Can you point me to the relevant code snippet? Better still I would like > >>> to see why it is the case in commit message. > >> Sure: tools/xenstore/xenstored_transaction.c > >> > >> do_transaction_start(): > >> ... > >> /* Pick an unused transaction identifier. */ > >> do { > >> trans->id = conn->next_transaction_id; > >> exists = transaction_lookup(conn, > >> conn->next_transaction_id++); > >> } while (!IS_ERR(exists)); > >> > >> It should be noted here that conn->next_transaction_id is initialized > >> to be 0. So the error would occur for the first transaction, too. > > > > Cxenstored isn't the only xenstored implementation, and I can't see > > anything in the Ocaml version which mitigates this issue. Furthermore, > > because Ocaml's int is 31 bits or 63 bits, I suspect a 64bit oxenstored > > will become unusable when the transaction id hits 4 billion and an a > > truncation occurs when writing the id into the ring. A 32bit oxenstored > > only uses half the available transaction id space, and does wrap around > > to 0. > > Okay, so either oxenstored should be corrected by some ocaml capable > developer, or I can send a patch which will limit the bug statement to > oxenstored. > A patch to spell out limitation on oxenstored works for me. Wei.
On Mon, Oct 24, 2016 at 01:27:17PM +0200, Juergen Gross wrote: > docs/misc/xenstore.txt states that xenstored will use "0" as a valid > transaction id after 2^32 transactions. This is not true. Remove that > statement. > > Signed-off-by: Juergen Gross <jgross@suse.com> Applied.
diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt index c9f4a05..ae1b6a8 100644 --- a/docs/misc/xenstore.txt +++ b/docs/misc/xenstore.txt @@ -229,8 +229,6 @@ TRANSACTION_START | <transid>| tx_id request header field. When transaction is started whole db is copied; reads and writes happen on the copy. It is not legal to send non-0 tx_id in TRANSACTION_START. - Currently xenstored has the bug that after 2^32 transactions - it will allocate the transid 0 for an actual transaction. TRANSACTION_END T| TRANSACTION_END F|
docs/misc/xenstore.txt states that xenstored will use "0" as a valid transaction id after 2^32 transactions. This is not true. Remove that statement. Signed-off-by: Juergen Gross <jgross@suse.com> --- docs/misc/xenstore.txt | 2 -- 1 file changed, 2 deletions(-)