| Message ID | 20200819144309.67579-1-liq3ea@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: vdpa: omit check return of g_malloc | expand |
On 8/19/20 4:43 PM, Li Qiang wrote: > If g_malloc fails, the application will be terminated. Which we don't want... better to use g_try_malloc() instead? > No need to check the return value of g_malloc. > > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > hw/virtio/vhost-vdpa.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 4580f3efd8..403ae3ae07 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, > struct vhost_vdpa_config *config; > int ret; > unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); > + > config = g_malloc(size + config_size); > - if (config == NULL) { > - return -1; > - } > config->off = offset; > config->len = size; > memcpy(config->buf, data, size); > @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, > int ret; > > v_config = g_malloc(config_len + config_size); > - if (v_config == NULL) { > - return -1; > - } > v_config->len = config_len; > v_config->off = 0; > ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config); >
Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年8月19日周三 下午11:07写道： > > On 8/19/20 4:43 PM, Li Qiang wrote: > > If g_malloc fails, the application will be terminated. > > Which we don't want... better to use g_try_malloc() instead? I don't think so. If g_malloc return NULL it means a critical situation I think terminate the application is OK. Though I don't find any rule/practices the qemu code base uses g_malloc far more than g_try_malloc. Thanks, Li Qiang > > > No need to check the return value of g_malloc. > > > > Signed-off-by: Li Qiang <liq3ea@163.com> > > --- > > hw/virtio/vhost-vdpa.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 4580f3efd8..403ae3ae07 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, > > struct vhost_vdpa_config *config; > > int ret; > > unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); > > + > > config = g_malloc(size + config_size); > > - if (config == NULL) { > > - return -1; > > - } > > config->off = offset; > > config->len = size; > > memcpy(config->buf, data, size); > > @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, > > int ret; > > > > v_config = g_malloc(config_len + config_size); > > - if (v_config == NULL) { > > - return -1; > > - } > > v_config->len = config_len; > > v_config->off = 0; > > ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config); > > >
On 19/08/2020 16:43, Li Qiang wrote: > If g_malloc fails, the application will be terminated. > No need to check the return value of g_malloc. > > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > hw/virtio/vhost-vdpa.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 4580f3efd8..403ae3ae07 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, > struct vhost_vdpa_config *config; > int ret; > unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); > + > config = g_malloc(size + config_size); > - if (config == NULL) { > - return -1; > - } > config->off = offset; > config->len = size; > memcpy(config->buf, data, size); > @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, > int ret; > > v_config = g_malloc(config_len + config_size); > - if (v_config == NULL) { > - return -1; > - } > v_config->len = config_len; > v_config->off = 0; > ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config); > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Li Qiang <liq3ea@gmail.com> writes: > Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年8月19日周三 下午11:07写道： >> >> On 8/19/20 4:43 PM, Li Qiang wrote: >> > If g_malloc fails, the application will be terminated. >> >> Which we don't want... better to use g_try_malloc() instead? > > I don't think so. If g_malloc return NULL it means a critical > situation I think terminate the application > is OK. Though I don't find any rule/practices the qemu code base uses > g_malloc far more than > g_try_malloc. g_try_malloc is only for cases you could recover from, by either deferring or doing something else. A straight out of memory failure is fatal. Arguably a bunch of the try_malloc's in the code base should be straight mallocs. The ELF loaders load_symbols does it because I guess having the symbols is a bonus and you could still run the program if a) there was enough memory to run and b) your symbol table was very large. > > Thanks, > Li Qiang > >> >> > No need to check the return value of g_malloc. >> > >> > Signed-off-by: Li Qiang <liq3ea@163.com> >> > --- >> > hw/virtio/vhost-vdpa.c | 7 +------ >> > 1 file changed, 1 insertion(+), 6 deletions(-) >> > >> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >> > index 4580f3efd8..403ae3ae07 100644 >> > --- a/hw/virtio/vhost-vdpa.c >> > +++ b/hw/virtio/vhost-vdpa.c >> > @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, >> > struct vhost_vdpa_config *config; >> > int ret; >> > unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); >> > + >> > config = g_malloc(size + config_size); >> > - if (config == NULL) { >> > - return -1; >> > - } >> > config->off = offset; >> > config->len = size; >> > memcpy(config->buf, data, size); >> > @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, >> > int ret; >> > >> > v_config = g_malloc(config_len + config_size); >> > - if (v_config == NULL) { >> > - return -1; >> > - } >> > v_config->len = config_len; >> > v_config->off = 0; >> > ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config); >> > >>
Li Qiang <liq3ea@163.com> writes: > If g_malloc fails, the application will be terminated. > No need to check the return value of g_malloc. > > Signed-off-by: Li Qiang <liq3ea@163.com> I think this is fine because a failure to config the dev would just cause an error_report later and it's extremely unlikely you don't have enough memory to get that far anyway. Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Le 18/09/2020 à 14:53, Laurent Vivier a écrit : > On 19/08/2020 16:43, Li Qiang wrote: >> If g_malloc fails, the application will be terminated. >> No need to check the return value of g_malloc. >> >> Signed-off-by: Li Qiang <liq3ea@163.com> >> --- >> hw/virtio/vhost-vdpa.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >> index 4580f3efd8..403ae3ae07 100644 >> --- a/hw/virtio/vhost-vdpa.c >> +++ b/hw/virtio/vhost-vdpa.c >> @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, >> struct vhost_vdpa_config *config; >> int ret; >> unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); >> + >> config = g_malloc(size + config_size); >> - if (config == NULL) { >> - return -1; >> - } >> config->off = offset; >> config->len = size; >> memcpy(config->buf, data, size); >> @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, >> int ret; >> >> v_config = g_malloc(config_len + config_size); >> - if (v_config == NULL) { >> - return -1; >> - } >> v_config->len = config_len; >> v_config->off = 0; >> ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config); >> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com> > > Applied to my trivial-patches branch. Thanks, Laurent
On 9/18/20 8:12 AM, Alex Bennée wrote: > > Li Qiang <liq3ea@gmail.com> writes: > >> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年8月19日周三 下午11:07写道： >>> >>> On 8/19/20 4:43 PM, Li Qiang wrote: >>>> If g_malloc fails, the application will be terminated. >>> >>> Which we don't want... better to use g_try_malloc() instead? >> >> I don't think so. If g_malloc return NULL it means a critical >> situation I think terminate the application >> is OK. Though I don't find any rule/practices the qemu code base uses >> g_malloc far more than >> g_try_malloc. > > g_try_malloc is only for cases you could recover from, by either > deferring or doing something else. A straight out of memory failure is > fatal. > > Arguably a bunch of the try_malloc's in the code base should be straight > mallocs. The ELF loaders load_symbols does it because I guess having the > symbols is a bonus and you could still run the program if a) there was > enough memory to run and b) your symbol table was very large. If the amount of memory being allocated is under user control (such as from an elf header or qcow2 image), you want g_try_malloc() to prevent against malicious users requesting outrageous amounts. But if it is solely under programmer control, where the maximum amount requested is not going to be a problem unless you are already truly out of memory for other reasons, g_malloc() is appropriate. A potential rule of thumb might be that if you know it is less than 1M of memory, it's not worth trying to recover from.
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 4580f3efd8..403ae3ae07 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, struct vhost_vdpa_config *config; int ret; unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); + config = g_malloc(size + config_size); - if (config == NULL) { - return -1; - } config->off = offset; config->len = size; memcpy(config->buf, data, size); @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, int ret; v_config = g_malloc(config_len + config_size); - if (v_config == NULL) { - return -1; - } v_config->len = config_len; v_config->off = 0; ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
If g_malloc fails, the application will be terminated. No need to check the return value of g_malloc. Signed-off-by: Li Qiang <liq3ea@163.com> --- hw/virtio/vhost-vdpa.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)