[Spice-devel] [PATCH spice-server 2/2] char device: Ensure public function parameters are not NULL
Pavel Grunt
pgrunt at redhat.com
Fri Nov 6 01:16:00 PST 2015
Hi Victor,
my intention was to not change the behaviour, but it seems
these "spice_return_if_fail(client != NULL)" may change it.
If you don't mind I would drop this patch from the series and solved it
separately.
On Fri, 2015-11-06 at 08:47 +0100, Victor Toso wrote:
> Hi,
>
> On Thu, Nov 05, 2015 at 04:58:32PM +0100, Pavel Grunt wrote:
> > ---
> > server/char_device.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> > server/reds.c | 2 ++
> > server/spicevmc.c | 6 ++++++
> > 3 files changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/server/char_device.c b/server/char_device.c
> > index 54357f0..00ed4d9 100644
> > --- a/server/char_device.c
> > +++ b/server/char_device.c
> > @@ -376,6 +376,9 @@ void
> > spice_char_device_send_to_client_tokens_add(SpiceCharDeviceState *dev,
> > {
> > SpiceCharDeviceClientState *dev_client;
> >
> > + spice_return_if_fail(dev != NULL);
> > + spice_return_if_fail(client != NULL);
>
> The check for client is not necessary (I should not abort I think).
> Logic bellow warn and return if spice_char_device_client_find returns
> NULL (which it should for NULL client)
Ok, I was hiding the error message
>
> > +
> > dev_client = spice_char_device_client_find(dev, client);
> >
> > if (!dev_client) {
> > @@ -391,6 +394,9 @@ void
> > spice_char_device_send_to_client_tokens_set(SpiceCharDeviceState *dev,
> > {
> > SpiceCharDeviceClientState *dev_client;
> >
> > + spice_return_if_fail(dev != NULL);
> > + spice_return_if_fail(client != NULL);
>
> Same.
>
Ok
> > +
> > dev_client = spice_char_device_client_find(dev, client);
> >
> > if (!dev_client) {
> > @@ -573,6 +579,8 @@ SpiceCharDeviceWriteBuffer
> > *spice_char_device_write_buffer_get(SpiceCharDeviceSt
> > RedClient
> > *client,
> > int size)
> > {
> > + spice_return_val_if_fail(dev != NULL, NULL);
> > +
> > return __spice_char_device_write_buffer_get(dev, client, size,
> > client ? WRITE_BUFFER_ORIGIN_CLIENT :
> > WRITE_BUFFER_ORIGIN_SERVER,
> > 0);
> > @@ -581,6 +589,8 @@ SpiceCharDeviceWriteBuffer
> > *spice_char_device_write_buffer_get(SpiceCharDeviceSt
> > SpiceCharDeviceWriteBuffer
> > *spice_char_device_write_buffer_get_server_no_token(
> > SpiceCharDeviceState *dev, int size)
> > {
> > + spice_return_val_if_fail(dev != NULL, NULL);
> > +
> > return __spice_char_device_write_buffer_get(dev, NULL, size,
> > WRITE_BUFFER_ORIGIN_SERVER_NO_TOKEN, 0);
> > }
> > @@ -597,6 +607,7 @@ void
> > spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
> > SpiceCharDeviceWriteBuffer
> > *write_buf)
> > {
> > spice_assert(dev);
> > + spice_return_if_fail(write_buf != NULL);
>
> I would not abort here as well.
I would return because of write_buf->origin
>
> > /* caller shouldn't add buffers for client that was removed */
> > if (write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT &&
> > !spice_char_device_client_find(dev, write_buf->client)) {
> > @@ -612,9 +623,15 @@ void
> > spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
> > void spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
> > SpiceCharDeviceWriteBuffer
> > *write_buf)
> > {
> > - int buf_origin = write_buf->origin;
> > - uint32_t buf_token_price = write_buf->token_price;
> > - RedClient *client = write_buf->client;
> > + int buf_origin;
> > + uint32_t buf_token_price;
> > + RedClient *client;
> > +
> > + spice_return_if_fail(write_buf != NULL);
>
> Not sure if abort here is okay
Should be ok, else NULL->origin;
>
> > +
> > + buf_origin = write_buf->origin;
> > + buf_token_price = write_buf->token_price;
> > + client = write_buf->client;
> >
> > spice_assert(!ring_item_is_linked(&write_buf->link));
> > if (!dev) {
> > @@ -690,6 +707,8 @@ SpiceCharDeviceState
> > *spice_char_device_state_create(SpiceCharDeviceInstance *si
> > void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState
> > *state,
> > SpiceCharDeviceInstance
> > *sin)
> > {
> > + spice_return_if_fail(state != NULL);
> > + spice_return_if_fail(sin != NULL);
> > spice_debug("sin %p dev_state %p", sin, state);
> > state->sin = sin;
> > sin->st = state;
> > @@ -697,6 +716,7 @@ void
> > spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *state,
> >
> > void *spice_char_device_state_opaque_get(SpiceCharDeviceState *dev)
> > {
> > + spice_return_val_if_fail(dev != NULL, NULL);
> > return dev->opaque;
> > }
> >
> > @@ -721,6 +741,8 @@ static void
> > spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
> >
> > void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
> > {
> > + spice_return_if_fail(char_dev != NULL);
> > +
>
> I would just return in this case, maybe with a spice_warning to log it.
Currently it aborts.
>
> > reds_on_char_device_state_destroy(char_dev);
> > if (char_dev->write_to_dev_timer) {
> > core->timer_remove(char_dev->write_to_dev_timer);
> > @@ -796,6 +818,9 @@ void
> > spice_char_device_client_remove(SpiceCharDeviceState *dev,
> > {
> > SpiceCharDeviceClientState *dev_client;
> >
> > + spice_return_if_fail(dev != NULL);
> > + spice_return_if_fail(client != NULL);
>
> Is it necessary to abort when trying to remove a NULL client?
It is not, I will remove the check, but it will abort anyway on the next
condition.
>
> > +
> > spice_debug("dev_state %p client %p", dev, client);
> > dev_client = spice_char_device_client_find(dev, client);
> >
> > @@ -814,11 +839,16 @@ void
> > spice_char_device_client_remove(SpiceCharDeviceState *dev,
> > int spice_char_device_client_exists(SpiceCharDeviceState *dev,
> > RedClient *client)
> > {
> > + spice_return_val_if_fail(dev != NULL, FALSE);
> > + spice_return_val_if_fail(client != NULL, FALSE);
>
> Is it necessary to abort when looking for a NULL client?
> Before, spice_char_device_client_find would be NULL so function
> would return false.
>
Hmm, it is not necessary. Removed.
Thanks,
Pavel
> > +
> > return (spice_char_device_client_find(dev, client) != NULL);
> > }
> >
> > void spice_char_device_start(SpiceCharDeviceState *dev)
> > {
> > + spice_return_if_fail(dev != NULL);
> > +
> > spice_debug("dev_state %p", dev);
> > dev->running = TRUE;
> > spice_char_device_state_ref(dev);
> > @@ -829,6 +859,8 @@ void spice_char_device_start(SpiceCharDeviceState *dev)
> >
> > void spice_char_device_stop(SpiceCharDeviceState *dev)
> > {
> > + spice_return_if_fail(dev != NULL);
> > +
> > spice_debug("dev_state %p", dev);
> > dev->running = FALSE;
> > dev->active = FALSE;
> > @@ -841,6 +873,8 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
> > {
> > RingItem *client_item;
> >
> > + spice_return_if_fail(dev != NULL);
> > +
> > spice_char_device_stop(dev);
> > dev->wait_for_migrate_data = FALSE;
> > spice_debug("dev_state %p", dev);
> > @@ -871,6 +905,8 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
> >
> > void spice_char_device_wakeup(SpiceCharDeviceState *dev)
> > {
> > + spice_return_if_fail(dev != NULL);
> > +
> > spice_char_device_write_to_device(dev);
> > spice_char_device_read_from_device(dev);
> > }
> > @@ -883,6 +919,7 @@ void
> > spice_char_device_state_migrate_data_marshall_empty(SpiceMarshaller *m)
> > {
> > SpiceMigrateDataCharDevice *mig_data;
> >
> > + spice_return_if_fail(m != NULL);
> > spice_debug(NULL);
> > mig_data = (SpiceMigrateDataCharDevice
> > *)spice_marshaller_reserve_space(m,
> >
> > sizeof(*mig_data));
> > @@ -907,6 +944,8 @@ void
> > spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
> > uint32_t *write_to_dev_tokens_ptr;
> > SpiceMarshaller *m2;
> >
> > + spice_return_if_fail(dev != NULL);
> > + spice_return_if_fail(m != NULL);
> > /* multi-clients are not supported */
> > spice_assert(dev->num_clients == 1);
> > client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->clients),
> > @@ -964,6 +1003,8 @@ int
> > spice_char_device_state_restore(SpiceCharDeviceState *dev,
> > SpiceCharDeviceClientState *client_state;
> > uint32_t client_tokens_window;
> >
> > + spice_return_val_if_fail(dev != NULL, FALSE);
> > + spice_return_val_if_fail(mig_data != NULL, FALSE);
> > spice_assert(dev->num_clients == 1 && dev->wait_for_migrate_data);
> >
> > client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->clients),
> > diff --git a/server/reds.c b/server/reds.c
> > index 1f6774e..4d18ca8 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2983,6 +2983,8 @@ static SpiceCharDeviceState
> > *attach_to_red_agent(SpiceCharDeviceInstance *sin)
> >
> > SPICE_GNUC_VISIBLE void
> > spice_server_char_device_wakeup(SpiceCharDeviceInstance* sin)
> > {
> > + spice_return_if_fail(sin != NULL);
> > +
> > if (!sin->st) {
> > spice_warning("no SpiceCharDeviceState attached to instance %p",
> > sin);
> > return;
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index 6ac1561..0780192 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -508,6 +508,8 @@ SpiceCharDeviceState
> > *spicevmc_device_connect(SpiceCharDeviceInstance *sin,
> > ClientCbs client_cbs = { NULL, };
> > SpiceCharDeviceCallbacks char_dev_cbs = {NULL, };
> >
> > + spice_return_val_if_fail(sin != NULL, NULL);
> > +
> > channel_cbs.config_socket = spicevmc_red_channel_client_config_socket;
> > channel_cbs.on_disconnect = spicevmc_red_channel_client_on_disconnect;
> > channel_cbs.send_item = spicevmc_red_channel_send_item;
> > @@ -552,6 +554,8 @@ void spicevmc_device_disconnect(SpiceCharDeviceInstance
> > *sin)
> > {
> > SpiceVmcState *state;
> >
> > + spice_return_if_fail(sin != NULL);
> > +
> > state = (SpiceVmcState *)spice_char_device_state_opaque_get(sin->st);
> >
> > if (state->recv_from_client_buf) {
> > @@ -569,6 +573,8 @@ SPICE_GNUC_VISIBLE void
> > spice_server_port_event(SpiceCharDeviceInstance *sin, ui
> > {
> > SpiceVmcState *state;
> >
> > + spice_return_if_fail(sin != NULL);
> > +
> > if (sin->st == NULL) {
> > spice_warning("no SpiceCharDeviceState attached to instance %p",
> > sin);
> > return;
More information about the Spice-devel
mailing list