Small series to fix a bug in get_spec_node().

Patch 1 is turning several function parameters into const in order to
avoid having to cast away the const attribute in get_spec_node().

Patch 2 is the fix, which is a backport candidate.

Alternatives to this series would be:

- merge the patches into one patch and backport that
- swap the sequence of the patches in order to have less code churn
  when backporting, but re-adding the cast from const to non-cont,
  while backporting only the fix
- leave the series as is and backport both patches
- leave the series as is and use V1 of patch 2 for the backport

Juergen Gross (2):
  tools/xenstore: add const to the return type of  canonicalize()
  tools/xenstore: fix get_spec_node()

 tools/xenstore/xenstored_core.c  | 27 ++++++++++++++++-----------
 tools/xenstore/xenstored_core.h  |  3 ++-
 tools/xenstore/xenstored_watch.c | 16 ++++++++++------
 3 files changed, 28 insertions(+), 18 deletions(-)

-- 
2.35.3

Hi Juergen,

On 22/07/2023 09:16, Juergen Gross wrote:
> Small series to fix a bug in get_spec_node().
> 
> Patch 1 is turning several function parameters into const in order to
> avoid having to cast away the const attribute in get_spec_node().
> 
> Patch 2 is the fix, which is a backport candidate.
> 
> Alternatives to this series would be:
> 
> - merge the patches into one patch and backport that

AFAIU, this would have the same outcome as applying the two patches but 
it would circumvent the "we don't backport clean-up to stable tree" and 
lose the history.

> - swap the sequence of the patches in order to have less code churn
>    when backporting, but re-adding the cast from const to non-cont,
>    while backporting only the fix

I am not sure I understand this. If you swap the patch, wouldn't the 
const-away cast be needed to compile and allow bisection?

> - leave the series as is and backport both patches

I am split. On one hand, it would be good to harden older Xenstored, on 
the other hand this is not strictly necessary to fix it. So possibly not 
a good option compare to the others.

> - leave the series as is and use V1 of patch 2 for the backpor
It would be my preference. But I would also be happy with swapping patch 
#1 and patch #2 if there is a desire to have a clean cherry-pick.

Cheers,

-- 
Julien Grall

On 22.07.23 18:54, Julien Grall wrote:
> Hi Juergen,
> 
> On 22/07/2023 09:16, Juergen Gross wrote:
>> Small series to fix a bug in get_spec_node().
>>
>> Patch 1 is turning several function parameters into const in order to
>> avoid having to cast away the const attribute in get_spec_node().
>>
>> Patch 2 is the fix, which is a backport candidate.
>>
>> Alternatives to this series would be:
>>
>> - merge the patches into one patch and backport that
> 
> AFAIU, this would have the same outcome as applying the two patches but it would 
> circumvent the "we don't backport clean-up to stable tree" and lose the history.

Correct. With your suggested patch modifying get_strings() this would grow even
larger, though.

> 
>> - swap the sequence of the patches in order to have less code churn
>>    when backporting, but re-adding the cast from const to non-cont,
>>    while backporting only the fix
> 
> I am not sure I understand this. If you swap the patch, wouldn't the const-away 
> cast be needed to compile and allow bisection?

Yes. This suggestion would be one way to use the const-away cast for the
backports. It would basically enable us to have the same (or very similar)
patch in unstable and the stable branches.

> 
>> - leave the series as is and backport both patches
> 
> I am split. On one hand, it would be good to harden older Xenstored, on the 
> other hand this is not strictly necessary to fix it. So possibly not a good 
> option compare to the others.

I agree.

> 
>> - leave the series as is and use V1 of patch 2 for the backpor
> It would be my preference. But I would also be happy with swapping patch #1 and 
> patch #2 if there is a desire to have a clean cherry-pick.

I'm fine both ways.


Juergen