| Message ID | 1485786709-9388-1-git-send-email-fatih.acar@gandi.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 30, 2017 at 03:31:49PM +0100, Fatih Acar wrote: > If we have no disk attached at startup, diskws is left unallocated > but `d_config.num_disks` may change if we attach a disk later. > When a domain is rebooted `evdisable_disk_ejects` is called > this will later result in a segfault if num_disks has changed. > > Expand diskws when num_disks increases. > > Signed-off-by: Fatih Acar <fatih.acar@gandi.net> > Signed-off-by: Nikita Kozlov <nikita.kozlov@gandi.net> > Signed-off-by: Vincent Legout <vincent.legout@gandi.net> > Signed-off-by: Baptiste Daroussin <baptiste.daroussin@gandi.net> > --- > tools/libxl/xl_cmdimpl.c | 34 ++++++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 7e8a8ae..f244e63 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -2517,13 +2517,25 @@ skip_usbdev: > xlu_cfg_destroy(config); > } > > +static void realloc_diskws(libxl_evgen_disk_eject ***diskws, int old_count, int new_count) > +{ > + libxl_evgen_disk_eject **diskws_new; > + > + diskws_new = xcalloc(new_count, sizeof(*diskws_new)); > + memcpy(diskws_new, *diskws, sizeof(**diskws) * old_count); > + free(*diskws); > + *diskws = diskws_new; You didn't check if diskws_new is NULL. Actually you can just use xrealloc here. > +} > + > static void reload_domain_config(uint32_t domid, > - libxl_domain_config *d_config) > + libxl_domain_config *d_config, > + libxl_evgen_disk_eject ***diskws) > { > int rc; > uint8_t *t_data; > int ret, t_len; > libxl_domain_config d_config_new; > + int disk_count; > > /* In case user has used "config-update" to store a new config > * file. > @@ -2534,9 +2546,14 @@ static void reload_domain_config(uint32_t domid, > } > if (t_len > 0) { > LOG("\"xl\" configuration found, using it\n"); > + disk_count = d_config->num_disks; > libxl_domain_config_dispose(d_config); > parse_config_data("<updated>", (const char *)t_data, > t_len, d_config); > + if (d_config->num_disks > disk_count) { > + /* reallocate bigger diskws */ > + realloc_diskws(diskws, disk_count, d_config->num_disks); > + } It seems that you've used "xl config-update" to update the domain configuration. Is this correct? But actually we might want to fix the other code path as well. Please give me some time to go over the code path to see if there is a better approach than this patch. Wei.
On 02/02/2017 12:25, Wei Liu wrote: > > You didn't check if diskws_new is NULL. > > Actually you can just use xrealloc here. > You're right, using xrealloc is much better here. > > It seems that you've used "xl config-update" to update the domain > configuration. Is this correct? > > But actually we might want to fix the other code path as well. > > Please give me some time to go over the code path to see if there is a > better approach than this patch. > Yes, we use "xl config-update" although the documentation states it should not be used. We need this in order to update the domain's kernel before a reboot in our case. Indeed, there should be a better approach than this. We were aware that this fix is not very elegant and will be thankful if you can come up with a better solution.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 7e8a8ae..f244e63 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -2517,13 +2517,25 @@ skip_usbdev: xlu_cfg_destroy(config); } +static void realloc_diskws(libxl_evgen_disk_eject ***diskws, int old_count, int new_count) +{ + libxl_evgen_disk_eject **diskws_new; + + diskws_new = xcalloc(new_count, sizeof(*diskws_new)); + memcpy(diskws_new, *diskws, sizeof(**diskws) * old_count); + free(*diskws); + *diskws = diskws_new; +} + static void reload_domain_config(uint32_t domid, - libxl_domain_config *d_config) + libxl_domain_config *d_config, + libxl_evgen_disk_eject ***diskws) { int rc; uint8_t *t_data; int ret, t_len; libxl_domain_config d_config_new; + int disk_count; /* In case user has used "config-update" to store a new config * file. @@ -2534,9 +2546,14 @@ static void reload_domain_config(uint32_t domid, } if (t_len > 0) { LOG("\"xl\" configuration found, using it\n"); + disk_count = d_config->num_disks; libxl_domain_config_dispose(d_config); parse_config_data("<updated>", (const char *)t_data, t_len, d_config); + if (d_config->num_disks > disk_count) { + /* reallocate bigger diskws */ + realloc_diskws(diskws, disk_count, d_config->num_disks); + } free(t_data); libxl_userdata_unlink(ctx, domid, "xl"); return; @@ -2549,6 +2566,10 @@ static void reload_domain_config(uint32_t domid, "reusing old configuration", rc); libxl_domain_config_dispose(&d_config_new); } else { + if (d_config_new.num_disks > d_config->num_disks) { + /* reallocate bigger diskws */ + realloc_diskws(diskws, d_config->num_disks, d_config_new.num_disks); + } libxl_domain_config_dispose(d_config); /* Steal allocations */ memcpy(d_config, &d_config_new, sizeof(libxl_domain_config)); @@ -2558,7 +2579,8 @@ static void reload_domain_config(uint32_t domid, /* Can update r_domid if domain is destroyed */ static domain_restart_type handle_domain_death(uint32_t *r_domid, libxl_event *event, - libxl_domain_config *d_config) + libxl_domain_config *d_config, + libxl_evgen_disk_eject ***diskws) { domain_restart_type restart = DOMAIN_RESTART_NONE; libxl_action_on_shutdown action; @@ -2614,12 +2636,12 @@ static domain_restart_type handle_domain_death(uint32_t *r_domid, break; case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME: - reload_domain_config(*r_domid, d_config); + reload_domain_config(*r_domid, d_config, diskws); restart = DOMAIN_RESTART_RENAME; break; case LIBXL_ACTION_ON_SHUTDOWN_RESTART: - reload_domain_config(*r_domid, d_config); + reload_domain_config(*r_domid, d_config, diskws); restart = DOMAIN_RESTART_NORMAL; /* fall-through */ case LIBXL_ACTION_ON_SHUTDOWN_DESTROY: @@ -2630,7 +2652,7 @@ static domain_restart_type handle_domain_death(uint32_t *r_domid, break; case LIBXL_ACTION_ON_SHUTDOWN_SOFT_RESET: - reload_domain_config(*r_domid, d_config); + reload_domain_config(*r_domid, d_config, diskws); restart = DOMAIN_RESTART_SOFT_RESET; break; @@ -3137,7 +3159,7 @@ start: LOG("Domain %u has shut down, reason code %d 0x%x", domid, event->u.domain_shutdown.shutdown_reason, event->u.domain_shutdown.shutdown_reason); - switch (handle_domain_death(&domid, event, &d_config)) { + switch (handle_domain_death(&domid, event, &d_config, &diskws)) { case DOMAIN_RESTART_SOFT_RESET: domid_soft_reset = domid; domid = INVALID_DOMID;