[libvirt] [PATCH 0/2] Two trivial fixes

*** BLURB HERE *** Michal Privoznik (2): virNetServerClientNewPostExecRestart: Drop useless typecasts virNetServerClientNewPostExecRestart: Avoid align problems src/rpc/virnetserverclient.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) -- 2.8.1

In this function, @id is defined as unsigned long long. When passing this variable to virJSONValueObjectGetNumberUlong(), well address of this variable, it's typecasted to ull*. There is no need for that. It's a same story with @nrequests_max. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserverclient.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index d38f421..d3a3a18 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -485,7 +485,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec return NULL; } if (virJSONValueObjectGetNumberUint(object, "nrequests_max", - (unsigned int *)&nrequests_max) < 0) { + &nrequests_max) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing nrequests_client_max field in JSON state document")); return NULL; @@ -501,8 +501,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec /* no ID found in, a new one must be generated */ id = virNetServerNextClientID((virNetServerPtr) opaque); } else { - if (virJSONValueObjectGetNumberUlong(object, "id", - (unsigned long long *) &id) < 0) { + if (virJSONValueObjectGetNumberUlong(object, "id", &id) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed id field in JSON state document")); return NULL; -- 2.8.1

On 05/05/16 09:30, Michal Privoznik wrote:
In this function, @id is defined as unsigned long long. When passing this variable to virJSONValueObjectGetNumberUlong(), well address of this variable, it's typecasted to ull*. There is no need for that. It's a same story with @nrequests_max.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserverclient.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index d38f421..d3a3a18 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -485,7 +485,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec return NULL; } if (virJSONValueObjectGetNumberUint(object, "nrequests_max", - (unsigned int *)&nrequests_max) < 0) { + &nrequests_max) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing nrequests_client_max field in JSON state document")); return NULL; @@ -501,8 +501,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec /* no ID found in, a new one must be generated */ id = virNetServerNextClientID((virNetServerPtr) opaque); } else { - if (virJSONValueObjectGetNumberUlong(object, "id", - (unsigned long long *) &id) < 0) { + if (virJSONValueObjectGetNumberUlong(object, "id", &id) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed id field in JSON state document")); return NULL;
ACK Erik

I've noticed this while trying to compile libvirt on my arm box. CC rpc/libvirt_net_rpc_server_la-virnetserverclient.lo rpc/virnetserverclient.c: In function 'virNetServerClientNewPostExecRestart': rpc/virnetserverclient.c:516:45: error: cast increases required alignment of target type [-Werror=cast-align] (long long *) ×tamp) < 0) { ^ cc1: all warnings being treated as errors Problem is, @timestap is defined as time_t which is 32 bits long, and we are typecasting it to long long which is 64bits long. Solution is to make @timestamp type of long long. But that could overflow when passing the variable to virNetServerClientNewInternal(). Well, we can do this and hope to find a better solution for this till 19 January 2038 (yes, time_t on my arm box is signed long int). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserverclient.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index d3a3a18..271930f 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -472,7 +472,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec bool readonly; unsigned int nrequests_max; unsigned long long id; - time_t timestamp; + long long timestamp; if (virJSONValueObjectGetNumberInt(object, "auth", &auth) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -511,8 +511,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec if (!virJSONValueObjectHasKey(object, "conn_time")) { timestamp = 0; } else { - if (virJSONValueObjectGetNumberLong(object, "conn_time", - (long long *) ×tamp) < 0) { + if (virJSONValueObjectGetNumberLong(object, "conn_time", ×tamp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed conn_time field in JSON " "state document")); -- 2.8.1

On 05/05/16 09:30, Michal Privoznik wrote:
I've noticed this while trying to compile libvirt on my arm box.
CC rpc/libvirt_net_rpc_server_la-virnetserverclient.lo rpc/virnetserverclient.c: In function 'virNetServerClientNewPostExecRestart': rpc/virnetserverclient.c:516:45: error: cast increases required alignment of target type [-Werror=cast-align] (long long *) ×tamp) < 0) { ^ cc1: all warnings being treated as errors
Problem is, @timestap is defined as time_t which is 32 bits long, and we are typecasting it to long long which is 64bits long. Solution is to make @timestamp type of long long. But that could overflow when passing the variable to virNetServerClientNewInternal(). Well, we can do this and hope to find a better solution for this till 19 January 2038 (yes, time_t on my arm box is signed long int).
Hmm, why don't we then represent the timestamp as long long in the first place, rather than having it as time_t. That way, no overflow as you mention could happen and the only problem for your machine then would be to cope with the 32bit timestamp time function returns, which will break libvirt and anything else anyway, but at least in an expected manner. Erik
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserverclient.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index d3a3a18..271930f 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -472,7 +472,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec bool readonly; unsigned int nrequests_max; unsigned long long id; - time_t timestamp; + long long timestamp;
if (virJSONValueObjectGetNumberInt(object, "auth", &auth) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -511,8 +511,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec if (!virJSONValueObjectHasKey(object, "conn_time")) { timestamp = 0; } else { - if (virJSONValueObjectGetNumberLong(object, "conn_time", - (long long *) ×tamp) < 0) { + if (virJSONValueObjectGetNumberLong(object, "conn_time", ×tamp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed conn_time field in JSON " "state document"));

On 05.05.2016 11:05, Erik Skultety wrote:
On 05/05/16 09:30, Michal Privoznik wrote:
I've noticed this while trying to compile libvirt on my arm box.
CC rpc/libvirt_net_rpc_server_la-virnetserverclient.lo rpc/virnetserverclient.c: In function 'virNetServerClientNewPostExecRestart': rpc/virnetserverclient.c:516:45: error: cast increases required alignment of target type [-Werror=cast-align] (long long *) ×tamp) < 0) { ^ cc1: all warnings being treated as errors
Problem is, @timestap is defined as time_t which is 32 bits long, and we are typecasting it to long long which is 64bits long. Solution is to make @timestamp type of long long. But that could overflow when passing the variable to virNetServerClientNewInternal(). Well, we can do this and hope to find a better solution for this till 19 January 2038 (yes, time_t on my arm box is signed long int).
Hmm, why don't we then represent the timestamp as long long in the first place, rather than having it as time_t. That way, no overflow as you mention could happen and the only problem for your machine then would be to cope with the 32bit timestamp time function returns, which will break libvirt and anything else anyway, but at least in an expected manner.
I don't know. It's not me who wrote the code :-P But sure, we could do that. It's going to be just a broadening of my patch anyway. Michal

On 05/05/16 12:26, Michal Privoznik wrote:
On 05.05.2016 11:05, Erik Skultety wrote:
On 05/05/16 09:30, Michal Privoznik wrote:
I've noticed this while trying to compile libvirt on my arm box.
CC rpc/libvirt_net_rpc_server_la-virnetserverclient.lo rpc/virnetserverclient.c: In function 'virNetServerClientNewPostExecRestart': rpc/virnetserverclient.c:516:45: error: cast increases required alignment of target type [-Werror=cast-align] (long long *) ×tamp) < 0) { ^ cc1: all warnings being treated as errors
Problem is, @timestap is defined as time_t which is 32 bits long, and we are typecasting it to long long which is 64bits long. Solution is to make @timestamp type of long long. But that could overflow when passing the variable to virNetServerClientNewInternal(). Well, we can do this and hope to find a better solution for this till 19 January 2038 (yes, time_t on my arm box is signed long int).
Hmm, why don't we then represent the timestamp as long long in the first place, rather than having it as time_t. That way, no overflow as you mention could happen and the only problem for your machine then would be to cope with the 32bit timestamp time function returns, which will break libvirt and anything else anyway, but at least in an expected manner.
I don't know. It's not me who wrote the code :-P But sure, we could do that. It's going to be just a broadening of my patch anyway.
Michal
I guess I wasn't explicit in my last reply, you have an ACK if you also squash changes mentioned above into your patch. Erik
participants (2)
-
Erik Skultety
-
Michal Privoznik